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: correct case usage for X-Telegram-Bot-Api-Secret-Token header in… #464

Merged
merged 2 commits into from
Jul 8, 2023

Conversation

lejovaar7
Copy link
Contributor

This update modifies the handling of the 'X-Telegram-Bot-Api-Secret-Token' header in both the synchronous and asynchronous AWS Lambda functions.

In the previous implementation, the SECRET_HEADER_LOWERCASE constant was being used to retrieve the header from the event. This caused complications since AWS Lambda does not interpret headers in a case-insensitive way, leading to potential mismatches if the header was not supplied in the exact expected case.

With this fix, the SECRET_HEADER constant is used, which matches the exact casing of the 'X-Telegram-Bot-Api-Secret-Token' header as it appears in the event. This adjustment should eliminate any issues caused by case sensitivity.

Copy link
Member

@KnorpelSenf KnorpelSenf left a comment

Choose a reason for hiding this comment

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

LGTM

@KnorpelSenf
Copy link
Member

@chrisandrewcl how come you used the lowercase version in #418? I didn't try this but it seems to me like the AWS docs imply that headers are not lowercased. Can you enlighten us?

@chrisandrewcl
Copy link
Contributor

chrisandrewcl commented Jul 8, 2023

Expanding on what I said here: while I have seen headers in the canonical form on lambdas before, I've not encountered telegram's secret header in canonical form while working with grammy for my project, just in its lowercase form. Given reports from other developers, it seems it doesn't mean it will always be lowercase, just that in this case it was lowercase.

Similar reports highlighting this behavior:
aws/aws-sam-cli#3083 (comment)
aws/aws-sam-cli#1860 (comment)

Given the inconsistencies, I was not confident opting for one or another but that was what ultimately was decided for the PR I submitted.

To avoid this issue, you could consider normalizing the headers, checking for both formats or adding a note to the docs.

I am happy either way since I ended up using the adapter just as a reference.

@KnorpelSenf
Copy link
Member

Telegram always delivers headers with proper casing: https://github.com/tdlib/telegram-bot-api/blob/251589221708a8280ffcad32fcdc5348d014a6ae/telegram-bot-api/WebhookActor.cpp#L561

Since it seems like we all agree that AWS doesn't lowercase the headers (side note: why does any software mess with header casing even???), it seems to be the reasonable thing to merge this.

If we find a repro where this breaks anything, we can restart the discussion on the matter.

@KnorpelSenf
Copy link
Member

CI will be fixed as soon as #465 is merged. The linting rules were updated, so we had to fix the code.

@codecov
Copy link

codecov bot commented Jul 8, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (361866b) 46.55% compared to head (f991228) 46.55%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #464   +/-   ##
=======================================
  Coverage   46.55%   46.55%           
=======================================
  Files          19       19           
  Lines        5595     5595           
  Branches      222      222           
=======================================
  Hits         2605     2605           
  Misses       2988     2988           
  Partials        2        2           
Impacted Files Coverage Δ
src/convenience/frameworks.ts 8.07% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@KnorpelSenf
Copy link
Member

@lejovaar7 please consider signing your commits in the future. I will overrule now and merge anyway, but we usually require this from contributors :)

@KnorpelSenf KnorpelSenf merged commit 51d3649 into grammyjs:main Jul 8, 2023
8 checks passed
@KnorpelSenf
Copy link
Member

@all-contributors add @lejovaar7 for bug and code

@allcontributors
Copy link
Contributor

@KnorpelSenf

I've put up a pull request to add @lejovaar7! 🎉

@KnorpelSenf
Copy link
Member

@all-contributors add @chrisandrewcl for review!

@allcontributors
Copy link
Contributor

@KnorpelSenf

I've put up a pull request to add @chrisandrewcl! 🎉

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

Successfully merging this pull request may close these issues.

None yet

3 participants