-
Notifications
You must be signed in to change notification settings - Fork 122
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
Conversation
… AWS Lambda functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@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? |
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: 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. |
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. |
CI will be fixed as soon as #465 is merged. The linting rules were updated, so we had to fix the code. |
Codecov ReportPatch and project coverage have no change.
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
☔ View full report in Codecov by Sentry. |
@lejovaar7 please consider signing your commits in the future. I will overrule now and merge anyway, but we usually require this from contributors :) |
@all-contributors add @lejovaar7 for bug and code |
I've put up a pull request to add @lejovaar7! 🎉 |
@all-contributors add @chrisandrewcl for review! |
I've put up a pull request to add @chrisandrewcl! 🎉 |
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.