diff --git a/packages/common/http/src/request.ts b/packages/common/http/src/request.ts index bce1fd1bf0ec..137b7939ce73 100644 --- a/packages/common/http/src/request.ts +++ b/packages/common/http/src/request.ts @@ -408,8 +408,20 @@ export class HttpRequest implements HttpRequestOptions { // No parameters, the visible URL is just the URL given at creation time. this.urlWithParams = url; } else { + // Strip the fragment (e.g. #bypass) from the URL before checking for existing query parameters. + // This ensures that new query parameters are inserted before the fragment, preventing + // situations where parameters are mistakenly appended after the fragment and thus + // only exposed to the frontend, which could bypass security checks. + let urlWithoutFragment = url; + let fragment = ''; + const hashIdx = url.indexOf('#'); + if (hashIdx !== -1) { + fragment = url.substring(hashIdx); + urlWithoutFragment = url.substring(0, hashIdx); + } + // Does the URL already have query parameters? Look for '?'. - const qIdx = url.indexOf('?'); + const qIdx = urlWithoutFragment.indexOf('?'); // There are 3 cases to handle: // 1) No existing parameters -> append '?' followed by params. // 2) '?' exists and is followed by existing query string -> @@ -417,8 +429,8 @@ export class HttpRequest implements HttpRequestOptions { // 3) '?' exists at the end of the url -> append params directly. // This basically amounts to determining the character, if any, with // which to join the URL and parameters. - const sep: string = qIdx === -1 ? '?' : qIdx < url.length - 1 ? '&' : ''; - this.urlWithParams = url + sep + params; + const sep: string = qIdx === -1 ? '?' : qIdx < urlWithoutFragment.length - 1 ? '&' : ''; + this.urlWithParams = urlWithoutFragment + sep + params + fragment; } } } diff --git a/packages/common/http/test/request_spec.ts b/packages/common/http/test/request_spec.ts index 35699f2a6ce3..191f205267f3 100644 --- a/packages/common/http/test/request_spec.ts +++ b/packages/common/http/test/request_spec.ts @@ -286,5 +286,22 @@ describe('HttpRequest', () => { const req = baseReq.clone({setParams: {'test': 'false'}}); expect(req.urlWithParams).toEqual('/test?test=false'); }); + it('appends parameters before a fragment', () => { + const fragmentParams = new HttpParams({fromString: 'auth_token=secret'}); + const req = baseReq.clone({params: fragmentParams, url: '/api/data/123#bypass'}); + expect(req.urlWithParams).toEqual('/api/data/123?auth_token=secret#bypass'); + }); + it('appends parameters before a URL fragment', () => { + const req = baseReq.clone({params, url: '/test#fragment'}); + expect(req.urlWithParams).toEqual('/test?test=true#fragment'); + }); + it('appends parameters before a URL fragment when URL has a query string', () => { + const req = baseReq.clone({params, url: '/test?other=false#fragment'}); + expect(req.urlWithParams).toEqual('/test?other=false&test=true#fragment'); + }); + it('appends parameters before a URL fragment when URL has multiple hash characters', () => { + const req = baseReq.clone({params, url: '/test#frag1#frag2'}); + expect(req.urlWithParams).toEqual('/test?test=true#frag1#frag2'); + }); }); });