Skip to content

fix: improve file upload resilience #1476

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.

Sign up for GitHub

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

Open
dbolotin wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: improve file upload resilience #1476

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions lib/node/pl-drivers/src/clients/upload.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,24 +316,33 @@ async function checkExpectedMTime(path: string, expectedMTimeUnix: bigint) {
}
}

function isExpiredTokenError(body: string): boolean {
return body.includes("<Code>ExpiredToken</Code>");
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 2, 2026

Choose a reason for hiding this comment

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

medium

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.

Suggested change
return body.includes("<Code>ExpiredToken</Code>");
return /<Code>ExpiredToken<\/Code>/.test(body);

}

function checkStatusCodeOk(
statusCode: number,
body: string,
headers: IncomingHttpHeaders,
info: UploadAPI_GetPartURL_Response,
) {
const message =
`response is not ok, status code: ${statusCode},` +
` body: ${body}, headers: ${JSON.stringify(headers)}, url: ${info.uploadUrl}`;
Comment on lines +329 to +331
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 2, 2026

Choose a reason for hiding this comment

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

security-medium medium

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.

Suggested change
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]}`;


if (statusCode == 400) {
throw new BadRequestError(
`response is not ok, status code: ${statusCode},` +
` body: ${body}, headers: ${JSON.stringify(headers)}, url: ${info.uploadUrl}`,
);
// S3 may return 400 with ExpiredToken when the STS session credentials
// used to sign the pre-signed URL have expired. This is recoverable:
// a retry will request a fresh pre-signed URL from the backend.
if (isExpiredTokenError(body)) {
throw new NetworkError(message);
}

throw new BadRequestError(message);
}

if (statusCode != 200) {
throw new NetworkError(
`response is not ok, status code: ${statusCode},` +
` body: ${body}, headers: ${JSON.stringify(headers)}, url: ${info.uploadUrl}`,
);
throw new NetworkError(message);
}
}

Expand Down