Skip to content

🐛 fix: decode percent-encoded S3 keys extracted from URLs - #14835

Open
dinershtein wants to merge 1 commit into
danny-avila:mainfrom
dinershtein:fix/s3-decode-key-from-url
Open

🐛 fix: decode percent-encoded S3 keys extracted from URLs#14835
dinershtein wants to merge 1 commit into
danny-avila:mainfrom
dinershtein:fix/s3-decode-key-from-url

Conversation

@dinershtein

Copy link
Copy Markdown

What happened

On a deployment with fileStrategy: "s3", any file whose name contains non-ASCII characters uploads fine but can never be read back. Every later read fails:

[getS3FileStream] Error retrieving S3 file stream: The specified key does not exist.
  Resource: '/my-bucket/uploads/<userId>/<uuid>__%25D0%2594%25D0%25BE%25D0%25B3%25D0%25BE%25D0%25B2%25D0%25BE%25D1%2580....pdf'

For the user this is silent: the file appears to attach, but the model never receives it and answers that no file was provided.

Root cause

extractKeyFromS3Url() derives the object key from URL.pathname, which is percent-encoded. Keys, however, are stored decodedgetS3Key() builds the key from the raw file name and PutObject stores it as-is. Listing the bucket confirms it:

key: uploads/<userId>/<uuid>__Договор_....pdf     ← what is stored

So the extracted key is ...__%D0%94%D0%BE%D0%B3..., the SDK then escapes the %, and the request that goes out asks for ...__%25D0%2594%25D0%25BE%25D0%25B3... — a key that does not exist.

Note that the correct key is already available: resolveStoredS3Key() prefers file.storageKey, and the DB record has it. But getS3FileStream(_req, filePath) receives only the path string, so it cannot use it — which is why the fix belongs in key extraction rather than in the caller.

This affects every non-ASCII alphabet (Cyrillic, CJK, accented Latin) and spaces, on both virtual-hosted and path-style URLs, so custom endpoints (MinIO, R2, Yandex Object Storage) are hit as well.

The fix

Decode keys that were derived from a URL path.

Keys supplied directly are deliberately left untouched: extractKeyFromS3Url('images/user/100%_final.pdf') must not be decoded, because there the % is part of the file name rather than an escape. The same reasoning applies to the fallback branch in catch, which also receives a key rather than a URL.

Tests

packages/api/src/storage/s3/__tests__/crud.test.ts:

  • decoding of escaped characters and of a non-ASCII file name taken from the URL path;
  • a literal % preserved when a key is passed instead of a URL;
  • a regression test asserting that getS3FileStream issues GetObjectCommand with the decoded key.

One existing expectation changed. handles URLs with encoded characters asserted images/user123/my%20file%20name.jpg, i.e. it codified the buggy behaviour; it now asserts images/user123/my file name.jpg, which is what the bucket actually contains.

npx jest src/storage in packages/api: 165 passed, 1 skipped (the integration spec, which needs live S3).

Verified in production

Patched image running against Yandex Object Storage (S3-compatible, path-style endpoint). The same 21 MB PDF that previously failed now streams in full:

прочитано байт: 21619868
сигнатура файла: "%PDF-"

Files whose name contains any non-ASCII character cannot be read back from
S3: uploading works, every later read fails with NoSuchKey.

`extractKeyFromS3Url()` derives the object key from `URL.pathname`, which is
percent-encoded, while keys are stored decoded — `getS3Key()` does not encode
the file name before `PutObject`. A file named "Договор.pdf" is therefore
looked up as "%D0%94%D0%BE%D0%B3%D0%BE%D0%B2%D0%BE%D1%80.pdf", the SDK escapes
the `%` again, and the request that goes out asks for
"%25D0%2594%25D0%25BE%25D0%25B3..." — a key that does not exist.

Decode keys derived from a URL path. Keys supplied directly are left alone:
there a `%` is part of the file name rather than an escape.

The existing "handles URLs with encoded characters" test asserted the encoded
form, so it is updated to the decoded one — matching what is actually stored
in the bucket.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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