Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fix storefrontRedirect on soft navigations
Browse files Browse the repository at this point in the history
Also change redirect status code from 301 to 302
blittle committed Mar 21, 2024

Verified

This commit was signed with the committer’s verified signature.
blittle Bret Little
1 parent fbef3cd commit 6733dad
Showing 3 changed files with 59 additions and 16 deletions.
5 changes: 5 additions & 0 deletions .changeset/hip-rocks-hammer.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/hydrogen': patch
---

Fix bug where `storefrontRedirect` would return an error on soft page navigations. Also change the redirect status code from 301 to 302.
29 changes: 28 additions & 1 deletion packages/hydrogen/src/routing/redirect.test.ts
Original file line number Diff line number Diff line change
@@ -36,7 +36,7 @@ describe('storefrontRedirect', () => {
}),
).resolves.toEqual(
new Response(null, {
status: 301,
status: 302,
headers: {location: shopifyDomain + '/some-page'},
}),
);
@@ -46,6 +46,33 @@ describe('storefrontRedirect', () => {
});
});

it('works with soft navigations', async () => {
queryMock.mockResolvedValueOnce({
urlRedirects: {edges: [{node: {target: shopifyDomain + '/some-page'}}]},
});

await expect(
storefrontRedirect({
storefront: storefrontMock,
request: new Request(
'https://domain.com/some-page?_data=%2Fcollections%2Fbackcountry',
),
}),
).resolves.toEqual(
new Response(null, {
status: 200,
headers: {
'X-Remix-Redirect': shopifyDomain + '/some-page',
'X-Remix-Status': '302',
},
}),
);

expect(queryMock).toHaveBeenCalledWith(expect.anything(), {
variables: {query: 'path:/some-page'},
});
});

it('queries the SFAPI and returns 404 on mismatch', async () => {
queryMock.mockResolvedValueOnce({
urlRedirects: {edges: []},
41 changes: 26 additions & 15 deletions packages/hydrogen/src/routing/redirect.ts
Original file line number Diff line number Diff line change
@@ -33,10 +33,21 @@ export async function storefrontRedirect(
} = options;

const {pathname, search} = new URL(request.url);
const redirectFrom = pathname + search;
const searchParams = new URLSearchParams(search);

const isSoftNavigation = searchParams.has('_data');

searchParams.delete('_data');
const filteredSearchParams = searchParams.toString();

const redirectFrom =
pathname + (filteredSearchParams ? '?' + isSoftNavigation : '');

if (pathname === '/admin' && !noAdminRedirect) {
return redirect(`${storefront.getShopifyDomain()}/admin`);
return createRedirectResponse(
`${storefront.getShopifyDomain()}/admin`,
isSoftNavigation,
);
}

try {
@@ -49,13 +60,13 @@ export async function storefrontRedirect(
const location = urlRedirects?.edges?.[0]?.node?.target;

if (location) {
return new Response(null, {status: 301, headers: {location}});
return createRedirectResponse(location, isSoftNavigation);
}

const redirectTo = getRedirectUrl(request.url);

if (redirectTo) {
return redirect(redirectTo);
return createRedirectResponse(redirectTo, isSoftNavigation);
}
} catch (error) {
console.error(
@@ -67,17 +78,17 @@ export async function storefrontRedirect(
return response;
}

function isLocalPath(requestUrl: string, redirectUrl: string) {
// We don't want to redirect cross domain,
// doing so could create phishing vulnerability
// Test for protocols, e.g. https://, http://, //
// and uris: mailto:, tel:, javascript:, etc.
try {
return (
new URL(requestUrl).origin === new URL(redirectUrl, requestUrl).origin
);
} catch (e) {
return false;
function createRedirectResponse(location: string, isSoftNavigation: boolean) {
if (isSoftNavigation) {
return new Response(null, {
status: 200,
headers: {'X-Remix-Redirect': location, 'X-Remix-Status': '302'},
});
} else {
return new Response(null, {
status: 302,
headers: {location: location},
});
}
}

0 comments on commit 6733dad

Please sign in to comment.