Skip to content
This repository has been archived by the owner on Oct 21, 2020. It is now read-only.

Authorization header casing #88

Merged
merged 1 commit into from
May 2, 2018

Conversation

ojongerius
Copy link
Contributor

@ojongerius ojongerius commented Apr 30, 2018

I'd like to support "Authorization" which would comply with https://tools.ietf.org/html/rfc7235#section-4.2

@Bouncey
Copy link
Member

Bouncey commented Apr 30, 2018

In all my testing for #58 I only ever changed the token, the Authorization field never changed and I always got headers.authorization at the directive. Apart from placing the headers directly on to context, we do not manipulate them. Do the directives still work with these changes?

I screenshot of the headers I used during #58
screenshot_20180430-071436_firefox

@Bouncey
Copy link
Member

Bouncey commented Apr 30, 2018

Further to above, I have just logged out what we have in the headers in handler

// handler.js
// ...
log(Object.keys(headers));
// ...
  fcc:handler [ 'Host',
  fcc:handler   'User-Agent',
  fcc:handler   'Accept',
  fcc:handler   'Accept-Language',
  fcc:handler   'Accept-Encoding',
  fcc:handler   'Referer',
  fcc:handler   'content-type',
  fcc:handler   'authorization',
  fcc:handler   'origin',
  fcc:handler   'Content-Length',
  fcc:handler   'Connection',
  fcc:handler   'Content-Type' ]

auth/index.js Outdated
@@ -8,7 +8,7 @@ const { JWT_CERT } = process.env;

export function verifyWebToken(ctx) {
log('Verifying token');
const token = ctx && ctx.headers && ctx.headers.authorization;
const token = ctx && ctx.headers && ctx.headers.Authorization;

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@raisedadead
Copy link
Member

...
  fcc:handler   'content-type', <----------------
  fcc:handler   'authorization',
  fcc:handler   'origin',
  fcc:handler   'Content-Length',
  fcc:handler   'Connection',
  fcc:handler   'Content-Type' ] <---------------

There are duplicates for the same keys, this leads me to think that this is being set by someone somewhere, (could be programmatic, by some middle-ware or even us in our logic), which needs investigation.

@ojongerius
Copy link
Contributor Author

ojongerius commented Apr 30, 2018

Interesting 😄

Running against staging f3f971c:

Using "Authorization":

▶ curl -s -X POST -H "Content-Type: application/json" --data '{ "query": "{ users { _id name email } }" }' localhost:4000/graphql --header "Authorization: Bearer ${JWT}"  |jq
{
  "data": {
    "users": null
  },
  "errors": [
    {
      "message": "You must supply a JSON Web Token for authorization!",
      "locations": [
        {
          "line": 1,
          "column": 3
        }
      ],
      "path": [
        "users"
      ]
    }
  ]
}

Using "authorization":

▶ curl -s -X POST -H "Content-Type: application/json" --data '{ "query": "{ users { _id name email } }" }' localhost:4000/graphql --header "authorization: Bearer ${JWT}"  |jq
{
  "data": {
    "users": [
      {
        "_id": "5acd3a7e9f80cf00248abc97",
        "name": "",
        "email": "[email protected]"
      },
      {
        "_id": "5ad1c7ace769e064e3a3487b",
        "name": "Yet another Otto",
        "email": "[email protected]"
      },
      {
        "_id": "5ad1c7eee769e01038a3487c",
        "name": "Frida Kahlo",
        "email": "[email protected]"
      },
      {
        "_id": "5ad5392f2109c9843e5939d7",
        "name": "Karel Appel",
        "email": "[email protected]"
      },
      {
        "_id": "5ad5817bb8d47b9b06e2d0f2",
        "name": "Quincy Larson",
        "email": "[email protected]"
      },
      {
        "_id": "5ad6aa28948979c90662ef4a",
        "name": "Quincy Larson",
        "email": "[email protected]"
      },
      {
        "_id": "5ad6acae94897931e962ef4b",
        "name": "Quincy Larson",
        "email": "[email protected]"
      }
    ]
  }
}

@Bouncey what browser are you using?

So the HTTP/2 specs says that headers "header field names MUST be converted to lowercase prior to their encoding in HTTP/2".

And the RFC actually specifies that " Field name are case-insensitive." (https://tools.ietf.org/html/rfc2616#section-4.2).

@ojongerius
Copy link
Contributor Author

Can I summarise that we all agree that, if a client sets some headers, in this case "Authorization" with the first letter being an uppercase the behaviour is incorrect?

I'm surprised that Stuarts test works initially and am wondering if his client lowercased it for him. I suspect this to be the case, I found graphql/graphql-playground#370 which sound similar, but I stumbled upon this bug while trying to use Authorization instead of authorization in the playground served at /api

@raisedadead I think you were spot in your suspicions: there is an interesting interaction between the HAPI framework used by serverless-offline which at leased used to lowercase header field names.

To make it more interesting, the Hapi framework used by serverless-offline (ie: running it locally) will lowercase header field names, but when you deploy it to AWS headers will come in as is -I've not verified this but base it on comment dherault/serverless-offline#260 (comment).

For now I'd suggest that we use ctx && ctx.headers && (ctx.headers.Authorization || ctx.headers.authorization).

auth/index.js Outdated
const token =
ctx &&
ctx.headers &&
(ctx.headers.Authorization || ctx.headers.authorization);

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@ojongerius ojongerius changed the title fix: authorization header should start with uppercase Authorization header casing May 1, 2018
@Bouncey
Copy link
Member

Bouncey commented May 1, 2018

LGTM

@raisedadead are you happy with the work-around?

@ojongerius
Copy link
Contributor Author

@raisedadead good to go?

@raisedadead raisedadead merged commit 5ce261d into freeCodeCamp:staging May 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants