fix(storage): percent-encode the object key in every URL, not just purgeCache

[PedroHenrique0713]

The bug

encodeStoragePath already exists in this repo, and its doc comment states the reason:

...key (e.g. ?, #) can't be interpreted as a querystring/fragment start.
Splits on / so real path separators stay literal — the storage server routes on them and decodes each segment back to the original key.

It is wired into exactly one call site (purgeCache). Every other method builds its URL straight from _getFinalPath, which only strips leading slashes:

private _getFinalPath(path: string) {
  return `${this.bucketId}/${path.replace(/^\/+/, '')}`
}

So an object key containing # or ? silently addresses a different object:

storage.from('bucket').getPublicUrl('folder/report#1.png')
// → https://<project>/storage/v1/object/public/bucket/folder/report#1.png

#1.png is a fragment. The server only ever sees .../folder/report, and the caller gets a 404 for a file that exists. Keys like report #1.pdf or Q&A?.png are ordinary user uploads, so this is reachable with no unusual input.

The same key breaks the URL in upload, uploadToSignedUrl, createSignedUploadUrl, createSignedUrl, the authenticated download path, info and exists — eight call sites in total.

getPublicUrl additionally wrapped the whole URL in encodeURI. That escapes a space, so the output looks encoded, while leaving # and ? untouched — the two characters that actually break routing.

The fix

Apply the existing helper at every call site, exactly as purgeCache already does, and drop the now-redundant encodeURI so there is a single mechanism.

Path separators keep working: encodeStoragePath splits on / before encoding each segment.

Verification

New test file test/object-key-encoding.test.ts, 6 assertions covering getPublicUrl (hash, question mark, separators kept literal, self-built query string untouched) plus info() and createSignedUrl() through an injected fetch.

• on master: 5 of the 6 fail, e.g. Expected .../report%231.png, Received .../report#1.png
• with the fix: all pass

Full storage-js suite goes from 409 to 415 passing. The 6 failures and 1 snapshot that remain are identical to master on my machine (integration specs that need a local storage server), confirmed with git stash. tsc --noEmit clean, prettier --check clean.

I could not exercise the integration specs against a real storage server, so the round trip that matters — server decoding each segment back to the original key — rests on the helper's own doc comment and on purgeCache already shipping this behaviour. Happy to adjust if any endpoint expects the raw key.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 58de7759-f47a-44bf-8537-a1014f9bd6ad

📥 Commits

Reviewing files that changed from the base of the PR and between aef432b and 6a8385f.

📒 Files selected for processing (2)

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

Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.

---

📝 Summary

Summary by CodeRabbit

• Bug Fixes
  • Fixed storage file operations involving object keys with URL-significant characters.
  • Public, signed, informational, upload, download, and existence-check URLs now encode path segments correctly while preserving path separators.
  • Corrected URL construction to prevent query parameters and object-key characters from being misinterpreted.

Walkthrough

StorageFileApi now applies encodeStoragePath to object-key paths before constructing upload, signed URL, download, metadata, existence, and public URLs. getPublicUrl no longer encodes the complete URL. New tests verify encoding for URL-significant characters, preservation of path separators, and encoding of generated query values.

Merge Risk: ⚪ Minimal · up to 6a838

The change percent-encodes object-key path segments across storage URLs while preserving separators, with passing targeted tests and clean type and formatting checks. No actionable merge-blocking risk remains after normal review.

---

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.}

[PedroHenrique0713]

One thing I left out of this PR on purpose, since it needs a maintainer call rather than a guess.

StorageBucketApi has the same shape of inconsistency this PR fixes in StorageFileApi: encodeStoragePath is applied to the bucket id in one place,

`${this.url}/cdn/${encodeStoragePath(id)}${queryString ? `?${queryString}` : ''}`

while getBucket, updateBucket, emptyBucket and deleteBucket interpolate the same id into the same position of the URL bare (${this.url}/bucket/${id}). StorageAnalyticsClient does the same at ${this.url}/bucket/${bucketName}.

I did not include it because I cannot verify from here which characters the server accepts in a bucket id, and that is what decides whether this is reachable or merely defensive. What I can say is that it is inconsistent within the same file, and that whoever added the cdn call site judged the encoding necessary for exactly that value in exactly that position.

For contrast, StorageAnalyticsClient does validate: it rejects anything outside the AWS object key rules before building a URL, which is why % is refused there. StorageBucketApi has no equivalent check.

Happy to extend this PR to those call sites, or to leave bucket ids alone if the server constrains them enough that encoding would be dead code. Just say which.