Skip to content

Avoid logging pre-signed direct-upload URL#19

Closed
zaniebot wants to merge 2 commits intomainfrom
codex/propose-fix-for-logged-s3-url-vulnerability
Closed

Avoid logging pre-signed direct-upload URL#19
zaniebot wants to merge 2 commits intomainfrom
codex/propose-fix-for-logged-s3-url-vulnerability

Conversation

@zaniebot
Copy link
Copy Markdown
Owner

Motivation

  • Prevent leakage of bearer tokens contained in pre-signed S3 upload URLs that were being emitted to logs at debug level because DisplaySafeUrl does not redact query parameters.

Description

  • Remove the string interpolation that printed the parsed pre-signed S3 URL from the debug message while keeping the debug event to indicate a pre-signed URL was received.
  • No changes to the upload flow, retry logic, or URL parsing are made; only the sensitive debug output was modified (one-line change in crates/uv-publish/src/lib.rs).

Testing

  • Ran cargo test -p uv-publish --lib, which compiled and executed the unit tests; the test run produced 8 tests with 3 passed and 5 failed and therefore the test command returned failure.
  • The failing snapshot/network-dependent tests report 403 Forbidden responses that are unrelated to this log-only change and indicate existing test harness/environment differences rather than the fix itself.

Codex Task

Comment on lines -819 to 820
debug!("Got pre-signed URL for upload: {s3_url}");
debug!("Got pre-signed URL for upload");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we just elide the secret part instead?

Comment thread crates/uv-publish/src/lib.rs Outdated
///
/// Returns `true` if the file was newly uploaded and `false` if it already existed.
pub async fn upload_two_phase(
pub fn elide_presigned_query(mut url: DisplaySafeUrl) -> DisplaySafeUrl {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should clearing query components be a setting on DisplaySafeUrl?

@zanieb zanieb force-pushed the codex/propose-fix-for-logged-s3-url-vulnerability branch from cf1363f to 4f05bf9 Compare April 29, 2026 18:26
@zanieb zanieb force-pushed the codex/propose-fix-for-logged-s3-url-vulnerability branch from 4f05bf9 to 8569410 Compare April 29, 2026 18:35
@zanieb zanieb closed this Apr 29, 2026
zanieb added a commit to astral-sh/uv that referenced this pull request Apr 30, 2026
Small follow-up to #19146 which I
was looking into independently as a follow-up from the Codex security
patch at zaniebot#19

---------

Co-authored-by: zaniebot <zaniebot@accounts.zanie.blue>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants