From b40465e7b73b5fca44c9c9748c18ca9dbce4e67d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samy=20Pess=C3=A9?= Date: Thu, 19 Mar 2026 19:27:00 +0100 Subject: [PATCH] Fix normalization of URL causing invalid redirections. (#4130) --- .changeset/mighty-ghosts-invent.md | 5 ++ packages/gitbook/src/lib/data/urls.test.ts | 61 +++++++++---------- packages/gitbook/src/lib/data/urls.ts | 68 +++++++++------------- packages/gitbook/src/middleware.ts | 22 +++---- 4 files changed, 72 insertions(+), 84 deletions(-) create mode 100644 .changeset/mighty-ghosts-invent.md diff --git a/.changeset/mighty-ghosts-invent.md b/.changeset/mighty-ghosts-invent.md new file mode 100644 index 000000000..846fc0541 --- /dev/null +++ b/.changeset/mighty-ghosts-invent.md @@ -0,0 +1,5 @@ +--- +"gitbook": patch +--- + +Fix normalization of URL causing invalid redirections. diff --git a/packages/gitbook/src/lib/data/urls.test.ts b/packages/gitbook/src/lib/data/urls.test.ts index 47a0d8d99..902b7fa7a 100644 --- a/packages/gitbook/src/lib/data/urls.test.ts +++ b/packages/gitbook/src/lib/data/urls.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'bun:test'; -import { decodeURLPath, getURLLookupAlternatives, normalizeURL } from './urls'; +import { getURLLookupAlternatives, normalizeURL } from './urls'; describe('getURLLookupAlternatives', () => { it('should return all URLs up to the root', () => { @@ -405,12 +405,20 @@ describe('normalizeURL', () => { new URL('https://docs.mycompany.com/hello/there') ); }); + + it('should throw for URL paths exceeding 2048 characters', () => { + const longPath = '/a'.repeat(1025); // 2050 chars + const url = new URL(`https://docs.mycompany.com${longPath}`); + expect(() => { + normalizeURL(url); + }).toThrow('URL path is too long'); + }); }); -describe('decodeURLPath', () => { +describe('normalizeURL with encoded paths', () => { it('should decode encoded path components', () => { const url = new URL('https://docs.mycompany.com/helloworld/tes%74'); - const result = decodeURLPath(url); + const result = normalizeURL(url); expect(result.pathname).toBe('/helloworld/test'); expect(result.toString()).toBe('https://docs.mycompany.com/helloworld/test'); }); @@ -419,48 +427,49 @@ describe('decodeURLPath', () => { // Double encoded: tes%2574 → tes%74 → test // %2574 decodes as: %25 → %, leaving %74, which then decodes to t const url = new URL('https://docs.mycompany.com/helloworld/tes%2574'); - const result = decodeURLPath(url); + const result = normalizeURL(url); expect(result.pathname).toBe('/helloworld/test'); - // Triple encoding (tes%252574) exceeds the 2-pass limit and is rejected - expect(() => { - decodeURLPath(new URL('https://docs.mycompany.com/helloworld/tes%252574')); - }).toThrow('URL path is malformed'); + // Triple encoding is also normalized through the nested normalizeURL flow. + const tripleEncoded = normalizeURL( + new URL('https://docs.mycompany.com/helloworld/tes%252574') + ); + expect(tripleEncoded.pathname).toBe('/helloworld/test'); }); it('should throw for malformed percent-encoding in the path', () => { // Invalid hex digits in percent-encoding expect(() => { - decodeURLPath(new URL('https://docs.mycompany.com/helloworld/%ZZ')); + normalizeURL(new URL('https://docs.mycompany.com/helloworld/%ZZ')); }).toThrow('URL path is malformed'); // Incomplete or invalid UTF-8 sequence expect(() => { - decodeURLPath(new URL('https://docs.mycompany.com/helloworld/%E0%A4%A')); + normalizeURL(new URL('https://docs.mycompany.com/helloworld/%E0%A4%A')); }).toThrow('URL path is malformed'); // Trailing '%' without two following hex digits expect(() => { - decodeURLPath(new URL('https://docs.mycompany.com/helloworld/trailing%')); + normalizeURL(new URL('https://docs.mycompany.com/helloworld/trailing%')); }).toThrow('URL path is malformed'); }); it.skip('should throw an error for invalid characters in the path', () => { expect(() => { - decodeURLPath(new URL('https://docs.mycompany.com/hello:world')); + normalizeURL(new URL('https://docs.mycompany.com/hello:world')); }).toThrow('URL path contains invalid characters'); expect(() => { - decodeURLPath(new URL('https://docs.mycompany.com/hello%3Btest')); + normalizeURL(new URL('https://docs.mycompany.com/hello%3Btest')); }).toThrow('URL path contains invalid characters'); expect(() => { - decodeURLPath(new URL('https://docs.mycompany.com/hello%40anchor')); + normalizeURL(new URL('https://docs.mycompany.com/hello%40anchor')); }).toThrow('URL path contains invalid characters'); // %20 (space) re-encodes to %20 after decoding, so the path is stable // and considered fully decoded — it should not throw. - expect(decodeURLPath(new URL('https://docs.mycompany.com/hello%20world')).pathname).toBe( + expect(normalizeURL(new URL('https://docs.mycompany.com/hello%20world')).pathname).toBe( '/hello%20world' ); }); @@ -470,33 +479,25 @@ describe('decodeURLPath', () => { // %25252525 needs 4 passes: %25252525 → %252525 → %2525 → %25 → % const url = new URL('https://docs.mycompany.com/%25252525'); expect(() => { - decodeURLPath(url); + normalizeURL(url); }).toThrow('URL path is malformed'); const deepUrl = new URL('https://docs.mycompany.com/%2525252525252525'); expect(() => { - decodeURLPath(deepUrl); + normalizeURL(deepUrl); }).toThrow('URL path is malformed'); }); - it('should throw for URL paths exceeding 2048 characters', () => { - const longPath = '/a'.repeat(1025); // 2050 chars - const url = new URL(`https://docs.mycompany.com${longPath}`); - expect(() => { - decodeURLPath(url); - }).toThrow('URL path is too long'); - }); - // TODO: should we do that actually? it.skip('should throw an error if the encoded path contains /', () => { expect(() => { - decodeURLPath(new URL('https://docs.mycompany.com/hello%2Fworld')); + normalizeURL(new URL('https://docs.mycompany.com/hello%2Fworld')); }).toThrow('URL path contains invalid characters'); }); it('should not decode search params or hash fragments', () => { const url = new URL('https://docs.mycompany.com/helloworld/tes%74?query=%74est#sec%74ion'); - const result = decodeURLPath(url); + const result = normalizeURL(url); expect(result.pathname).toBe('/helloworld/test'); expect(result.search).toBe('?query=%74est'); expect(result.hash).toBe('#sec%74ion'); @@ -508,7 +509,7 @@ describe('decodeURLPath', () => { const url = new URL(`https://docs.mycompany.com/short-path?jwt_token=${fakeJwt}`); // The path itself is well within the limit; only the query param is huge. expect(url.pathname.length).toBeLessThan(2048); - const result = decodeURLPath(url); + const result = normalizeURL(url); expect(result.pathname).toBe('/short-path'); // The query string must pass through untouched. expect(result.searchParams.get('jwt_token')).toBe(fakeJwt); @@ -521,9 +522,9 @@ describe('decodeURLPath', () => { const url = new URL( `https://docs.mycompany.com/some-page?filter=${encodeURIComponent(risonValue)}` ); - const result = decodeURLPath(url); + const result = normalizeURL(url); expect(result.pathname).toBe('/some-page'); - // The rison param must survive decodeURLPath intact. + // The rison param must survive normalizeURL intact. expect(result.searchParams.get('filter')).toBe(risonValue); }); }); diff --git a/packages/gitbook/src/lib/data/urls.ts b/packages/gitbook/src/lib/data/urls.ts index 9736abb2c..4355396f1 100644 --- a/packages/gitbook/src/lib/data/urls.ts +++ b/packages/gitbook/src/lib/data/urls.ts @@ -1,5 +1,5 @@ import { isProxyRootRequest } from '../proxy'; -import { DataFetcherError } from './errors'; +import { DataFetcherError, getExposableError } from './errors'; /** * For a given GitBook URL, return a list of alternative URLs that could be matched against to lookup the content. @@ -118,46 +118,39 @@ export function getURLLookupAlternatives(input: URL) { return { urls: alternatives, basePath, changeRequest, revision }; } +/** + * Normalize the URL in a request and redirect if the normalized URL is different from the original one. + */ +export function normalizeRequestURL(url: URL): Response | null { + try { + const normalizedURL = normalizeURL(url); + + if (normalizedURL.toString() !== url.toString()) { + return Response.redirect(normalizedURL.toString(), 302); + } + + return null; + } catch (error) { + const sanitized = getExposableError(error); + return new Response(sanitized.message, { status: sanitized.code }); + } +} + /** * Normalize a URL to remove duplicate slashes and trailing slashes * and transform the pathname to lowercase. */ export function normalizeURL(url: URL) { const result = new URL(url); - result.pathname = url.pathname.replace(/\/{2,}/g, '/').replace(/\/$/, ''); - return result; -} -/** - * This function checks if a decoded URL path segment contains characters that are not allowed - * in GitBook content paths. These characters are valid in generic RFC 3986 URL paths, but are - * rejected here as an application-level constraint for GitBook routing and security. - * https://developer.mozilla.org/en-US/docs/Glossary/Percent-encoding - * "%" itself is excluded because it could be part of percent-encoding and may require decoding. - */ -// function containsInvalidURLCharacters(segment: string): boolean { -// const invalidCharacters = [ -// ':', -// '/', -// '?', -// '#', -// '[', -// ']', -// '@', -// '!', -// '$', -// '&', -// "'", -// '(', -// ')', -// '*', -// '+', -// ',', -// ';', -// '=', -// ]; -// return invalidCharacters.some((char) => segment.includes(char)); -// } + // Reject excessively long paths up-front to bound per-request work. + if (url.pathname.length > 2048) { + throw new DataFetcherError('URL path is too long.', 400); + } + + result.pathname = url.pathname.replace(/\/{2,}/g, '/').replace(/\/$/, ''); + return decodeURLPath(result); +} /** * Decode the url path component, we redirect URLs with encoded path components @@ -167,12 +160,7 @@ export function normalizeURL(url: URL) { * percent-encoding. Legitimate URLs are at most singly encoded; double-encoding covers * any reasonable proxy behaviour. */ -export function decodeURLPath(url: URL): URL { - // Reject excessively long paths up-front to bound per-request work. - if (url.pathname.length > 2048) { - throw new DataFetcherError(`URL path is too long: ${url.pathname}`, 400); - } - +function decodeURLPath(url: URL): URL { let current = url; for (let i = 0; i < 2; i++) { diff --git a/packages/gitbook/src/middleware.ts b/packages/gitbook/src/middleware.ts index 0b7fd3697..28dbeeb8d 100644 --- a/packages/gitbook/src/middleware.ts +++ b/packages/gitbook/src/middleware.ts @@ -13,10 +13,9 @@ import { getContentSecurityPolicy } from '@/lib/csp'; import { validateSerializedCustomization } from '@/lib/customization'; import { DataFetcherError, - decodeURLPath, getVisitorAuthBasePath, lookupPublishedContentByUrl, - normalizeURL, + normalizeRequestURL, throwIfDataError, } from '@/lib/data'; import { GITBOOK_OAUTH_SERVER_URL, isGitBookAssetsHostURL, isGitBookHostURL } from '@/lib/env'; @@ -60,18 +59,6 @@ export async function middleware(request: NextRequest) { try { const requestURL = new URL(request.url); - // Redirect to normalize the URL - const normalized = normalizeURL(requestURL); - if (normalized.toString() !== requestURL.toString()) { - return NextResponse.redirect(normalized.toString()); - } - - // If the URL path is encoded, decode it and redirect to the decoded URL - const decoded = decodeURLPath(requestURL); - if (decoded.toString() !== requestURL.toString()) { - return NextResponse.redirect(decoded.toString()); - } - // Reject malicious requests const rejectResponse = await validateServerActionRequest(request); if (rejectResponse) { @@ -152,6 +139,13 @@ async function serveSiteRoutes(requestURL: URL, request: NextRequest) { } const { url: siteRequestURL, mode } = match; + + // Normalize URL after extracting the URL from the request to make sure the client is redirected to the proper one + const normalizationResponse = normalizeRequestURL(siteRequestURL); + if (normalizationResponse) { + return normalizationResponse; + } + const imagesContextId = getImageResizingContextId(siteRequestURL); /** * Serve image resizing requests (all requests containing `/~gitbook/image`).