From 0b19c92d44feaf7fab8b02fd92eb071fd83c5c72 Mon Sep 17 00:00:00 2001 From: Alan Agius <17563226+alan-agius4@users.noreply.github.com> Date: Fri, 29 May 2026 18:22:12 +0000 Subject: [PATCH] fix(platform-server): throw on suspicious URLs and restrict protocol-relative URLs Currently, the platform-server attempts to neutralize URL hijacking and SSRF bypasses by collapsing multiple leading slashes in relative paths. However, sophisticated bypasses using obfuscated protocols (e.g., carriage returns or newlines) or relative-like backslash paths can still lead to unexpected origin takeovers. This commit improves security by doing the following: - Rejects protocol-relative URLs by throwing an error if they are not explicitly permitted via `allowProtocolRelative`. - Strictly validates resolved URLs against the provided origin using `isSafeOriginChange`. If a URL unexpectedly shifts origins, an error is thrown. - Permits origin changes only when standard absolute http/https protocols are explicitly declared in the input URL. --- packages/platform-server/src/url.ts | 77 ++++++++----- .../platform-server/test/integration_spec.ts | 56 ++++++--- .../test/platform_location_spec.ts | 108 +++++------------- packages/platform-server/test/url_spec.ts | 15 ++- 4 files changed, 129 insertions(+), 127 deletions(-) diff --git a/packages/platform-server/src/url.ts b/packages/platform-server/src/url.ts index ad828ffeb76..3d4418506ff 100644 --- a/packages/platform-server/src/url.ts +++ b/packages/platform-server/src/url.ts @@ -6,6 +6,11 @@ * found in the LICENSE file at https://angular.dev/license */ +/** + * Matches http: or https: + */ +const HTTP_OR_HTTPS_PROTOCOL_REGEX = /^https?:/i; + /** * Options for {@link resolveUrl}. */ @@ -36,17 +41,28 @@ export function resolveUrl( origin?: string | URL, options: ResolveUrlOptions = {}, ): URL | null { + const originUrl = typeof origin === 'string' ? new URL('/', origin) : origin; + if (!urlStr) { - return origin !== undefined ? new URL('/', origin) : null; + return originUrl || null; } urlStr = urlStr.trim(); // Fast-path: if the URL is a valid, standard absolute URL, parse and return it immediately. + let resolved: URL | undefined; try { - return new URL(urlStr); + resolved = new URL(urlStr); } catch {} + if (resolved) { + if (originUrl && !isSafeOriginChange(resolved, originUrl, urlStr)) { + throwSuspiciousUrlError(urlStr); + } + + return resolved; + } + // We identify and throw on malformed absolute URLs (like double port). // Per the WHATWG URL standard, parsing an input starting with a scheme (like 'http:') against // a standard base (like 'http://fake') ignores the base argument and parses strictly as an @@ -56,7 +72,7 @@ export function resolveUrl( throw new Error(`Invalid URL: ${urlStr}`); } - if (origin === undefined) { + if (!originUrl) { return null; } @@ -64,35 +80,40 @@ export function resolveUrl( // Check if we have a legitimate protocol-relative URL (starts with '//' and not a duplicate/backslash bypass) // and we are configured to allow and preserve standard cross-origin protocol-relative requests. - const isProtocolRelative = - allowProtocolRelative && - urlStr[0] === '/' && - urlStr[1] === '/' && - urlStr.length > 2 && - urlStr[2] !== '/' && - urlStr[2] !== '\\'; + if (urlStr.startsWith('//')) { + if (!allowProtocolRelative) { + throw new Error(`Protocol relative URLs are not allowed in this context. URL: ${urlStr}`); + } - if (isProtocolRelative) { return new URL(urlStr, origin); } - // Safe relative path preservation: if a relative path has no leading forward or backward slashes, - // we do not prepend any slash so the native URL constructor can resolve it correctly relative - // to trailing-slash sub-paths (e.g., 'testing' against 'http://localhost/foo/' -> 'http://localhost/foo/testing'). - const startsWithSlash = urlStr[0] === '/' || urlStr[0] === '\\'; - if (!startsWithSlash) { - return new URL(urlStr, origin); + resolved = new URL(urlStr, origin); + + if (!isSafeOriginChange(resolved, originUrl, urlStr)) { + throwSuspiciousUrlError(urlStr); } - // For other relative inputs starting with slashes, we collapse all consecutive leading forward/backward - // slashes to a single forward slash. This guarantees consistent same-origin path representation and - // blocks any hostname hijack or takeover attempts. - let startIdx = 0; - while (startIdx < urlStr.length && (urlStr[startIdx] === '/' || urlStr[startIdx] === '\\')) { - startIdx++; - } - const pathWithoutLeadingSlashes = urlStr.slice(startIdx); - const normalizedPath = '/' + pathWithoutLeadingSlashes; - - return new URL(normalizedPath, origin); + return resolved; +} + +/** + * Throws a suspicious URL error indicating a security bypass attempt. + */ +function throwSuspiciousUrlError(urlStr: string): never { + throw new Error( + `URL ${urlStr} changed origin unexpectedly. This is suspicious and may indicate a security bypass attempt.`, + ); +} + +/** + * Checks if the origin has changed in a safe way. + * + * @param resolved The resolved URL. + * @param origin The origin URL. + * @param urlStr The URL string. + * @returns True if the origin has changed in a safe way, false otherwise. + */ +function isSafeOriginChange(resolved: URL, origin: URL, urlStr: string): boolean { + return origin.origin === resolved.origin || HTTP_OR_HTTPS_PROTOCOL_REGEX.test(urlStr); } diff --git a/packages/platform-server/test/integration_spec.ts b/packages/platform-server/test/integration_spec.ts index 4756c73f87b..36191c1576e 100644 --- a/packages/platform-server/test/integration_spec.ts +++ b/packages/platform-server/test/integration_spec.ts @@ -1456,7 +1456,7 @@ class HiddenModule {} }); }); - it('prevents SSRF bypasses via backslash URLs in HttpClient', async () => { + it('prevents SSRF bypasses via backslash URLs in HttpClient by throwing a suspicious origin error', async () => { const platform = platformServer([ { provide: INITIAL_CONFIG, @@ -1467,11 +1467,16 @@ class HiddenModule {} const mock = ref.injector.get(HttpTestingController); const http = ref.injector.get(HttpClient); ref.injector.get(NgZone).run(() => { - http.get('/\\evil.com/api').subscribe(); + http.get('/\\evil.com/api').subscribe({ + next: () => fail('Expected request to fail, but it succeeded.'), + error: (err) => { + expect(err.message).toBe( + `URL /\\evil.com/api changed origin unexpectedly. This is suspicious and may indicate a security bypass attempt.`, + ); + }, + }); - // To prevent SSRF, we ensures it's forced as a relative path and backslashes - // inside path-relative segments are normalized via URL constructor, generating a safe URL. - mock.expectOne('http://localhost:4000/evil.com/api').flush('safe'); + mock.verify(); }); }); }); @@ -1579,25 +1584,50 @@ class HiddenModule {} }); }); - it('should treat backslash bypass SSRF attempts in relative requests strictly as pathnames', async () => { + it('should reject backslash bypass SSRF attempts in relative requests and throw a suspicious origin error', async () => { const badUrls = [ '/\\attacker.com', '\\\\attacker.com', - '///attacker.com', - '//\\attacker.com', ' /\\attacker.com', '\r\n/\\attacker.com', ]; ref.injector.get(NgZone).run(() => { for (const badUrl of badUrls) { - http.get(badUrl).subscribe((body) => { - expect(body).toEqual('success!'); + http.get(badUrl).subscribe({ + next: () => fail(`Expected request for ${badUrl} to fail, but it succeeded.`), + error: (err) => { + expect(err.message).toBe( + `URL ${badUrl.trim()} changed origin unexpectedly. This is suspicious and may indicate a security bypass attempt.`, + ); + }, }); - mock - .expectOne('http://localhost:4000/attacker.com', `URL: ${badUrl}`) - .flush('success!'); } + + mock.verify(); + }); + }); + + it('should reject obfuscated protocal SSRF attempts in relative requests and throw a suspicious origin error', async () => { + const badUrls = [ + 'htt\rps://evil.com/path', + ' htt\rps://evil.com/path', + '\r\nhtt\rps://evil.com/path', + ]; + + ref.injector.get(NgZone).run(() => { + for (const badUrl of badUrls) { + http.get(badUrl).subscribe({ + next: () => fail(`Expected request for ${badUrl} to fail, but it succeeded.`), + error: (err) => { + expect(err.message).toBe( + `URL ${badUrl.trim()} changed origin unexpectedly. This is suspicious and may indicate a security bypass attempt.`, + ); + }, + }); + } + + mock.verify(); }); }); diff --git a/packages/platform-server/test/platform_location_spec.ts b/packages/platform-server/test/platform_location_spec.ts index ec5a1f6e5ab..090c8e67ac1 100644 --- a/packages/platform-server/test/platform_location_spec.ts +++ b/packages/platform-server/test/platform_location_spec.ts @@ -135,94 +135,38 @@ import {INITIAL_CONFIG, platformServer} from '@angular/platform-server'; location.pushState(null, 'Test', '/foo#bar'); }); - it('neutralizes hostname hijack attempts', async () => { - const urls = ['/\\attacker.com/deep/path', '//attacker.com/deep/path']; - - for (const url of urls) { - const platform = platformServer([ - { - provide: INITIAL_CONFIG, - useValue: { - document: '', - // This should be treated as relative URL. - // Example: `req.url: '//attacker.com/deep/path'` where request - // to express server is 'http://localhost:4200//attacker.com/deep/path'. - url, - }, + it('should throw on hostname hijack attempts to prevent origin hijack', async () => { + const platform = platformServer([ + { + provide: INITIAL_CONFIG, + useValue: { + document: '', + url: '/\\attacker.com/deep/path', }, - ]); + }, + ]); - const location = platform.injector.get(PlatformLocation); - platform.destroy(); - - expect(location.hostname).withContext(`hostname for URL: "${url}"`).toBe(''); - expect(location.pathname) - .withContext(`pathname for URL: "${url}"`) - .toBe('/attacker.com/deep/path'); - } + expect(() => platform.injector.get(DOCUMENT)).toThrowError( + `URL /\\attacker.com/deep/path changed origin unexpectedly. This is suspicious and may indicate a security bypass attempt.`, + ); + platform.destroy(); }); - it('should set the proper document location when the URL has leading slashes to prevent origin hijack', async () => { - const urls = ['/\\attacker.com/deep/path', '//attacker.com/deep/path']; - - for (const url of urls) { - const platform = platformServer([ - { - provide: INITIAL_CONFIG, - useValue: { - document: '', - url, - }, + it('should throw on protocol-relative URLs in INITIAL_CONFIG', async () => { + const platform = platformServer([ + { + provide: INITIAL_CONFIG, + useValue: { + document: '', + url: '//attacker.com/deep/path', }, - ]); + }, + ]); - const doc = platform.injector.get(DOCUMENT); - platform.destroy(); - - expect(doc.location.origin).not.toBe('http://attacker.com'); - expect(doc.location.pathname).toBe('/attacker.com/deep/path'); - } - }); - - it('should not expose protocol-relative URLs on the location to prevent open redirect and SSRF bypasses', async () => { - const urls = ['/\\attacker.com/deep/path', '//attacker.com/deep/path']; - const origins = [undefined, 'http://localhost:4200']; - - for (const url of urls) { - for (const origin of origins) { - const providers: any[] = [ - { - provide: INITIAL_CONFIG, - useValue: { - document: '', - url, - }, - }, - ]; - - if (origin) { - providers.push({ - provide: DOCUMENT, - useValue: { - location: { - origin, - }, - }, - }); - } - - const platform = platformServer(providers); - const location = platform.injector.get(PlatformLocation) as any; - platform.destroy(); - - // A relative redirect URL starting with // or /\ is normalized by browsers to a protocol-relative URL. - // The PlatformLocation.url property MUST NOT expose these unsafe patterns. - const isVulnerable = location.url.startsWith('//') || location.url.startsWith('/\\'); - expect(isVulnerable) - .withContext(`URL: "${url}", origin: "${origin}", location.url: "${location.url}"`) - .toBeFalse(); - } - } + expect(() => platform.injector.get(DOCUMENT)).toThrowError( + `Protocol relative URLs are not allowed in this context. URL: //attacker.com/deep/path`, + ); + platform.destroy(); }); }); })(); diff --git a/packages/platform-server/test/url_spec.ts b/packages/platform-server/test/url_spec.ts index be873a1d0d1..b348dbc608d 100644 --- a/packages/platform-server/test/url_spec.ts +++ b/packages/platform-server/test/url_spec.ts @@ -17,12 +17,12 @@ describe('resolveUrl', () => { expect(url.hash).toBe('#hash'); }); - it('should neutralize backslash-prefixed hijack attempts by forcing them same-origin', () => { + it('should throw on backslash-prefixed hijack attempts', () => { const urls = ['/\\attacker.com/deep/path', '\\\\attacker.com/deep/path']; for (const url of urls) { - const parsed = resolveUrl(url, 'http://test.com'); - expect(parsed.origin).toBe('http://test.com'); - expect(parsed.pathname).toBe('/attacker.com/deep/path'); + expect(() => resolveUrl(url, 'http://test.com')).toThrowError( + `URL ${url} changed origin unexpectedly. This is suspicious and may indicate a security bypass attempt.`, + ); } }); @@ -47,6 +47,13 @@ describe('resolveUrl', () => { ); } }); + + it('should throw on obfuscated protocols attempting to change origin', () => { + const url = 'ht\ntp://evil.com/path'; + expect(() => resolveUrl(url, 'http://test.com')).toThrowError( + `URL ${url} changed origin unexpectedly. This is suspicious and may indicate a security bypass attempt.`, + ); + }); }); describe('without origin', () => {