fix(blobs): Encode blob key parts before saving to disk on windows (#316)

* chore: add test for weird keys

* test: add some even weirder keys

* chore: install only chromium

* test: add log when key set fails

* fix(blobs): encode path keys as well as store keys when saving and loading blobs

* fix(blobs): encode more names on win32

* fix(blobs): disallow more invalid win32 names

Author: chee

.github/workflows/test.yaml

@@ -48,7 +48,7 @@ jobs:
       - name: Install dependencies
         run: npm ci
       - name: Install playwright browsers
-        run: npx playwright install --with-deps
+        run: npx playwright install --with-deps chromium
       - name: Build
         run: npm run build --workspaces=true
       - name: Tests

packages/blobs/src/server.test.ts

@@ -149,6 +149,44 @@ test('Reads and writes from the file system', async () => {
   await fs.rm(directory2.path, { force: true, recursive: true })
 })
 
+test('Works with weird keys -- valid blob keys, tricky in filesystems', async () => {
+  const directory = await tmp.dir()
+  const server = new BlobsServer({
+    directory: directory.path,
+    token,
+  })
+  const { port } = await server.start()
+  const store = getStore({
+    edgeURL: `http://localhost:${port}`,
+    name: 'store',
+    token,
+    siteID,
+  })
+
+  // valid as keys and store names
+  const pipes = 'valid|key'
+  const question = 'key?'
+  const glob = '*hehe*'
+  const con = 'CON'
+  const emoji = '🐰'
+  const angles = '<>'
+  // valid only as keys
+  const slashes = 'a///b'
+  const namespace = 'name:space'
+  for (const key of [pipes, question, glob, con, angles, emoji, slashes, namespace]) {
+    try {
+      await store.set(key, 'value')
+      expect(await store.get(key)).toBe('value')
+    } catch (error) {
+      console.warn(`couldn't set valid key of "${key}"`)
+      throw error
+    }
+  }
+
+  await server.stop()
+  await fs.rm(directory.path, { force: true, recursive: true })
+})
+
 test('Separates keys from different stores', async () => {
   const directory = await tmp.dir()
   const server = new BlobsServer({

packages/blobs/src/server.ts

@@ -2,15 +2,14 @@ import { createHmac } from 'node:crypto'
 import { createReadStream, promises as fs } from 'node:fs'
 import { tmpdir } from 'node:os'
 import { dirname, join, relative, resolve, sep } from 'node:path'
-import { platform } from 'node:process'
 
 import { HTTPServer } from '@netlify/dev-utils'
 
 import { ListResponse } from './backend/list.ts'
 import { SIGNED_URL_ACCEPT_HEADER } from './client.ts'
 import { decodeMetadata, encodeMetadata, METADATA_HEADER_INTERNAL } from './metadata.ts'
 import { HTTPMethod } from './types.ts'
-import { isNodeError, Logger } from './util.ts'
+import { decodeName, encodeName, isNodeError, Logger } from './util.ts'
 
 const API_URL_PATH = /\/api\/v1\/blobs\/(?<site_id>[^/]+)\/(?<store_name>[^/]+)\/?(?<key>[^?]*)/
 const LEGACY_API_URL_PATH = /\/api\/v1\/sites\/(?<site_id>[^/]+)\/blobs\/?(?<key>[^?]*)/
@@ -231,7 +230,7 @@ export class BlobsServer {
     req: Request
     url: URL
   }): Promise<Response> {
-    const { dataPath, rootPath, req, url } = options
+    const { dataPath, rootPath, url } = options
     const directories = url.searchParams.get('directories') === 'true'
     const prefix = url.searchParams.get('prefix') ?? ''
     const result: ListResponse = {
@@ -259,10 +258,7 @@ export class BlobsServer {
   private async listStores(rootPath: string, prefix: string): Promise<Response> {
     try {
       const allStores = await fs.readdir(rootPath)
-      const filteredStores = allStores
-        // Store names are URI-encoded on Windows, so we must decode them first.
-        .map((store) => (platform === 'win32' ? decodeURIComponent(store) : store))
-        .filter((store) => store.startsWith(prefix))
+      const filteredStores = allStores.map(decodeName).filter((store) => store.startsWith(prefix))
 
       return Response.json({ stores: filteredStores })
     } catch (error) {
@@ -364,7 +360,7 @@ export class BlobsServer {
       parts = parts.slice(1)
     }
 
-    const [siteID, rawStoreName, ...key] = parts
+    const [siteID, rawStoreName, ...rawKey] = parts
 
     if (!siteID) {
       return {}
@@ -376,9 +372,9 @@ export class BlobsServer {
       return { rootPath }
     }
 
-    // On Windows, file paths can't include the `:` character, so we URI-encode
-    // them.
-    const storeName = platform === 'win32' ? encodeURIComponent(rawStoreName) : rawStoreName
+    const key = rawKey.map(encodeName)
+
+    const storeName = encodeName(rawStoreName)
     const storePath = resolve(rootPath, storeName)
     const dataPath = resolve(storePath, ...key)
     const metadataPath = resolve(this.directory, 'metadata', siteID, storeName, ...key)

packages/blobs/src/util.test.ts

@@ -0,0 +1,43 @@
+import { it, expect, describe } from 'vitest'
+
+import { decodeWin32SafeName, encodeWin32SafeName } from './util.ts'
+
+describe('win32 safe names', () => {
+  it('encodes unsafe path parts', () => {
+    const unsafe = 'hello|*<>world'
+    const safe = encodeWin32SafeName(unsafe)
+    expect(safe).not.toContain('|')
+    expect(safe).not.toContain('.')
+    expect(safe).not.toContain('*')
+    expect(safe).not.toContain('<')
+    expect(safe).not.toContain('>')
+  })
+
+  it('disallows invalid names', () => {
+    expect(encodeWin32SafeName('CON')).not.toBe('CON')
+    expect(encodeWin32SafeName('COM1')).not.toBe('COM1')
+    expect(encodeWin32SafeName('com2')).not.toBe('com2')
+    expect(encodeWin32SafeName('NUL')).not.toBe('NUL')
+    expect(encodeWin32SafeName('PRN')).not.toBe('PRN')
+    expect(encodeWin32SafeName('LPT3')).not.toBe('LPT3')
+
+    // no false positives
+    expect(encodeWin32SafeName('annuling')).toBe('annuling')
+  })
+
+  it('replaces end dots', () => {
+    const safe = encodeWin32SafeName('hello.')
+    expect(safe).not.toMatch(/\.$/)
+  })
+
+  it('replaces end spaces', () => {
+    const safe = encodeWin32SafeName('hehe ')
+    expect(safe).not.toMatch(/\s+$/)
+  })
+
+  it('can be reversed', () => {
+    const unsafe = 'hello|.*<>world'
+    const safe = encodeWin32SafeName(unsafe)
+    expect(decodeWin32SafeName(safe)).toEqual(unsafe)
+  })
+})

packages/blobs/src/util.ts

@@ -1,3 +1,4 @@
+import process from 'node:process'
 import { NF_ERROR, NF_REQUEST_ID } from './headers.ts'
 
 export class BlobsInternalError extends Error {
@@ -27,3 +28,38 @@ export const collectIterator = async <T>(iterator: AsyncIterable<T>): Promise<T[
 export const isNodeError = (error: unknown): error is NodeJS.ErrnoException => error instanceof Error
 
 export type Logger = (...message: unknown[]) => void
+
+function percentEncode(str: string): string {
+  return str.replace(/./, (char) => {
+    return '%' + char.charCodeAt(0).toString(16).padStart(2, '0')
+  })
+}
+
+const invalidWin32File = /^(CON|COM[1-9]|LPT[1-9]|NUL|PRN|AUX)$/i
+
+/*
+ *  On Windows, file paths can't include some valid blob/store key characters, so we URI-encode them.  fixme: limitations
+ *
+ *  Limitations:
+ *    - this doesn't deal with long names (blob keys can be 600 chars, default on windows is max 260)
+ *  For keys (which we don't need to decode) maybe a hash would be a better idea
+ */
+export function encodeWin32SafeName(string: string): string {
+  if (invalidWin32File.exec(string)) {
+    return percentEncode(string)
+  }
+  return encodeURIComponent(string).replace(/([*]|[. ]$)/g, percentEncode)
+}
+
+// Names are URI-encoded on Windows, so we must decode them first.
+export function decodeWin32SafeName(string: string): string {
+  return decodeURIComponent(string)
+}
+
+export function encodeName(string: string): string {
+  return process.platform == 'win32' ? encodeWin32SafeName(string) : string
+}
+
+export function decodeName(string: string): string {
+  return process.platform == 'win32' ? decodeWin32SafeName(string) : string
+}