Skip to content

Commit ea3c61d

Browse files
SGudbrandssondanny-aviladinershtein
authored
🪣 fix: Decode S3 Object Keys from URLs (#15426)
* 🔤 fix: Decode percent-encoded S3 keys so non-ASCII filenames are readable extractKeyFromS3Url returns `URL.pathname` as the S3 object key without decoding it. The AWS SDK percent-encodes the Key again when it signs the request, so a file stored under `Ársreikningur.pdf` is fetched as `%C3%81rsreikningur.pdf` and every read fails with NoSuchKey. ASCII keys are byte-identical either way, which is why this only appears for non-English filenames. Decode keys derived from a URL path in all three branches (path-style endpoint, bucket-in-path, virtual-hosted). A key passed in raw — not a URL — still returns untouched, and a malformed escape sequence falls back to the raw value with a warning rather than throwing. * 🔗 fix: Percent-encode CloudFront URL keys so both URL forms agree buildCloudFrontUrl interpolated the raw S3 key into the URL while SDK-generated S3 URLs carry an encoded one, so the two forms of `file.filepath` disagreed about what a `%` means. `assertS3FileName` permits `%`, so a key containing the literal text `report%20final.pdf` produced a CloudFront URL indistinguishable from one for a key containing a space — and decoding on extraction would then target the wrong object on read, re-sign, and delete. Encoding each path segment here (separators stay literal) makes both producers consistent, which is what lets extractKeyFromS3Url decode unconditionally. * style: sort imports in cloudfront/crud.ts (pre-existing drift) The changed-file import-sort gate flags this file; the drift predates this PR (the untouched upstream version fails the same check). Kept as its own commit so it does not obscure the fix. * 🔏 fix: Encode the CloudFront Invalidation Path Like the Viewer URL `buildCloudFrontUrl` now percent-encodes each key segment, so the cached viewer path for a key with a literal `%` or a non-ASCII character is the encoded form. `deleteFileFromCloudFront` still handed the raw key to `CreateInvalidationCommand`, so the invalidation no longer matched the cached object and deleted content stayed served until the entry expired. Both producers now share one `encodeKeyPath` helper. Also asserts that `getS3FileStream` sends the decoded key to `GetObjectCommand`, which is the call that actually failed with `NoSuchKey`, rather than only checking the extractor. Co-authored-by: dinershtein <228485+dinershtein@users.noreply.github.com> --------- Co-authored-by: Danny Avila <danny@librechat.ai> Co-authored-by: dinershtein <228485+dinershtein@users.noreply.github.com>
1 parent 1aa86a9 commit ea3c61d

4 files changed

Lines changed: 138 additions & 16 deletions

File tree

‎packages/api/src/storage/cloudfront/__tests__/crud.test.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,28 @@ describe('CloudFront CRUD', () => {
9292
expect(mockGetSignedUrl).not.toHaveBeenCalled();
9393
});
9494

95+
it('percent-encodes non-ASCII characters in the key', async () => {
96+
const { getCloudFrontURL } = await import('~/storage/cloudfront/crud');
97+
const url = await getCloudFrontURL({ userId: 'user1', fileName: 'Ársreikningur.pdf' });
98+
expect(url).toBe('https://d123.cloudfront.net/i/images/user1/%C3%81rsreikningur.pdf');
99+
});
100+
101+
it('percent-encodes a literal % so it cannot be read back as an escape', async () => {
102+
const { getCloudFrontURL } = await import('~/storage/cloudfront/crud');
103+
const url = await getCloudFrontURL({ userId: 'user1', fileName: 'report%20final.pdf' });
104+
expect(url).toBe('https://d123.cloudfront.net/i/images/user1/report%2520final.pdf');
105+
});
106+
107+
it('leaves path separators literal', async () => {
108+
const { getCloudFrontURL } = await import('~/storage/cloudfront/crud');
109+
const url = await getCloudFrontURL({
110+
userId: 'user1',
111+
fileName: 'doc.pdf',
112+
basePath: 'documents',
113+
});
114+
expect(url).toBe('https://d123.cloudfront.net/documents/user1/doc.pdf');
115+
});
116+
95117
it('uses custom basePath when provided', async () => {
96118
const { getCloudFrontURL } = await import('~/storage/cloudfront/crud');
97119
const url = await getCloudFrontURL({
@@ -512,6 +534,26 @@ describe('CloudFront CRUD', () => {
512534
);
513535
});
514536

537+
it('encodes the invalidation path the same way as the viewer URL', async () => {
538+
mockResolveStoredS3Key.mockReturnValue('images/u/report%20final \u0151.webp');
539+
mockGetCloudFrontConfig.mockReturnValue(
540+
makeConfig({ invalidateOnDelete: true, distributionId: 'E123' }),
541+
);
542+
mockCloudFrontSend.mockResolvedValue({});
543+
544+
const { deleteFileFromCloudFront } = await import('~/storage/cloudfront/crud');
545+
await deleteFileFromCloudFront(mockReq, mockFile);
546+
547+
const { CreateInvalidationCommand } = await import('@aws-sdk/client-cloudfront');
548+
expect(CreateInvalidationCommand).toHaveBeenCalledWith(
549+
expect.objectContaining({
550+
InvalidationBatch: expect.objectContaining({
551+
Paths: { Quantity: 1, Items: ['/images/u/report%2520final%20%C5%91.webp'] },
552+
}),
553+
}),
554+
);
555+
});
556+
515557
it('prefixes key with / for invalidation path', async () => {
516558
mockResolveStoredS3Key.mockReturnValue('images/u/file.webp'); // no leading slash
517559
mockGetCloudFrontConfig.mockReturnValue(

‎packages/api/src/storage/cloudfront/crud.ts‎

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,9 @@
11
import crypto from 'crypto';
2+
import { logger } from '@librechat/data-schemas';
23
import { getSignedUrl } from '@aws-sdk/cloudfront-signer';
34
import { CloudFrontClient, CreateInvalidationCommand } from '@aws-sdk/client-cloudfront';
4-
import { logger } from '@librechat/data-schemas';
55
import type { TFile } from 'librechat-data-provider';
66
import type { Readable } from 'stream';
7-
import type { ServerRequest } from '~/types';
87
import type {
98
SaveBufferParams,
109
GetURLParams,
@@ -14,10 +13,7 @@ import type {
1413
SaveURLResult,
1514
UploadResult,
1615
} from '~/storage/types';
17-
import { getCloudFrontConfig } from '~/cdn/cloudfront';
18-
import { s3Config } from '~/storage/s3/s3Config';
19-
import { AVATAR_BASE_PATH, DEFAULT_BASE_PATH as defaultBasePath } from '~/storage/constants';
20-
import { sanitizeContentDispositionFilename } from '~/storage/validation';
16+
import type { ServerRequest } from '~/types';
2117
import {
2218
getS3Key,
2319
saveBufferToS3,
@@ -27,6 +23,10 @@ import {
2723
getS3FileStream,
2824
resolveStoredS3Key,
2925
} from '~/storage/s3/crud';
26+
import { AVATAR_BASE_PATH, DEFAULT_BASE_PATH as defaultBasePath } from '~/storage/constants';
27+
import { sanitizeContentDispositionFilename } from '~/storage/validation';
28+
import { getCloudFrontConfig } from '~/cdn/cloudfront';
29+
import { s3Config } from '~/storage/s3/s3Config';
3030

3131
let _cloudFrontClient: CloudFrontClient | null = null;
3232

@@ -77,14 +77,25 @@ function isInlineFileUpload({ basePath, file, useInlinePath }: UploadFileParams)
7777
return (basePath ?? defaultBasePath) === defaultBasePath && file.mimetype?.startsWith('image/');
7878
}
7979

80+
/**
81+
* Percent-encodes each path segment of an S3 key (the separators stay literal). Without this a
82+
* CloudFront URL carries the raw key while an SDK-generated S3 URL carries an encoded one, so the
83+
* two forms of `file.filepath` disagree about what a `%` means: a key containing the literal text
84+
* `%20` would be indistinguishable from a key containing a space. Encoding here keeps both
85+
* producers consistent, which is what lets `extractKeyFromS3Url` decode unconditionally. The
86+
* invalidation path must use the same encoding, or it no longer matches the cached viewer URL.
87+
*/
88+
function encodeKeyPath(s3Key: string): string {
89+
return s3Key.replace(/^\/+/, '').split('/').map(encodeURIComponent).join('/');
90+
}
91+
8092
function buildCloudFrontUrl(s3Key: string): string {
8193
const config = getCloudFrontConfig();
8294
if (!config?.domain) {
8395
throw new Error('[buildCloudFrontUrl] CloudFront not initialized.');
8496
}
8597
const cleanDomain = config.domain.replace(/\/+$/, '');
86-
const cleanKey = s3Key.replace(/^\/+/, '');
87-
return `${cleanDomain}/${cleanKey}`;
98+
return `${cleanDomain}/${encodeKeyPath(s3Key)}`;
8899
}
89100

90101
function signUrl(url: string | URL): string {
@@ -235,8 +246,7 @@ export async function deleteFileFromCloudFront(req: ServerRequest, file: TFile):
235246
try {
236247
const client = getOrCreateCloudFrontClient();
237248
// CloudFront URL pathname matches S3 key when no origin path prefix is configured
238-
const key = resolveStoredS3Key(file);
239-
const path = key.startsWith('/') ? key : `/${key}`;
249+
const path = `/${encodeKeyPath(resolveStoredS3Key(file))}`;
240250

241251
await client.send(
242252
new CreateInvalidationCommand({

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

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1118,6 +1118,19 @@ describe('S3 CRUD', () => {
11181118
expect(s3Mock.commandCalls(GetObjectCommand)).toHaveLength(1);
11191119
});
11201120

1121+
it('requests the decoded key for a non-ASCII file name', async () => {
1122+
const { getS3FileStream } = await import('../crud');
1123+
await getS3FileStream(
1124+
{} as ServerRequest,
1125+
'https://test-bucket.s3.amazonaws.com/images/user123/%D0%94%D0%BE%D0%B3%D0%BE%D0%B2%D0%BE%D1%80.pdf',
1126+
);
1127+
1128+
const [call] = s3Mock.commandCalls(GetObjectCommand);
1129+
expect(call.args[0].input.Key).toBe(
1130+
'images/user123/\u0414\u043e\u0433\u043e\u0432\u043e\u0440.pdf',
1131+
);
1132+
});
1133+
11211134
it('handles errors when retrieving stream', async () => {
11221135
s3Mock.on(GetObjectCommand).rejects(new Error('Stream error'));
11231136

@@ -1463,6 +1476,40 @@ describe('S3 CRUD', () => {
14631476
expect(key).toBe('images/user123/file.png');
14641477
});
14651478

1479+
it('decodes percent-encoded keys from virtual-hosted-style URLs', async () => {
1480+
const { extractKeyFromS3Url } = await import('../crud');
1481+
const key = extractKeyFromS3Url(
1482+
'https://bucket.s3.amazonaws.com/uploads/user123/abc__%C3%81rsreikningur_2025.pdf',
1483+
);
1484+
expect(key).toBe('uploads/user123/abc__Ársreikningur_2025.pdf');
1485+
});
1486+
1487+
it('decodes percent-encoded keys from path-style URLs', async () => {
1488+
const { extractKeyFromS3Url } = await import('../crud');
1489+
const key = extractKeyFromS3Url(
1490+
'https://s3.us-west-2.amazonaws.com/test-bucket/uploads/user123/%E6%97%A5%E6%9C%AC%E8%AA%9E.pdf',
1491+
);
1492+
expect(key).toBe('uploads/user123/日本語.pdf');
1493+
});
1494+
1495+
it('preserves a literal percent in a filename (round-trip through %25)', async () => {
1496+
const { extractKeyFromS3Url } = await import('../crud');
1497+
const key = extractKeyFromS3Url('https://bucket.s3.amazonaws.com/uploads/100%25_done.pdf');
1498+
expect(key).toBe('uploads/100%_done.pdf');
1499+
});
1500+
1501+
it('returns a raw key untouched even when it contains a percent sign', async () => {
1502+
const { extractKeyFromS3Url } = await import('../crud');
1503+
const key = 'uploads/user123/100%_done.pdf';
1504+
expect(extractKeyFromS3Url(key)).toBe(key);
1505+
});
1506+
1507+
it('falls back to the raw value on a malformed escape sequence', async () => {
1508+
const { extractKeyFromS3Url } = await import('../crud');
1509+
const key = extractKeyFromS3Url('https://bucket.s3.amazonaws.com/uploads/bad%E0%A4A.pdf');
1510+
expect(key).toBe('uploads/bad%E0%A4A.pdf');
1511+
});
1512+
14661513
it('extracts key from path-style regional endpoint', async () => {
14671514
const { extractKeyFromS3Url } = await import('../crud');
14681515
const key = extractKeyFromS3Url(
@@ -1511,12 +1558,13 @@ describe('S3 CRUD', () => {
15111558
expect(key).toBe('folder/file.txt');
15121559
});
15131560

1514-
it('handles URLs with encoded characters', async () => {
1561+
it('decodes URLs with encoded characters back to the stored key', async () => {
15151562
const { extractKeyFromS3Url } = await import('../crud');
15161563
const key = extractKeyFromS3Url(
15171564
'https://bucket.s3.amazonaws.com/test-bucket/images/user123/my%20file%20name.jpg',
15181565
);
1519-
expect(key).toBe('images/user123/my%20file%20name.jpg');
1566+
/** The object was stored under `my file name.jpg`; `%20` is URL transport, not part of the key. */
1567+
expect(key).toBe('images/user123/my file name.jpg');
15201568
});
15211569

15221570
it('handles deep nested paths', async () => {

‎packages/api/src/storage/s3/crud.ts‎

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -638,6 +638,27 @@ export async function saveURLToS3(
638638
return filepath;
639639
}
640640

641+
/**
642+
* Decodes a key taken from a URL path.
643+
*
644+
* `URL.pathname` is percent-encoded, but S3 object keys are raw UTF-8 and the AWS SDK encodes
645+
* them again when it signs the request. Returning the encoded pathname therefore asks S3 for a
646+
* literally different key (`%C3%81rsreikningur.pdf` instead of `Ársreikningur.pdf`) and every
647+
* read of a file whose name contains a non-ASCII character fails with `NoSuchKey`. Keys made of
648+
* ASCII are unaffected, which is why this only shows up for non-English filenames.
649+
*
650+
* Malformed sequences fall back to the raw value rather than throwing — a key that cannot be
651+
* decoded is still worth attempting.
652+
*/
653+
function decodeKeyFromUrlPath(key: string): string {
654+
try {
655+
return decodeURIComponent(key);
656+
} catch {
657+
logger.warn(`[extractKeyFromS3Url] Could not decode key, using it as-is: ${key}`);
658+
return key;
659+
}
660+
}
661+
641662
export function extractKeyFromS3Url(fileUrlOrKey: string): string {
642663
if (!fileUrlOrKey) {
643664
throw new Error('Invalid input: URL or key is empty');
@@ -659,7 +680,7 @@ export function extractKeyFromS3Url(fileUrlOrKey: string): string {
659680
(endpointUrl.pathname.endsWith('/') ? 0 : 1) +
660681
bucketName.length +
661682
1;
662-
const key = url.pathname.substring(startPos);
683+
const key = decodeKeyFromUrlPath(url.pathname.substring(startPos));
663684
if (!key) {
664685
logger.warn(
665686
`[extractKeyFromS3Url] Extracted key is empty for endpoint path-style URL: ${fileUrlOrKey}`,
@@ -677,7 +698,7 @@ export function extractKeyFromS3Url(fileUrlOrKey: string): string {
677698
) {
678699
const firstSlashIndex = pathname.indexOf('/');
679700
if (firstSlashIndex > 0) {
680-
const key = pathname.substring(firstSlashIndex + 1);
701+
const key = decodeKeyFromUrlPath(pathname.substring(firstSlashIndex + 1));
681702
if (key === '') {
682703
logger.warn(
683704
`[extractKeyFromS3Url] Extracted key is empty after removing bucket name from URL: ${fileUrlOrKey}`,
@@ -695,8 +716,9 @@ export function extractKeyFromS3Url(fileUrlOrKey: string): string {
695716
return '';
696717
}
697718

698-
logger.debug(`[extractKeyFromS3Url] fileUrlOrKey: ${fileUrlOrKey}, Extracted key: ${pathname}`);
699-
return pathname;
719+
const key = decodeKeyFromUrlPath(pathname);
720+
logger.debug(`[extractKeyFromS3Url] fileUrlOrKey: ${fileUrlOrKey}, Extracted key: ${key}`);
721+
return key;
700722
} catch (error) {
701723
if (fileUrlOrKey.startsWith('http://') || fileUrlOrKey.startsWith('https://')) {
702724
logger.error(

0 commit comments

Comments
 (0)