fix(rt): reject CR and LF in ECS authorization tokens - #1964
Conversation
|
Thanks for the pull request, @JerimothLau. I'm seeking confirmation from other AWS SDKs but at first glance this seems like a good change that would improve parsing robustness. If I can't find any objections/problems then it'll likely be accepted and merged. 👍 Out of curiosity, is the absence of the single-character validation causing problems for you? |
|
Hi @ianbotsf , thanks for taking a look and for the feedback, appreciate it. The absence of the single-character validation has caused some minor issues on our side, inputs that don’t strictly conform to the expected format are still being parsed, which lead to subtle downstream errors rather than failing fast. Adding this validation helps ensure that malformed values are caught early and handled more predictably. Actually the impact isn’t widespread, but tightening the parsing behavior would definitely improve robustness and make error handling more consistent. |
|
Hi @ianbotsf , just noting that the failing Kat Transform check appears to be caused by AWS credentials being unavailable to fork PR workflows rather than by the changes in this PR. Please let me know if any action is needed from my side, thanks. |
Yes, most of our GitHub PR checks cannot run on forks at present. Internally we're discussing some ways to improve this but the ideas all involve a lot of work and won't land in time for this PR. For now our best option is to copy this PR and resubmit it as a non-fork PR so the checks can run, which I've done in #1977. One of the checks that can run successfully in a fork PR is the Lint / ktlint check, which is failing for a valid reason in this PR. Let me know if you'd like to fix the lint issues or if you'd like us to handle it. All other checks are passing in #1977. |
|
Hi @ianbotsf , thanks for your reply and efforts on fixing the PR, I would best let you handle the remaining fix and the lint issues as well, thanks again. |
Issue
No linked issue.
Description of changes
Update
EcsCredentialsProviderto reject authorization tokens containing either a carriage return (\r) or line feed (\n).Previously, validation rejected only the combined CRLF sequence (
\r\n), allowing tokens containing a standalone CR or LF to pass provider-level validation before being used as an HTTPAuthorizationheader value.This change:
This provides earlier and consistent validation of invalid HTTP header values rather than relying on HTTP-engine-specific behavior.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.