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

fix(auth): Handle when authorization header is lowercased #10442

Merged
merged 2 commits into from
Apr 11, 2024

Conversation

dac09
Copy link
Collaborator

@dac09 dac09 commented Apr 11, 2024

In one of the PRs I did for last release, I switched getting the header using the new getEventHeaders function.

This function will check for two cases:

getEventHeaders('Authorization') 
=> 
a) header['authorization']
b) header['Authorization']

BUT if you passed it a lowercase header in the first place:

getEventHeaders('authorization') 
=> 
a) header['authorization']
b) header['authorization']

I actually didn't change the logic it's the same as before, but inparseAuthorizationHeader, we used to call it with the capital case.

I know the full solution is to grab the headers, and convert them all to lower-case, but I'm intentionally avoiding this because I don't want to slow down handling of every request by looping over all the headers.


This PR makes a minor change, and adds some extra tests. 🤞 we'll move everything to Fetch API soon and won't have to deal with this sillyness!

@dac09 dac09 added the release:fix This PR is a fix label Apr 11, 2024
@dac09 dac09 added this to the next-release-patch milestone Apr 11, 2024
@dac09 dac09 requested a review from dthyresson April 11, 2024 10:20
@@ -33,7 +33,7 @@ export interface AuthorizationHeader {
export const parseAuthorizationHeader = (
event: APIGatewayProxyEvent | Request,
): AuthorizationHeader => {
const parts = getEventHeader(event, 'authorization')?.split(' ')
const parts = getEventHeader(event, 'Authorization')?.split(' ')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the real change. Crazy in 2024 we have to worry about this 🤣🤣🤣🤣

Copy link
Contributor

@dthyresson dthyresson left a comment

Choose a reason for hiding this comment

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

While I see how the fix fixes, I was curious why one didn't also change so that in packages/api/src/event.ts:

// Extracts the header from an event, handling lower and upper case header names.
export const getEventHeader = (
  event: APIGatewayProxyEvent | Request,
  headerName: string,
) => {
  if (isFetchApiRequest(event)) {
    return event.headers.get(headerName)
  }

  return event.headers[headerName] || event.headers[headerName.toLowerCase()]
}

the event.headers.get didn't ask for both cases as well?

@dac09
Copy link
Collaborator Author

dac09 commented Apr 11, 2024

the event.headers.get didn't ask for both cases as well?

The headers class handles casing for us!

@dthyresson dthyresson self-requested a review April 11, 2024 11:45
Copy link
Contributor

@dthyresson dthyresson left a comment

Choose a reason for hiding this comment

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

Approved as in the fetch request the get headers handles casing.

LGTM and :shipit:

@dthyresson dthyresson merged commit 0fc28b6 into redwoodjs:main Apr 11, 2024
46 checks passed
dac09 added a commit that referenced this pull request Apr 11, 2024
…th-mw-auth

* 'main' of github.com:redwoodjs/redwood: (21 commits)
  fix(auth): Handle when authorization header is lowercased (#10442)
  Update rbac.md - code match (#10405)
  chore: make crwa e2e test work across branches (#10437)
  feat: [Auth] Common AuthProvider & use* changes for middleware auth (#10420)
  fix(cli): only show webpack options for dev if `bundler = "webpack"` (#10359)
  fix(vercel): specify build env vars as a string (#10436)
  fix(vercel): write `vercel.json` as a part of setup (#10355)
  fix(middleware): Handle POST requests in middleware router too (#10418)
  chore(ci): get ci running on next (#10432)
  RSC: Explain noExternal vite config option (#10429)
  chore(web): Fix .d.ts overwrite build issue (#10431)
  chore(web): .js imports to prep for ESM (#10430)
  chore(refactor): Split rwjs/forms up into several smaller logical units (#10428)
  chore(rsc): simplify `noExternals` config (#10220)
  chore(deps): Update vite to 5.2.8 (#10427)
  chore(auth): Convert `@redwoodjs/auth` to ESM+CJS dual build (#10417)
  chore(framework-tools): Warn about missing metafile (#10426)
  chore(test): Switch rwjs/auth over to vitest (#10423)
  chore(whatwg-fetch): Switch to importing instead of requiring (#10424)
  chore(deps): bump undici from 5.28.3 to 5.28.4 in /.github/actions/check_changesets (#10421)
  ...
Josh-Walker-GM pushed a commit that referenced this pull request Apr 11, 2024
In one of the PRs I did for last release, I switched getting the header
using the new `getEventHeaders` function.

This function will check for two cases:
```
getEventHeaders('Authorization') 
=> 
a) header['authorization']
b) header['Authorization']
```

**BUT** if you passed it a lowercase header in the first place:

```
getEventHeaders('authorization') 
=> 
a) header['authorization']
b) header['authorization']
```

I actually didn't change the logic it's the same as before, but
in`parseAuthorizationHeader`, we used to call it with the capital case.

I know the _full_ solution is to grab the headers, and convert them all
to lower-case, but I'm intentionally avoiding this because I don't want
to slow down handling of every request by looping over all the headers.

---


This PR makes a minor change, and adds some extra tests. 🤞 we'll move
everything to Fetch API soon and won't have to deal with this sillyness!
Josh-Walker-GM pushed a commit that referenced this pull request Apr 11, 2024
In one of the PRs I did for last release, I switched getting the header
using the new `getEventHeaders` function.

This function will check for two cases:
```
getEventHeaders('Authorization') 
=> 
a) header['authorization']
b) header['Authorization']
```

**BUT** if you passed it a lowercase header in the first place:

```
getEventHeaders('authorization') 
=> 
a) header['authorization']
b) header['authorization']
```

I actually didn't change the logic it's the same as before, but
in`parseAuthorizationHeader`, we used to call it with the capital case.

I know the _full_ solution is to grab the headers, and convert them all
to lower-case, but I'm intentionally avoiding this because I don't want
to slow down handling of every request by looping over all the headers.

---


This PR makes a minor change, and adds some extra tests. 🤞 we'll move
everything to Fetch API soon and won't have to deal with this sillyness!
dac09 added a commit to dac09/redwood that referenced this pull request Apr 12, 2024
… into feat/og-gen-mw-vite-plugin

* 'feat/og-gen-mw-vite-plugin' of github.com:dac09/redwood:
  chore(deps): bump browserify-sign from 4.2.1 to 4.2.3 (redwoodjs#10446)
  chore(deps): bump tar from 6.1.11 to 6.2.1 in /docs (redwoodjs#10438)
  chore(deps): update dependency firebase to v10.11.0 (redwoodjs#10366)
  fix(auth): Handle when authorization header is lowercased (redwoodjs#10442)
dac09 added a commit that referenced this pull request Apr 12, 2024
…g-gen-mw-p2

* 'main' of github.com:redwoodjs/redwood:
  feat(og-gen): Adds package and vite plugin for dynamic og generation (#10439)
  chore(deps): bump browserify-sign from 4.2.1 to 4.2.3 (#10446)
  chore(deps): bump tar from 6.1.11 to 6.2.1 in /docs (#10438)
  chore(deps): update dependency firebase to v10.11.0 (#10366)
  fix(auth): Handle when authorization header is lowercased (#10442)
  Update rbac.md - code match (#10405)
  chore: make crwa e2e test work across branches (#10437)
  feat: [Auth] Common AuthProvider & use* changes for middleware auth (#10420)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants