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

Lowercase custom header #219

Merged
merged 3 commits into from Jun 9, 2016

Conversation

Projects
None yet
5 participants
@mmwtsn
Copy link
Contributor

mmwtsn commented Jun 3, 2016

Background

This ensures that if a custom server-side header value is set authentication still works. Currently on master requests fail if a custom header value is set to a string with capital letters. See the linked issue for additional background. If this patch is accepted I'll update the docs.

Fixes #218.

Test steps

  1. Add an example header key with "Example-Header" to the example app.
  2. Create a new token by making a POST request to /auth/local with the default user credentials.
  3. Use that token with to make a request to /messages.
    • Verify that you are able to authenticate with a capitalized header (Example-Header).
    • Verify that you are able to authenticate with a lowercase header (example-header).
@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Jun 3, 2016

I thought that the body-parser module automatically converted them to lowercase. I think we definitely want this if it's not happening already.

@joshuajabbour

This comment has been minimized.

Copy link

joshuajabbour commented Jun 3, 2016

The body-parser module doesn't touch the headers.

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Jun 3, 2016

Well, see what I know? :) Whichever module it is, I’ve not seen an uppercase header in node.

On Jun 3, 2016, at 4:24 PM, Joshua Jabbour notifications@github.com wrote:

The body-parser module doesn't touch the headers.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub #219 (comment), or mute the thread https://github.com/notifications/unsubscribe/AAH3WTL0tLnojWFlZfFSbRpxcAWZMP_Sks5qIKmtgaJpZM4It5dl.

@joshuajabbour

This comment has been minimized.

Copy link

joshuajabbour commented Jun 3, 2016

Yeah, node itself does the lowercasing iirc. So yes, Feathers should def also do the same.

@joshuajabbour

This comment has been minimized.

Copy link

joshuajabbour commented Jun 3, 2016

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Jun 4, 2016

@mmwtsn 👍 awesome! Would you be able to add a quick test in auth to make sure that headers are being lower cased?

@mmwtsn

This comment has been minimized.

Copy link
Contributor Author

mmwtsn commented Jun 8, 2016

@ekryski thanks! I pushed up a commit lowercasing the client-side header as discussed in #218 (comment) as well as a test for the capitalized header bug. Without .toLowerCase() the test fails.

I opened up a pull request to update the docs accordingly once this is merged.

@daffl daffl merged commit 40276d3 into feathersjs:master Jun 9, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@daffl

This comment has been minimized.

Copy link
Member

daffl commented Jun 9, 2016

Thanks again! This has just been released in v0.7.8

@mmwtsn mmwtsn deleted the mmwtsn:lowercase-header branch Jun 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.