Conversation
When STS session credentials expire during long multipart uploads, S3 returns HTTP 400 with ExpiredToken. Previously this was classified as a non-recoverable BadRequestError, killing the upload permanently. Now it's classified as a recoverable NetworkError, allowing the retry loop to request a fresh pre-signed URL from the backend.
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the resilience of large file uploads by specifically addressing 'ExpiredToken' errors returned by S3. Previously, these errors would cause uploads to fail permanently, but now they are correctly identified and treated as temporary network issues, enabling the system to refresh credentials and successfully complete the upload process. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly improves file upload resilience by treating S3 ExpiredToken errors as recoverable. However, a critical security concern has been identified: the inclusion of full pre-signed URLs in error messages can leak sensitive authentication tokens into application logs. It is recommended to sanitize these URLs in error message construction. Additionally, while the approach of checking the response body for the specific error code is sound, there is a suggestion to make this check more robust.
| const message = | ||
| `response is not ok, status code: ${statusCode},` + | ||
| ` body: ${body}, headers: ${JSON.stringify(headers)}, url: ${info.uploadUrl}`; |
There was a problem hiding this comment.
The error message includes the full info.uploadUrl, which is a pre-signed URL containing sensitive authentication information in its query parameters (e.g., X-Amz-Signature, X-Amz-Security-Token). Logging these secrets can lead to unauthorized access if logs are compromised. Consider sanitizing the URL by removing the query string before including it in the error message.
| const message = | |
| `response is not ok, status code: ${statusCode},` + | |
| ` body: ${body}, headers: ${JSON.stringify(headers)}, url: ${info.uploadUrl}`; | |
| const message = | |
| `response is not ok, status code: ${statusCode},` + | |
| ` body: ${body}, headers: ${JSON.stringify(headers)}, url: ${info.uploadUrl.split('?')[0]}`; |
| } | ||
|
|
||
| function isExpiredTokenError(body: string): boolean { | ||
| return body.includes("<Code>ExpiredToken</Code>"); |
There was a problem hiding this comment.
Using String.prototype.includes() for this check is a bit fragile because it performs a simple substring search. A regular expression is more robust as it can match the specific XML tag structure, making the check less prone to accidental matches and more resilient to potential (though unlikely) whitespace variations in the S3 error response.
| return body.includes("<Code>ExpiredToken</Code>"); | |
| return /<Code>ExpiredToken<\/Code>/.test(body); |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1476 +/- ##
==========================================
- Coverage 54.47% 54.36% -0.11%
==========================================
Files 242 242
Lines 13481 13475 -6
Branches 2759 2760 +1
==========================================
- Hits 7344 7326 -18
- Misses 5219 5228 +9
- Partials 918 921 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
2 participants
Summary
ExpiredTokenerrors as recoverable during file uploads. When STS session credentials expire during long multipart uploads (800+ chunks), S3 returns HTTP 400 withExpiredToken. Previously classified as non-recoverableBadRequestError, permanently killing the upload. Now classified as recoverableNetworkError, allowing the retry loop to request a fresh pre-signed URL.Context
Large file uploads that exceed the STS token lifetime (1-12h depending on AWS config) would fail terminally at an arbitrary chunk with no way to recover. The fix parses the S3 error XML body to distinguish
ExpiredTokenfrom other 400 errors.Test plan
ExpiredToken, the next attempt gets a fresh pre-signed URL from the backend