mirror of
https://github.com/angular/angular.git
synced 2026-09-14 13:54:52 +08:00
fix(platform-server): throw on suspicious URLs and restrict protocol-relative URLs
Backports the security fixes from: - https://github.com/angular/angular/pull/68973 - https://github.com/angular/angular/pull/69018
This commit is contained in:
committed by
Pawel Kozlowski
parent
a0193fa1bc
commit
e2fb854d55
@@ -16,6 +16,8 @@ import {
|
||||
import {inject, Injectable, Provider} from '@angular/core';
|
||||
import {Observable} from 'rxjs';
|
||||
|
||||
import {parseUrl} from './url';
|
||||
|
||||
@Injectable()
|
||||
export class ServerXhr implements XhrFactory {
|
||||
private xhrImpl: typeof import('xhr2') | undefined;
|
||||
@@ -41,10 +43,21 @@ export class ServerXhr implements XhrFactory {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Regex to match a URL schema.
|
||||
*/
|
||||
const URL_SCHEMA_REGEXP = /^(?:[a-zA-Z][a-zA-Z0-9+\-.]*:)/;
|
||||
|
||||
function relativeUrlsTransformerInterceptorFn(
|
||||
request: HttpRequest<unknown>,
|
||||
next: HttpHandlerFn,
|
||||
): Observable<HttpEvent<unknown>> {
|
||||
const trimmedUrl = request.url.trim();
|
||||
if (URL_SCHEMA_REGEXP.test(trimmedUrl)) {
|
||||
// URLs with a schema should be left unchanged.
|
||||
return next(request);
|
||||
}
|
||||
|
||||
const platformLocation = inject(PlatformLocation);
|
||||
const {href, protocol, hostname, port} = platformLocation;
|
||||
if (!protocol.startsWith('http')) {
|
||||
@@ -58,9 +71,11 @@ function relativeUrlsTransformerInterceptorFn(
|
||||
|
||||
const baseHref = platformLocation.getBaseHrefFromDOM() || href;
|
||||
const baseUrl = new URL(baseHref, urlPrefix);
|
||||
const newUrl = new URL(request.url, baseUrl).toString();
|
||||
const parsedUrl = parseUrl(request.url, baseUrl, {
|
||||
allowProtocolRelative: true,
|
||||
});
|
||||
|
||||
return next(request.clone({url: newUrl}));
|
||||
return next(request.clone({url: parsedUrl.toString()}));
|
||||
}
|
||||
|
||||
export const SERVER_HTTP_PROVIDERS: Provider[] = [
|
||||
|
||||
@@ -6,8 +6,20 @@
|
||||
* found in the LICENSE file at https://angular.dev/license
|
||||
*/
|
||||
|
||||
const LEADING_SLASHES_REGEX = /^[/\\]+/;
|
||||
const MALFORMED_ABSOLUTE_URL_REGEX = /^[a-zA-Z][a-zA-Z0-9+.-]*:(\/\/|\\\\)/;
|
||||
/**
|
||||
* Matches http: or https:
|
||||
*/
|
||||
const HTTP_OR_HTTPS_PROTOCOL_REGEX = /^https?:/i;
|
||||
|
||||
/**
|
||||
* Options for {@link parseUrl}.
|
||||
*/
|
||||
export interface ParseUrlOptions {
|
||||
/**
|
||||
* Allow protocol-relative URLs (e.g. `//example.com`).
|
||||
*/
|
||||
allowProtocolRelative?: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
* Parses a URL string and returns a resolved WHATWG URL object.
|
||||
@@ -16,32 +28,89 @@ const MALFORMED_ABSOLUTE_URL_REGEX = /^[a-zA-Z][a-zA-Z0-9+.-]*:(\/\/|\\\\)/;
|
||||
* If an origin is provided, relative URLs and protocol-relative URLs are normalized and resolved against it.
|
||||
*/
|
||||
export function parseUrl(urlStr: string | undefined): URL | null;
|
||||
export function parseUrl(urlStr: string | undefined, origin: string): URL;
|
||||
export function parseUrl(urlStr: string | undefined, origin?: string): URL | null {
|
||||
export function parseUrl(
|
||||
urlStr: string | undefined,
|
||||
origin: string | URL,
|
||||
options?: ParseUrlOptions,
|
||||
): URL;
|
||||
export function parseUrl(
|
||||
urlStr: string | undefined,
|
||||
origin?: string | URL,
|
||||
options: ParseUrlOptions = {},
|
||||
): URL | null {
|
||||
const originUrl = typeof origin === 'string' ? new URL('/', origin) : origin;
|
||||
|
||||
if (!urlStr) {
|
||||
return origin !== undefined ? new URL('/', origin) : null;
|
||||
return originUrl || null;
|
||||
}
|
||||
|
||||
if (URL.canParse(urlStr)) {
|
||||
return new URL(urlStr);
|
||||
urlStr = urlStr.trim();
|
||||
|
||||
// Fast-path: if the URL is a valid, standard absolute URL, parse and return it immediately.
|
||||
let resolved: URL | undefined;
|
||||
try {
|
||||
resolved = new URL(urlStr);
|
||||
} catch {}
|
||||
|
||||
if (resolved) {
|
||||
if (originUrl && !isSafeOriginChange(resolved, originUrl, urlStr)) {
|
||||
throwSuspiciousUrlError(urlStr);
|
||||
}
|
||||
|
||||
return resolved;
|
||||
}
|
||||
|
||||
if (MALFORMED_ABSOLUTE_URL_REGEX.test(urlStr)) {
|
||||
// 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
|
||||
// absolute URL. Since it is malformed, the native URL constructor will throw a validation
|
||||
// error. Standard relative/protocol-relative paths parse successfully, allowing the flow to continue.
|
||||
if (!URL.canParse(urlStr, 'http://fake')) {
|
||||
throw new Error(`Invalid URL: ${urlStr}`);
|
||||
}
|
||||
|
||||
if (origin === undefined) {
|
||||
if (!originUrl) {
|
||||
return null;
|
||||
}
|
||||
|
||||
// Normalizes request path parsing by collapsing multiple consecutive leading slashes
|
||||
// and backslashes (e.g. // or /\) down to a single forward slash. This ensures consistent
|
||||
// resolution of relative path segments and prevents unexpected absolute path overrides
|
||||
// during URL parsing.
|
||||
let normalizedPath = urlStr.replace(LEADING_SLASHES_REGEX, '/');
|
||||
if (normalizedPath[0] !== '/') {
|
||||
normalizedPath = `/${normalizedPath}`;
|
||||
const {allowProtocolRelative = false} = options;
|
||||
|
||||
// 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.
|
||||
if (urlStr.startsWith('//')) {
|
||||
if (!allowProtocolRelative) {
|
||||
throw new Error(`Protocol relative URLs are not allowed in this context. URL: ${urlStr}`);
|
||||
}
|
||||
|
||||
return new URL(urlStr, origin);
|
||||
}
|
||||
|
||||
return new URL(normalizedPath, origin);
|
||||
resolved = new URL(urlStr, origin);
|
||||
|
||||
if (!isSafeOriginChange(resolved, originUrl, urlStr)) {
|
||||
throwSuspiciousUrlError(urlStr);
|
||||
}
|
||||
|
||||
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);
|
||||
}
|
||||
|
||||
@@ -1524,6 +1524,68 @@ class HiddenModule {}
|
||||
mock.expectOne('http://localhost/testing').flush('success!');
|
||||
});
|
||||
});
|
||||
|
||||
it('prevents SSRF bypasses via backslash URLs in HttpClient by throwing a suspicious origin error', async () => {
|
||||
ref.injector.get(NgZone).run(() => {
|
||||
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.`,
|
||||
);
|
||||
},
|
||||
});
|
||||
|
||||
mock.verify();
|
||||
});
|
||||
});
|
||||
|
||||
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',
|
||||
'\r\n/\\attacker.com',
|
||||
];
|
||||
|
||||
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();
|
||||
});
|
||||
});
|
||||
|
||||
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();
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -32,6 +32,7 @@ import {INITIAL_CONFIG, platformServer} from '@angular/platform-server';
|
||||
expect(location.pathname).toBe('/');
|
||||
platform.destroy();
|
||||
});
|
||||
|
||||
it('is configurable via INITIAL_CONFIG', async () => {
|
||||
const platform = platformServer([
|
||||
{
|
||||
@@ -135,94 +136,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: '<html><head></head><body></body></html>',
|
||||
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: '<html><head></head><body></body></html>',
|
||||
url,
|
||||
},
|
||||
it('should throw on protocol-relative URLs in INITIAL_CONFIG', async () => {
|
||||
const platform = platformServer([
|
||||
{
|
||||
provide: INITIAL_CONFIG,
|
||||
useValue: {
|
||||
document: '<html><head></head><body></body></html>',
|
||||
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();
|
||||
});
|
||||
});
|
||||
})();
|
||||
|
||||
@@ -17,6 +17,15 @@ describe('parseUrl', () => {
|
||||
expect(url.hash).toBe('#hash');
|
||||
});
|
||||
|
||||
it('should throw on backslash-prefixed hijack attempts', () => {
|
||||
const urls = ['/\\attacker.com/deep/path', '\\\\attacker.com/deep/path'];
|
||||
for (const url of urls) {
|
||||
expect(() => parseUrl(url, 'http://test.com')).toThrowError(
|
||||
`URL ${url} changed origin unexpectedly. This is suspicious and may indicate a security bypass attempt.`,
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
it('should resolve absolute URLs ignoring origin', () => {
|
||||
const url = parseUrl('http://other.com/deep/path', 'http://test.com');
|
||||
expect(url.href).toBe('http://other.com/deep/path');
|
||||
@@ -38,6 +47,13 @@ describe('parseUrl', () => {
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
it('should throw on obfuscated protocols attempting to change origin', () => {
|
||||
const url = 'ht\ntp://evil.com/path';
|
||||
expect(() => parseUrl(url, 'http://test.com')).toThrowError(
|
||||
`URL ${url} changed origin unexpectedly. This is suspicious and may indicate a security bypass attempt.`,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('without origin', () => {
|
||||
|
||||
Reference in New Issue
Block a user