Skip to content

fix(clips): include the endpoint path prefix in the S3 canonical URI - #4225

Open
thesalmanx wants to merge 3 commits into
BuilderIO:mainfrom
thesalmanx:fix/s3-sigv4-endpoint-path-prefix
Open

fix(clips): include the endpoint path prefix in the S3 canonical URI#4225
thesalmanx wants to merge 3 commits into
BuilderIO:mainfrom
thesalmanx:fix/s3-sigv4-endpoint-path-prefix

Conversation

@thesalmanx

@thesalmanx thesalmanx commented Sep 3, 2026

Copy link
Copy Markdown

The bug

SigV4 signs the request's absolute path, but objectUri() builds the canonical URI as /{bucket}/{key} regardless of the endpoint. For an endpoint served under a path prefix this signs a different path than the one requested, and the server rejects it.

Supabase Storage is the case that hits this. Its S3 API lives at https://<project>.storage.supabase.co/storage/v1/s3, so:

  • request goes to /storage/v1/s3/{bucket}/{key}
  • signature covers /{bucket}/{key}
  • every upload returns 403

The request URL was always correct; only the signature was wrong. That's why it can't be caught by asserting the URL, and why the failure surfaces to the user as Video storage could not start an upload: S3 credentials are not configured, which points at the wrong thing entirely.

Plain S3 and R2 endpoints have a / pathname, so their canonical URI is unchanged by this fix.

The fix

objectUri() now prepends the endpoint's pathname, and the request URL is built from the endpoint's origin instead of the full endpoint, since the prefix would otherwise appear twice. Two lines, plus comments explaining the coupling between them.

Worth noting the file was already inconsistent about this: the URL parser a little further down (parseObjectUrl) handles endpoint.pathname correctly, so only the signer was missing it.

Verification

Against a real Supabase Storage endpoint, same key, same credentials, two signed PUTs:

canonical URI result
includes endpoint prefix (this PR) 200
omits it (current main) 403

The uploaded object then read back 200 over the public URL.

The added test pins the property that actually broke rather than the URL: two endpoints differing only by path prefix must produce different signatures. It fails on current main with two identical Authorization headers, and passes with the fix. Full file: 15 passed.

The same signer is copied elsewhere

Not touched here, since it is your call whether these should share code or stay independent copies. Three other places build the canonical URI as /{bucket}/{key} with no endpoint pathname, so all three reject a path-prefixed endpoint the same way:

  • packages/core/src/file-upload/s3.ts (objectPath, used by both putObject and the GET path)
  • templates/assets/server/lib/s3-upload-provider.ts (canonicalObjectUri, which also feeds getPresignedS3ObjectUrl, so a presigned link built against such an endpoint is signed for a path it will never be fetched from)
  • templates/analytics/server/lib/s3-upload-provider.ts

Happy to send the same two line change for any or all of them if you want it, or to leave them as they are.

Three lines in the new case ran past printWidth, so oxfmt --check failed
and the Lint & format job with it. Formatted with the repo's own oxfmt
rather than by hand. No assertions changed; still 15 of 15.
@thesalmanx
thesalmanx marked this pull request as ready for review September 5, 2026 04:58

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Builder reviewed your changes — looks good ✅

Review Details

Code Review Summary

PR #4225 fixes the Clips S3 SigV4 mismatch for endpoints served beneath a path prefix, such as Supabase Storage. The implementation derives the endpoint pathname for the canonical URI and then builds the request URL from the endpoint origin, preventing the prefix from being omitted from the signature or duplicated in the request path. The change is narrowly scoped, preserves the existing root-path behavior used by plain S3/R2 endpoints, and adds a focused regression test covering both URL construction and signature divergence.

Risk assessment: Standard — this changes shared server-side request signing and upload behavior, but does not alter authentication, authorization, or data mutation semantics beyond making valid path-prefixed uploads work.

One reviewer noted a low-severity test-strength limitation: the regression test checks that prefixed and unprefixed Authorization headers differ, rather than independently recomputing the expected SigV4 signature. This is useful coverage and the implementation itself is sound, but a future test could validate the exact canonical request to guard against unrelated signature changes. This falls below the standard-risk submission threshold.

✅ No high- or medium-severity issues found.

🧪 Browser testing: Skipped — PR only modifies backend S3 signing logic and its server-side test; no user-facing UI impact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant