Refuse a download path that climbs out of /user_uploads

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')
+        }
+      }),
+    ))
 }

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<Record<string, ZulipParamValue>>
 
+const isPlainSegment = (segment: string): boolean =>
+  Either.match(Encoding.decodeUriComponent(segment), {
+    onLeft: () => false,
+    onRight: (name) => !/[/\\]/.test(name),
+  })
+
+const decodePath = (path: string): Option.Option<string> =>
+  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'),
 )
 

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<InProcessRealm> = Effect.gen(function* () {
   const serial = yield* Ref.make(0)
@@ -1233,7 +1242,7 @@ export const makeInProcessRealm: Effect.Effect<InProcessRealm> = 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<InProcessRealm> = Effect.gen(func
   const serve = (web: Request): Effect.Effect<Response> =>
     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<InProcessRealm> = 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<InProcessRealm> = 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))))