Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[sitecore-jss-nextjs]: Redirects with double and more parameters has been fixed for netlify #SXA-7554 #1935

Merged
merged 7 commits into from
Oct 8, 2024
Merged
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Our versioning strategy is as follows:
* `[templates/nextjs]` `[templates/react]` `[templates/angular]` `[templates/vue]` Fixed an issue when environment variable is undefined (not present in .env), that produced an "undefined" value in generated config file ([#1875](https://github.com/Sitecore/jss/pull/1875))
* `[templates/nextjs]` Fix embedded personalization not rendering correctly after navigation through router links. ([#1911](https://github.com/Sitecore/jss/pull/1911))
* `[template/angular]` Prevent client-side dictionary API call when SSR data is available ([#1930](https://github.com/Sitecore/jss/pull/1930)) ([#1932](https://github.com/Sitecore/jss/pull/1932))
* `[sitecore-jss-nextjs]` Resolved an issue with Netlify where URL query parameters were being sorted, causing redirect failures. Added a method to generate all possible permutations of query parameters, ensuring proper matching with URL patterns regardless of their order. ([#1935](https://github.com/Sitecore/jss/pull/1935))

### 🎉 New Features & Improvements

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1490,6 +1490,67 @@ describe('RedirectsMiddleware', () => {
expect(finalRes.status).to.equal(expected.status);
});

it('should return 301 redirect when queryString is ordered by alphabetic(Netlify feature)', async () => {
const setCookies = () => {};
const res = createResponse({
url: 'http://localhost:3000/found/',
status: 301,
setCookies,
});
const nextRedirectStub = sinon.stub(NextResponse, 'redirect').callsFake((url, init) => {
const status = typeof init === 'number' ? init : init?.status || 307;
return ({
url,
status,
cookies: { set: setCookies },
headers: res.headers,
} as unknown) as NextResponse;
});
const req = createRequest({
nextUrl: {
pathname: '/not-found/',
search: '?a=1&w=1',
href: 'http://localhost:3000/not-found/?a=1&w=1',
locale: 'en',
origin: 'http://localhost:3000',
clone() {
return Object.assign({}, req.nextUrl);
},
},
});

const { middleware, fetchRedirects, siteResolver } = createMiddleware({
pattern: '/not-found?w=1&a=1',
target: 'http://localhost:3000/found/',
redirectType: REDIRECT_TYPE_301,
isQueryStringPreserved: true,
locale: 'en',
});

const finalRes = await middleware.getHandler()(req);

validateDebugLog('redirects middleware start: %o', {
hostname: 'foo.net',
language: 'en',
pathname: '/not-found/',
});

validateEndMessageDebugLog('redirects middleware end in %dms: %o', {
headers: {},
redirected: undefined,
status: 301,
url: 'http://localhost:3000/found/',
});

expect(siteResolver.getByHost).to.be.calledWith(hostname);
// eslint-disable-next-line no-unused-expressions
expect(fetchRedirects.called).to.be.true;
expect(finalRes).to.deep.equal(res);
expect(finalRes.status).to.equal(res.status);

nextRedirectStub.restore();
});

describe('should redirect to normalized path when nextjs specific "path" query string parameter is provided', () => {
it('should return 301 redirect', async () => {
const setCookies = () => {};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ export class RedirectsMiddleware extends MiddlewareBase {
if (REGEXP_ABSOLUTE_URL.test(existsRedirect.target)) {
url.href = existsRedirect.target;
} else {
const source = `${url.pathname.replace(/\/*$/gi, '')}${url.search}`;
const source = `${url.pathname.replace(/\/*$/gi, '')}${existsRedirect.matchedQueryString}`;
const urlFirstPart = existsRedirect.target.split('/')[1];

if (this.locales.includes(urlFirstPart)) {
Expand Down Expand Up @@ -183,7 +183,7 @@ export class RedirectsMiddleware extends MiddlewareBase {
private async getExistsRedirect(
req: NextRequest,
siteName: string
): Promise<RedirectInfo | undefined> {
): Promise<(RedirectInfo & { matchedQueryString?: string }) | undefined> {
const redirects = await this.redirectsService.fetchRedirects(siteName);
const normalizedUrl = this.normalizeUrl(req.nextUrl.clone());
const tragetURL = normalizedUrl.pathname;
Expand All @@ -192,22 +192,26 @@ export class RedirectsMiddleware extends MiddlewareBase {
const modifyRedirects = structuredClone(redirects);

return modifyRedirects.length
? modifyRedirects.find((redirect: RedirectInfo) => {
? modifyRedirects.find((redirect: RedirectInfo & { matchedQueryString?: string }) => {
redirect.pattern = redirect.pattern.replace(RegExp(`^[^]?/${language}/`, 'gi'), '');
redirect.pattern = `/^\/${redirect.pattern
.replace(/^\/|\/$/g, '')
.replace(/^\^\/|\/\$$/g, '')
.replace(/^\^|\$$/g, '')
.replace(/(?<!\\)\?/g, '\\?')
.replace(/\$\/gi$/g, '')}[\/]?$/gi`;
.replace(/\$\/gi$/g, '')}[\/]?$/i`;
const matchedQueryString = this.isPermutedQueryMatch({
sc-ruslanmatkovskyi marked this conversation as resolved.
Show resolved Hide resolved
pathname: tragetURL,
queryString: targetQS,
pattern: redirect.pattern,
locale: req.nextUrl.locale,
});
redirect.matchedQueryString = matchedQueryString || '';

return (
(regexParser(redirect.pattern).test(tragetURL) ||
regexParser(redirect.pattern).test(`${tragetURL.replace(/\/*$/gi, '')}${targetQS}`) ||
regexParser(redirect.pattern).test(`/${req.nextUrl.locale}${tragetURL}`) ||
regexParser(redirect.pattern).test(
`/${req.nextUrl.locale}${tragetURL}${targetQS}`
)) &&
matchedQueryString) &&
(redirect.locale
? redirect.locale.toLowerCase() === req.nextUrl.locale.toLowerCase()
: true)
Expand Down Expand Up @@ -285,4 +289,54 @@ export class RedirectsMiddleware extends MiddlewareBase {
}
return redirect;
}

/**
* Generates all possible permutations of an array of key-value pairs.
* This is used to create every possible combination of URL query parameters.
* @param {Array<[string, string]>} array - The array of key-value pairs to permute.
* @returns {Array<Array<[string, string]>>} - A 2D array where each inner array is a unique permutation of the input.
*/
private getPermutations(array: [string, string][]): [string, string][][] {
sc-ruslanmatkovskyi marked this conversation as resolved.
Show resolved Hide resolved
if (array.length <= 1) return [array];

return array.flatMap((current, i) => {
const remaining = array.filter((_, idx) => idx !== i);
return this.getPermutations(remaining).map((permutation) => [current, ...permutation]);
});
}

/**
* Checks if the current URL query matches the provided pattern, considering all permutations of query parameters.
* It constructs all possible query parameter permutations and tests them against the pattern.
* @param {Object} params - The parameters for the URL match.
* @param {string} params.pathname - The current URL pathname.
* @param {string} params.queryString - The current URL query string.
* @param {string} params.pattern - The regex pattern to test the constructed URLs against.
* @param {string} [params.locale] - The locale prefix to include in the URL if present.
* @returns {string | undefined} - return query string if any of the query permutations match the provided pattern, undefined otherwise.
*/
private isPermutedQueryMatch({
pathname,
queryString,
pattern,
locale,
}: {
pathname: string;
queryString: string;
pattern: string;
locale?: string;
}): string | undefined {
const paramsArray = Array.from(new URLSearchParams(queryString).entries());
const listOfPermuted = this.getPermutations(paramsArray).map(
(permutation) => '?' + permutation.map(([key, value]) => `${key}=${value}`).join('&')
);

const normalizedPath = pathname.replace(/\/*$/gi, '');
return listOfPermuted.find((query) =>
[
regexParser(pattern).test(`${normalizedPath}${query}`),
regexParser(pattern).test(`/${locale}${normalizedPath}${query}`),
].some(Boolean)
);
}
}