fix(storage): url-encode object keys in file API request paths

[Sy-D]

Description

Object keys may contain characters the Storage server accepts but that change the meaning of a URL. _getFinalPath interpolated them into the request path raw, so a valid key was sent as key-plus-querystring and the server resolved a different object, with no error:

exists("folder/a?b.txt")
  → GET /object/bucket/folder/a?b.txt     // server sees key "folder/a"

This is the same failure #2545 fixed for the CDN purge methods — the helper and rationale added there (encodeStoragePath) were never applied to the rest of the file API.

Affected: upload/update, createSignedUploadUrl, createSignedUrl, createSignedUrls, info, exists, getPublicUrl.

Which characters this is about

I probed a local Storage server rather than guessing. It accepts these in an object key and stores them verbatim:

?   space   &   =   +   ,   (   )   @   :   ;   $   *   !   and the apostrophe

It rejects #, %, [, ], ~, backtick, ^, | and all non-ASCII with 400 InvalidKey. So # was never the interesting case — ?, a space, &, = and + are.

What changed

Encoding moved into _getFinalPath, so every call site is covered and a new one cannot silently reintroduce the bug. All 8 uses of _getFinalPath feed a URL; none goes into a request body.

Two follow-on removals, both required to avoid double-encoding:

• purgeCache no longer wraps with encodeStoragePath (now redundant).
• getPublicUrl no longer wraps with encodeURI. That wrapper was insufficient anyway — encodeURI leaves ?, &, = and + untouched — and it would double-encode the new output.

The encodeURI in createSignedUrl stays. It wraps data.signedURL from the server response, and the server returns that path with %20 already decoded to a literal space, so it is still required there.

Testing

packages/core/storage-js/test/encode-object-key.test.ts — 4 unit tests (captured fetch, URL shape) plus 3 integration tests against the Docker-backed Storage server that upload folder/a?b&c=d e+f.txt and read the same object back.

On master, 6 of the 7 fail. The integration failure is the telling one: the upload lands as an object literally named a, and list("folder") returns ["a"].

Full nx test:storage storage-js:

	Suites	Tests	Snapshots
master	18 passed / 18	411 passed, 2 skipped	6 passed
this branch	19 passed / 19	418 passed, 2 skipped	6 passed

Both fully green; the delta is exactly this suite and its 7 tests.

nx lint storage-js reports 15 errors both here and on master — all pre-existing. nx format:check and nx build storage-js pass.

A server-side bug this does not fix

With this patch, upload, exists, info, download and getPublicUrl work for every accepted character. createSignedUrl does not: it returns 400 for ? & = + , ; @ : $, and works only for keys whose sole escape is %20 — plus the characters that need no escaping at all.

The cause is server-side: the token is signed over the percent-encoded key, but the request is validated against the decoded path, so the signature never matches. The URL the server itself returns fails identically, so no client-side encoding can work around it. Two tests pin this behaviour explicitly, including the 400, so the limitation is documented rather than silently shipped. Happy to open a separate storage-api issue if that would help.

Also worth knowing

Since % is an invalid key character, callers who percent-encode their keys before passing them in were already broken and remain so — they now fail at the client-encoding step instead of reaching the server. I do not believe that is a regression, but it is a behaviour change worth a maintainer eye.

Type of Change

• Bug fix (fix)

Checklist

• Code formatted (nx format)
• Unit tests added and passing
• Integration tests added and passing (nx test:storage storage-js, fully green)
• Builds passing (nx build storage-js)
• Used conventional commits

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes
  • Fixed storage URL generation for object keys with special characters (for example # and ?), ensuring they are properly percent-encoded.
  • Prevented encoded keys from being treated as URL fragments or query parameters, keeping them in the URL path.
  • Improved consistency across public URL, metadata/exists requests, signed URL generation, and cache purging.
• Tests
  • Added unit and local integration coverage for encoded-path behavior, including signed URL failure for keys containing ?.

Walkthrough

StorageFileApi now encodes bucket-prefixed storage paths through _getFinalPath(). Public URL construction uses the encoded path directly, and cache purging avoids applying a second encoding step. New unit and integration tests verify that object keys containing URL delimiters remain in URL pathnames across public URL generation, metadata, existence, downloads, and signed URL requests.

Sequence Diagram(s)

sequenceDiagram
  participant StorageClient
  participant StorageFileApi
  participant StorageEndpoint
  StorageClient->>StorageFileApi: request URL for object key
  StorageFileApi->>StorageFileApi: encode bucket and object path
  StorageFileApi->>StorageEndpoint: send URL with encoded pathname
  StorageEndpoint-->>StorageClient: return response or generated URL

Loading

Possibly related PRs

• supabase/supabase-js#2545: Encodes CDN purge paths containing ? and #.
• supabase/supabase-js#2553: Changes URI encoding for storage object paths in StorageFileApi.

Suggested labels: storage-js

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/core/storage-js/test/encode-object-key.test.ts`:
- Around line 58-62: Remove the hardcoded SERVICE_KEY service-role JWT from the
test and load the test-only credential from the established environment or CI
secret mechanism before constructing StorageClient. Preserve the Authorization
bearer format, fail clearly when the required secret is absent, and rotate the
exposed token if it was used outside an isolated local fixture.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 95773fc0-8154-4a7e-aa28-84e52c7b4b3c

📥 Commits

Reviewing files that changed from the base of the PR and between 1709090 and 2132350.

📒 Files selected for processing (2)

• packages/core/storage-js/src/packages/StorageFileApi.ts
• packages/core/storage-js/test/encode-object-key.test.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• packages/core/storage-js/src/packages/StorageFileApi.ts

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Remove the committed service-role JWT.

This hardcoded bearer credential grants service_role access and is sent to the Storage server. Inject a test-only key through environment/CI secrets instead, and rotate this token if it has been used outside an isolated local fixture.

🧰 Tools

🪛 Betterleaks (1.7.0)

[high] 60-60: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.

(jwt)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/storage-js/test/encode-object-key.test.ts` around lines 58 -
62, Remove the hardcoded SERVICE_KEY service-role JWT from the test and load the
test-only credential from the established environment or CI secret mechanism
before constructing StorageClient. Preserve the Authorization bearer format,
fail clearly when the required secret is absent, and rotate the exposed token if
it was used outside an isolated local fixture.

Sources: Path instructions, Linters/SAST tools

[Sy-D]

Thanks — I looked at this one carefully, but I do not think it applies here.

The token is not a credential. Its payload is:

{ "iss": "supabase-demo", "role": "service_role", "exp": 1983812996 }

That is the public demo key the Supabase CLI ships for local development, not a secret belonging to any project. The exact same string is already hardcoded in three other test files in this package:

• packages/core/storage-js/test/storageFileApi.test.ts
• packages/core/storage-js/test/storageFileApiNode.test.ts
• packages/core/storage-js/test/storageApi.test.ts

each with the same // secret key - bypasses RLS for testing / // Supabase CLI local development defaults comment. I copied that convention deliberately so the new test reads like its neighbours.

Loading it from an env var or CI secret instead would make this one file behave differently from the other three, and add a failure mode (missing secret) for a value that is a public constant. There is also nothing to rotate.

Happy to switch if maintainers would rather move all four files to a shared constant or an env lookup — but that seems like a separate change, not something this PR should do on its own.