From 7592bd8dabbe9f6f5fffbb9f2e5c2b2c6c64f671 Mon Sep 17 00:00:00 2001 From: Graeme Foster <80714+GraemeF@users.noreply.github.com> Date: Thu, 22 Oct 2026 18:14:46 +0100 Subject: [PATCH] Refuse a download path that climbs out of /user_uploads An upload path was accepted on its /user_uploads/ prefix alone. The HTTP client resolves dot segments, their percent-encoded forms and backslashes before it sends, so a path such as /user_uploads/../api/v1/users reached an API endpoint with the minter's credentials. The upload-path brand now accepts only a path that URL parsing leaves unchanged once decoded, and whose segments decode to no separator. Both the adapter's downloadFile and the download_file tool decode through it, so a refused path sends no request. The shared attachment contract now tries those climbs, plainly written and percent-encoded, and checks that a file with a non-ASCII name still downloads. To make that second test mean what it does on Zulip, the in-process realm now sanitises upload names the way Zulip does, keeping Unicode word characters, and decodes a request path before it routes it. --- packages/testing/contract/attachments.ts | 29 ++++++++++++++++++++++++ packages/zulip/http.ts | 24 ++++++++++++++++++++ packages/zulip/in-process-realm.ts | 23 +++++++++++++++---- 3 files changed, 71 insertions(+), 5 deletions(-) diff --git a/packages/testing/contract/attachments.ts b/packages/testing/contract/attachments.ts index 5cbf9cf..ef44268 100644 --- a/packages/testing/contract/attachments.ts +++ b/packages/testing/contract/attachments.ts @@ -29,6 +29,15 @@ export const attachmentsContract = (env: () => AttachmentContractEnv): void => { expect(downloaded.filename).toBe(uploaded.filename) }), )) + test('attachments.downloadFile returns a file whose name is not ASCII', () => + Effect.runPromise( + Effect.gen(function* () { + const data = new TextEncoder().encode('bonjour') + const uploaded = yield* env().attachments.uploadFile('café.txt', data) + const downloaded = yield* env().attachments.downloadFile(uploaded.ref) + expect(downloaded.data).toEqual(data) + }), + )) // A ref is opaque above the port, so a caller can hand over one the realm // never issued. Two shapes: one built from nothing, and one that looks like // an issued ref but names no file. @@ -50,4 +59,24 @@ export const attachmentsContract = (env: () => AttachmentContractEnv): void => { } }), )) + // Eight climbs are more than any issued ref is deep, so each ref climbs to + // the realm's root. + test('attachments.downloadFile fails with a download AttachmentError on a ref that climbs out of an issued one', () => + Effect.runPromise( + Effect.gen(function* () { + const uploaded = yield* env().attachments.uploadFile( + 'real.txt', + new TextEncoder().encode('real'), + ) + const climbs = ['..', '%2e%2e', '%2E%2E', '.%2e', '..%2f..', '..%5c..'] + for (const climb of climbs) { + const ref = decodeAttachmentRefSync( + `${uploaded.ref}/${`${climb}/`.repeat(8)}api/v1/users`, + ) + const error = yield* Effect.flip(env().attachments.downloadFile(ref)) + expect(error._tag).toBe('AttachmentError') + expect(error.operation).toBe('download') + } + }), + )) } diff --git a/packages/zulip/http.ts b/packages/zulip/http.ts index f61cb1c..7ce972c 100644 --- a/packages/zulip/http.ts +++ b/packages/zulip/http.ts @@ -6,6 +6,7 @@ import { Effect, Either, Encoding, + Equal, Option, type ParseResult, Schedule, @@ -119,8 +120,31 @@ const decodeEnvelope = Schema.decodeUnknownEither(Schema.parseJson(envelopeSchem export type ZulipParamValue = string | number | boolean export type ZulipParams = Readonly> +const isPlainSegment = (segment: string): boolean => + Either.match(Encoding.decodeUriComponent(segment), { + onLeft: () => false, + onRight: (name) => !/[/\\]/.test(name), + }) + +const decodePath = (path: string): Option.Option => + Either.getRight(Encoding.decodeUriComponent(path)) + +// The HTTP client resolves dot segments, their percent-encoded forms and +// backslashes before it sends a request, so a path it would rewrite can land +// outside /user_uploads/ while still starting with it. +const isPlainPath = (raw: string): boolean => { + const written = decodePath(raw) + const sent = Option.fromNullable(URL.parse(raw, 'http://realm.invalid')).pipe( + Option.flatMap((url) => decodePath(url.pathname)), + ) + return ( + Option.isSome(written) && Equal.equals(written, sent) && raw.split('/').every(isPlainSegment) + ) +} + export const UserUploadPathSchema = Schema.NonEmptyString.pipe( Schema.pattern(/^\/user_uploads\//), + Schema.filter(isPlainPath, { message: () => 'a plain path under /user_uploads/' }), Schema.brand('UserUploadPath'), ) diff --git a/packages/zulip/in-process-realm.ts b/packages/zulip/in-process-realm.ts index 3d733a7..fc4825d 100644 --- a/packages/zulip/in-process-realm.ts +++ b/packages/zulip/in-process-realm.ts @@ -581,7 +581,16 @@ const isResolved = (topic: string): boolean => topic.startsWith(RESOLVED_TOPIC_P const unresolvedName = (topic: string): string => isResolved(topic) ? topic.slice(RESOLVED_TOPIC_PREFIX.length) : topic -const SAFE_FILENAME = /[^A-Za-z0-9._-]/g +// This follows Zulip's sanitize_name. Unicode word characters survive, so a +// stored path can hold characters that a URL percent-encodes. +const sanitizeUploadName = (name: string): string => { + const value = name + .normalize('NFKC') + .replace(/[^\p{L}\p{N}_\s.-]/gu, '') + .trim() + .replace(/[-\s]+/gu, '-') + return value === '' || value === '.' || value === '..' ? 'uploaded-file' : value +} export const makeInProcessRealm: Effect.Effect = Effect.gen(function* () { const serial = yield* Ref.make(0) @@ -1233,7 +1242,7 @@ export const makeInProcessRealm: Effect.Effect = Effect.gen(func Effect.gen(function* () { if (Option.isNone(call.file)) return yield* refuse('You must specify a file to upload') const file = call.file.value - const filename = file.filename.replace(SAFE_FILENAME, '-') || 'file' + const filename = sanitizeUploadName(file.filename) const url = `/user_uploads/2/${yield* nextId}/${filename}` yield* SubscriptionRef.update(stateRef, (s) => ({ ...s, @@ -1292,6 +1301,10 @@ export const makeInProcessRealm: Effect.Effect = Effect.gen(func const serve = (web: Request): Effect.Effect => Effect.gen(function* () { const url = new URL(web.url) + const pathname = Either.getOrElse( + Encoding.decodeUriComponent(url.pathname), + () => url.pathname, + ) const contentType = web.headers.get('content-type') ?? '' const multipart = contentType.startsWith('multipart/form-data') const file = multipart ? yield* firstFile(web) : Option.none() @@ -1301,10 +1314,10 @@ export const makeInProcessRealm: Effect.Effect = Effect.gen(func const state = yield* SubscriptionRef.get(stateRef) return yield* Effect.gen(function* () { const route = yield* Either.fromOption( - Arr.findFirst(routes, (r) => r.method === web.method && r.path.test(url.pathname)), + Arr.findFirst(routes, (r) => r.method === web.method && r.path.test(pathname)), () => refusal( - `in-process realm has no handler for ${web.method} ${url.pathname}`, + `in-process realm has no handler for ${web.method} ${pathname}`, 'NOT_FOUND', 404, ), @@ -1313,7 +1326,7 @@ export const makeInProcessRealm: Effect.Effect = Effect.gen(func return yield* route.handle({ caller, params, - path: (route.path.exec(url.pathname) ?? []).slice(1), + path: (route.path.exec(pathname) ?? []).slice(1), file, }) }).pipe(Effect.catchTag('RealmRefusal', (r) => Effect.succeed(refusalResponse(r))))