Skip to content

Commit

Permalink
fix(sendRedirect): always encode location uri
Browse files Browse the repository at this point in the history
  • Loading branch information
pi0 committed Aug 30, 2022
1 parent 2fa27e6 commit 01476ac
Showing 1 changed file with 5 additions and 4 deletions.
9 changes: 5 additions & 4 deletions src/utils/response.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,14 @@ export function defaultContentType (event: CompatibilityEvent, type?: string) {
}

export function sendRedirect (event: CompatibilityEvent, location: string, code = 302) {
const encodedLoc = encodeURI(location)
event.res.statusCode = code
event.res.setHeader('Location', location)
// minimal html document that redirects on client side
event.res.setHeader('Location', encodedLoc)
// Minimal html document that redirects on client side
const html = `<!DOCTYPE html>
<html>
<head><meta http-equiv="refresh" content="0; url=${encodeURI(location)}"></head>
<body>Redirecting to <a href=${JSON.stringify(location)}>${encodeURI(location)}</a></body>
<head><meta http-equiv="refresh" content="0; url=${encodedLoc}"></head>
<body>Redirecting to <a href=${JSON.stringify(encodedLoc)}>${encodedLoc}</a></body>
</html>`
return send(event, html, MIMES.html)
}
Expand Down

5 comments on commit 01476ac

@MurmeltierS
Copy link

Choose a reason for hiding this comment

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

This just destoryed our OAuth Flow. Why was this even introduced, I can't find a corresponding issue? This seems like a non-fix.

@pi0
Copy link
Member Author

@pi0 pi0 commented on 01476ac Sep 1, 2022

Choose a reason for hiding this comment

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

@MurmeltierS It was from a security report not published yet. Sorry for the inconvenience. Can you please explain why this broke your flow with encoding? Would be happy to make a hotfix asap.

@MurmeltierS
Copy link

@MurmeltierS MurmeltierS commented on 01476ac Sep 1, 2022

Choose a reason for hiding this comment

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

@MurmeltierS when using this to forward to an OAuth URL Query Parameters get double-URI-encoded. This will most definitely break things on the other end.

e.g.: https://foobar.myshopify.com/admin/oauth/authorize?client_id=6a63bcef27a43f48e07c239bc9741cd8&scope=write_products%252Cwrite_files&redirect_uri=https%253A%252F%252Fpictofit-shopify-app.vercel.app%252Fauth%252Fcallback-login&state=848902450404611&grant_options%255B%255D=per-user instead of the correct url https://foobar.myshopify.com/admin/oauth/authorize?client_id=6a63bcef27a43f48e07c239bc9741cd8&scope=write_products%2Cwrite_files&redirect_uri=https%3A%2F%2Fpictofit-shopify-app.vercel.app%2Fauth%2Fcallback-login&state=848902450404611&grant_options%5B%5D=per-user

@pi0
Copy link
Member Author

@pi0 pi0 commented on 01476ac Sep 1, 2022

Choose a reason for hiding this comment

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

Fix on the way!

@pi0
Copy link
Member Author

@pi0 pi0 commented on 01476ac Sep 1, 2022

Choose a reason for hiding this comment

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

Should be fixed in latest. Please try updating lockfile. (04b432c)

Please sign in to comment.