Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions packages/angular/ssr/src/utils/url.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,16 @@ export function addTrailingSlash(url: string): string {
* ```js
* joinUrlParts('path/', '/to/resource'); // '/path/to/resource'
* joinUrlParts('/path/', 'to/resource'); // '/path/to/resource'
* joinUrlParts('/base/', '/login/?next=/a/'); // '/base/login?next=/a/'
* joinUrlParts('', ''); // '/'
* ```
*/
export function joinUrlParts(...parts: string[]): string {
const normalizedParts: string[] = [];
let suffix = '';

for (const part of parts) {
for (let i = 0, len = parts.length; i < len; i++) {
const part = parts[i];
if (part === '') {
// Skip any empty parts
continue;
Expand All @@ -108,6 +111,22 @@ export function joinUrlParts(...parts: string[]): string {
let start = 0;
let end = part.length;

if (i === len - 1) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: If parts ends with empty string arguments (e.g., joinUrlParts('/login/?next=/a/', '')), i === len - 1 will match on the trailing empty string rather than the last non-empty URL segment. As a result, suffix extraction is skipped for the actual last segment and any trailing slashes in its query parameters or hash fragments will be stripped.

Consider trimming trailing empty strings from parts or identifying the last non-empty segment index before the loop.

const queryIndex = part.indexOf('?');
const hashIndex = part.indexOf('#');
const suffixIndex =
queryIndex === -1
? hashIndex
: hashIndex === -1
? queryIndex
: Math.min(queryIndex, hashIndex);

if (suffixIndex !== -1) {
suffix = part.slice(suffixIndex);
end = suffixIndex;
}
}

// Use "Pointers" to avoid intermediate slices
while (start < end && part[start] === '/') {
start++;
Expand All @@ -122,7 +141,7 @@ export function joinUrlParts(...parts: string[]): string {
}
}

return addLeadingSlash(normalizedParts.join('/'));
return addLeadingSlash(normalizedParts.join('/')) + suffix;
}

/**
Expand Down
22 changes: 22 additions & 0 deletions packages/angular/ssr/test/app_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,14 @@ describe('AngularServerApp', () => {
path: 'redirect-query-placeholder',
redirectTo: '/home?q=:term&id=:missing&static=value',
},
{
path: 'redirect-query-trailing-slash',
redirectTo: '/home?next=/a/',
},
{
path: 'redirect-fragment-trailing-slash',
redirectTo: '/home#section/',
},
{
path: 'redirect-to-function',
redirectTo: () => 'home',
Expand Down Expand Up @@ -282,6 +290,20 @@ describe('AngularServerApp', () => {
expect(response?.status).toBe(302);
});

it('should preserve trailing slashes in query parameters and fragments of redirect targets', async () => {
const queryResponse = await app.handle(
new Request('http://localhost/redirect-query-trailing-slash'),
);
expect(queryResponse?.headers.get('location')).toBe('/home?next=/a/');
expect(queryResponse?.status).toBe(302);

const fragmentResponse = await app.handle(
new Request('http://localhost/redirect-fragment-trailing-slash'),
);
expect(fragmentResponse?.headers.get('location')).toBe('/home#section/');
expect(fragmentResponse?.status).toBe(302);
});

it('should handle request abortion gracefully', async () => {
const controller = new AbortController();
const request = new Request('http://localhost/home', { signal: controller.signal });
Expand Down
25 changes: 25 additions & 0 deletions packages/angular/ssr/test/routes/ng-routes_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -812,6 +812,31 @@ describe('extractRoutesAndCreateRouteTree', () => {
]);
});

it('should preserve trailing slashes in query parameters and fragments of absolute redirect targets', async () => {
setAngularAppTestingManifest(
[
{ path: 'query-slash', redirectTo: '/login?next=/a/' },
{ path: 'fragment-slash', redirectTo: '/login#section/' },
{ path: 'path-and-query-slash', redirectTo: '/login/?next=/a/' },
{ path: 'login', component: DummyComponent },
],
[{ path: '**', renderMode: RenderMode.Server }],
);

const { routeTree, errors } = await extractRoutesAndCreateRouteTree({ url });
expect(errors).toHaveSize(0);
expect(routeTree.toObject()).toEqual([
{ route: '/query-slash', renderMode: RenderMode.Server, redirectTo: '/login?next=/a/' },
{ route: '/fragment-slash', renderMode: RenderMode.Server, redirectTo: '/login#section/' },
{
route: '/path-and-query-slash',
renderMode: RenderMode.Server,
redirectTo: '/login?next=/a/',
},
{ route: '/login', renderMode: RenderMode.Server },
]);
});

it('should not resolve parameterized routes for SSG when `invokeGetPrerenderParams` is false', async () => {
setAngularAppTestingManifest(
[
Expand Down
12 changes: 12 additions & 0 deletions packages/angular/ssr/test/utils/url_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,18 @@ describe('URL Utils', () => {
it('should handle parts containing only slashes', () => {
expect(joinUrlParts('//', '///')).toBe('/');
});

it('should preserve trailing slashes in query parameters and fragments', () => {
expect(joinUrlParts('/', '/login?next=/a/')).toBe('/login?next=/a/');
expect(joinUrlParts('/', '/login#section/')).toBe('/login#section/');
expect(joinUrlParts('/base/', '/foo/?next=/a/#section/')).toBe('/base/foo?next=/a/#section/');
expect(joinUrlParts('/base/', '/login/#section?next=/a/')).toBe(
'/base/login#section?next=/a/',
);
expect(joinUrlParts('/base/', '/?next=/a/')).toBe('/base?next=/a/');
expect(joinUrlParts('/', '/?next=/a/')).toBe('/?next=/a/');
expect(joinUrlParts('', '///evil.com?a=/')).toBe('/evil.com?a=/');
});
});

describe('stripIndexHtmlFromURL', () => {
Expand Down
Loading