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

Headers case-insensitivity #62

Closed
axel3rd opened this issue Feb 23, 2021 · 8 comments · Fixed by #66
Closed

Headers case-insensitivity #62

axel3rd opened this issue Feb 23, 2021 · 8 comments · Fixed by #66
Labels

Comments

@axel3rd
Copy link
Contributor

axel3rd commented Feb 23, 2021

id:
event.headers["X-GitHub-Delivery"] ||
event.headers["x-github-delivery"],
name: event.headers["X-GitHub-Event"] || event.headers["x-github-event"],

From RFC 7230 > 3.2. Header Fields:

Each header field consists of a case-insensitive field name followed by a colon (":"), optional leading whitespace, the field value, and optional trailing whitespace.

=> If you are testing (sample #61) your Lambda Probot with some tools which can "rewrite" headers (sample: Ansible URI module) like that (in Camel Case):

X-Github-Delivery: fake-ansible
X-Github-Event: test

Note the 'h' in lowercase

It doesn't work 😢. It would be nice if adapter-aws-lambda-serverless could be headers case-insensitive.

Error detail:

INFO	AggregateError: 
    Error: Event name not passed
        at Array.map (<anonymous>)
        at receiverHandle (/var/task/node_modules/@octokit/webhooks/dist-node/index.js:98:11)
        at verifyAndReceive (/var/task/node_modules/@octokit/webhooks/dist-node/index.js:307:29)
        at lambdaFunction (/var/task/node_modules/@probot/adapter-aws-lambda-serverless/lambda-function.js:12:24)
        at Runtime.handleOnce (/var/runtime/Runtime.js:66:25)
    at receiverHandle (/var/task/node_modules/@octokit/webhooks/dist-node/index.js:98:11)
    at verifyAndReceive (/var/task/node_modules/@octokit/webhooks/dist-node/index.js:307:29)
    at lambdaFunction (/var/task/node_modules/@probot/adapter-aws-lambda-serverless/lambda-function.js:12:24)
    at Runtime.handleOnce (/var/runtime/Runtime.js:66:25)

NB: False line number, console.log added for debug

@gr2m
Copy link
Contributor

gr2m commented Feb 23, 2021

It would be nice if adapter-aws-lambda-serverless could be headers case-insensitive

I know, I agree, but there is no efficient way to lowercase all headers. Because we know the source of the request, I thought it would be safe to access the headers with the casing we know is sent by GitHub, plus lowercase just to be sure.

If you are testing (sample #61) your Lambda Probot with some tools which can "rewrite" headers (sample: Ansible URI module) like that (in Camel Case):

The docs says

The module returns all the HTTP headers in lower-case

I don't get it. Why would X-GitHub-Delivery be changed to X-Github-Delivery?

@axel3rd
Copy link
Contributor Author

axel3rd commented Feb 23, 2021

The docs says: The module returns all the HTTP headers in lower-case

Yes, but it is not true 😆.

I don't get it. Why would X-GitHub-Delivery be changed to X-Github-Delivery?

I don't searched why in Ansible code, but all headers are written in Camel-case (even if not modified in debug log ... spent lot of time to catch it).
Camel-case seems a common usage for HTTP headers (Content-Type, ...) even if should be considered insensitively.

but there is no efficient way to lowercase all headers

Is it more complex than searching LowerCase value in an Array 🤔 ? I was started to work on it.

@gr2m
Copy link
Contributor

gr2m commented Feb 23, 2021

The docs says: The module returns all the HTTP headers in lower-case

Yes, but it is not true 😆.

maybe send a PR to their docs?

We could use https://github.com/sindresorhus/lowercase-keys, it's a simple function, but it copies the entire headers object.

Maybe we should just do it and safe us more headache in future

GitHub
Lowercase the keys of an object. Contribute to sindresorhus/lowercase-keys development by creating an account on GitHub.

@axel3rd
Copy link
Contributor Author

axel3rd commented Feb 23, 2021

maybe send a PR to their docs?

👍 (Need a simple test to proof it)

Maybe we should just do it and safe us more headache in future

I will have a look on headers it (values should not be modified).

@axel3rd
Copy link
Contributor Author

axel3rd commented Feb 23, 2021

The docs says: The module returns all the HTTP headers in lower-case

maybe send a PR to their docs?

To be precise, the doc speaks about reponse headers, enforced in lowercase (verified), but nothing about request headers. PR: #73702

@gr2m
Copy link
Contributor

gr2m commented Feb 23, 2021

okay let's use https://github.com/sindresorhus/lowercase-keys to be safe.

GitHub
Lowercase the keys of an object. Contribute to sindresorhus/lowercase-keys development by creating an account on GitHub.

@github-actions
Copy link

🎉 This issue has been resolved in version 2.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@axel3rd
Copy link
Contributor Author

axel3rd commented Feb 26, 2021

Tested in real life (Ansible smoke tests, probot deployment, ...) / LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants