From 3a381f34b59ec4874b8a261a48891c4b4375b387 Mon Sep 17 00:00:00 2001 From: Isaac Su Date: Thu, 13 Aug 2026 10:33:57 +1000 Subject: [PATCH 1/2] Implement url signing for upload and download URLs --- README.md | 26 ++ lib/env.ts | 28 +- lib/schemas.ts | 32 ++ lib/storage.ts | 13 +- lib/url-signing.ts | 111 +++++++ plugins/setup.ts | 9 +- routes/[...path].ts | 8 +- .../devstoreaccount1/upload/[uploadId].put.ts | 5 + routes/download/[cacheEntryId].ts | 3 + .../CreateCacheEntry.post.ts | 11 +- tests/setup.ts | 1 + tests/url-signing-e2e.test.ts | 118 +++++++ tests/url-signing.test.ts | 308 ++++++++++++++++++ 13 files changed, 653 insertions(+), 20 deletions(-) create mode 100644 lib/url-signing.ts create mode 100644 tests/url-signing-e2e.test.ts create mode 100644 tests/url-signing.test.ts diff --git a/README.md b/README.md index 90307017..91812d4a 100644 --- a/README.md +++ b/README.md @@ -28,6 +28,32 @@ volumes: cache-data: ``` +## Signed cache URLs (optional) + +The server-proxied upload and download URLs (`/devstoreaccount1/upload/{id}` and +`/download/{id}`) are unauthenticated by default. You can enable an expiring HMAC signature +on them: + +| Env var | Description | +| - | - | +| `URL_SIGNING_ENABLED` | `boolean`, default `false`. When `true`, upload/download URLs are signed on generation and strictly verified on the handlers (no unsigned fallback). | +| `URL_SIGNING_SECRET` | The active signing secret, **≥ 16 chars**. Signs every issued URL and is the first candidate on verification. Required when signing is enabled — boot fails otherwise. | +| `URL_SIGNING_SECRET_SECONDARY` | Optional verify-only rotation secret, **≥ 16 chars when set**. Never signs; accepted on verification so URLs minted with the previous secret keep working. | + +Notes: + +- **Stable shared value:** the secrets must be identical across all cluster workers/pods, or + a signature minted by one worker will fail verification on another. +- **Rotation:** move the current `URL_SIGNING_SECRET` into `URL_SIGNING_SECRET_SECONDARY`, + set the new secret as `URL_SIGNING_SECRET`, then drop `URL_SIGNING_SECRET_SECONDARY` after + the 1h signature TTL has elapsed — no disruption. +- **Fixed 1h TTL:** signatures expire after 1 hour and cannot be refreshed mid-upload, so a + single upload running longer than 1h will fail. +- **Enabling is a hard cutover:** flipping `URL_SIGNING_ENABLED` to `true` immediately makes + every already-issued unsigned URL return `401`. Downloads mostly recover (buildx + re-requests a fresh URL), but in-flight cache saves fail and re-run on the next job — + prefer enabling during low activity. + ## Documentation 👉 👈 diff --git a/lib/env.ts b/lib/env.ts index 587caa4e..59f63c71 100644 --- a/lib/env.ts +++ b/lib/env.ts @@ -1,15 +1,19 @@ import type { envDbDriverSchema, envStorageDriverSchema } from './schemas' import arkenv from 'arkenv' -import { envSchema } from './schemas' +import { envSchema, envSchemaValidated } from './schemas' -export const env = arkenv(envSchema, { - env: Object.assign( - { - STORAGE_DRIVER: 'filesystem', - STORAGE_FILESYSTEM_PATH: '.data/storage/filesystem', - DB_DRIVER: 'sqlite', - DB_SQLITE_PATH: '.data/sqlite.db', - } satisfies typeof envStorageDriverSchema.infer & typeof envDbDriverSchema.infer, - process.env, - ), -}) +// arkenv drops arktype's object morphs on a narrowed root (see `envSchemaValidated`), +// so re-apply them by running its output through `envSchema.assert`. +export const env = envSchema.assert( + arkenv(envSchemaValidated, { + env: Object.assign( + { + STORAGE_DRIVER: 'filesystem', + STORAGE_FILESYSTEM_PATH: '.data/storage/filesystem', + DB_DRIVER: 'sqlite', + DB_SQLITE_PATH: '.data/sqlite.db', + } satisfies typeof envStorageDriverSchema.infer & typeof envDbDriverSchema.infer, + process.env, + ), + }), +) diff --git a/lib/schemas.ts b/lib/schemas.ts index bca21c62..637dc2b5 100644 --- a/lib/schemas.ts +++ b/lib/schemas.ts @@ -72,7 +72,39 @@ export const envBaseSchema = type({ 'BENCHMARK': 'boolean = false', 'SKIP_TOKEN_VALIDATION': 'boolean = false', 'MANAGEMENT_API_KEY?': 'string', + 'URL_SIGNING_ENABLED': 'boolean = false', + 'URL_SIGNING_SECRET?': type('string').pipe((s) => s.trim()), + 'URL_SIGNING_SECRET_SECONDARY?': type('string').pipe((s) => s.trim()), }) export const envSchema = envBaseSchema.and(envStorageDriverSchema).and(envDbDriverSchema) export type Env = typeof envSchema.infer + +const URL_SIGNING_SECRET_MIN_LENGTH = 16 + +/** + * Cross-field validation for the signing secrets, used by `lib/env.ts`. Separate + * from `envSchema` because arkenv's `.get()` throws on a narrowed schema, and + * `tests/setup.ts` calls `envSchema.get(...)`. + * + * Validation only — arkenv drops object morphs on a narrowed root, so `lib/env.ts` + * re-applies them via `envSchema.assert(...)` and the predicate must `.trim()` the + * secrets itself (it sees the raw, un-morphed value). + */ +export const envSchemaValidated = envSchema.narrow((data, ctx) => { + if (!data.URL_SIGNING_ENABLED) return true + + const secret = data.URL_SIGNING_SECRET?.trim() + if (!secret || secret.length < URL_SIGNING_SECRET_MIN_LENGTH) + return ctx.reject( + `URL_SIGNING_ENABLED requires URL_SIGNING_SECRET (>= ${URL_SIGNING_SECRET_MIN_LENGTH} chars)`, + ) + + const secondary = data.URL_SIGNING_SECRET_SECONDARY?.trim() + if (secondary && secondary.length < URL_SIGNING_SECRET_MIN_LENGTH) + return ctx.reject( + `URL_SIGNING_SECRET_SECONDARY must be >= ${URL_SIGNING_SECRET_MIN_LENGTH} chars when set`, + ) + + return true +}) diff --git a/lib/storage.ts b/lib/storage.ts index c31bd21c..0078fdec 100644 --- a/lib/storage.ts +++ b/lib/storage.ts @@ -42,6 +42,7 @@ import { renewReaderLease, } from './storage-leases' import { deleteStorageLocationIfUnread, noActiveReaderLease } from './storage-lifecycle' +import { signQuery, urlSigningConfigFromEnv } from './url-signing' // Bounds the self-heal retry when matching keeps surfacing Dangling Cache // Entries for the same prefix — caps a pathological scan (ADR-0005). @@ -690,11 +691,17 @@ export class Storage { continue } - const defaultUrl = `${env.API_BASE_URL}/download/${cacheEntry.match.id}` + // The signed proxied URL — computed lazily so the HMAC is only minted on + // the code paths that actually emit it. + const proxiedDownloadUrl = () => + `${env.API_BASE_URL}/download/${cacheEntry.match.id}${signQuery( + `/download/${cacheEntry.match.id}`, + urlSigningConfigFromEnv(), + )}` if (!env.ENABLE_DIRECT_DOWNLOADS || !this.adapter.createDownloadUrl || !location.mergedAt) return { - downloadUrl: defaultUrl, + downloadUrl: proxiedDownloadUrl(), cacheEntry: cacheEntry.match, } @@ -725,7 +732,7 @@ export class Storage { `${leased.folderName}/merged`, directDownloadExpiresAt, ) - : defaultUrl + : proxiedDownloadUrl() return { downloadUrl, diff --git a/lib/url-signing.ts b/lib/url-signing.ts new file mode 100644 index 00000000..1315d3aa --- /dev/null +++ b/lib/url-signing.ts @@ -0,0 +1,111 @@ +import type { H3Event } from 'h3' + +import { createHmac, timingSafeEqual } from 'node:crypto' + +import { createError, getQuery } from 'h3' +import { env } from '~/lib/env' + +/** + * Fixed lifetime of a signed URL. Not configurable: 1h keeps a leaked URL + * short-lived while covering any realistic upload. The upload signature is minted + * once at `CreateCacheEntry` for the whole multi-chunk upload. Clients do not + * re-fetch the upload URL on 401 so an upload exceeding 1h fails hard. + */ +export const URL_SIGNING_TTL_MS = 60 * 60 * 1000 // 1 hour + +/** + * Redact the `sig` bearer credential from a path/URL before it is logged. `sig` + * is replayable for the whole TTL, so a leaked log line must not carry it; the + * rest of the path and query (including `exp`) is left intact. Every code path + * that logs a request path MUST route through this. + */ +export function redactSignedPath(path: string): string { + return path.replace(/([?&]sig=)[^&]*/i, '$1[redacted]') +} + +export interface UrlSigningConfig { + enabled: boolean + /** The active signing secret. Also the first candidate on verify. */ + secret: string + /** Optional verify-only rotation secret (the previous `secret`). */ + secondary?: string +} + +/** + * The only reader of the `env` singleton. The core takes config explicitly so it + * can be unit-tested across enabled/disabled/rotation cases without env juggling. + */ +export function urlSigningConfigFromEnv(): UrlSigningConfig { + return { + enabled: env.URL_SIGNING_ENABLED, + secret: env.URL_SIGNING_SECRET ?? '', + secondary: env.URL_SIGNING_SECRET_SECONDARY || undefined, + } +} + +/** Ordered verify candidates: the active secret first, then the rotation secret. */ +function verifySecrets(config: UrlSigningConfig): string[] { + return [config.secret, config.secondary].filter((s): s is string => Boolean(s)) +} + +function computeSignature(canonicalPath: string, exp: number, secret: string): string { + return createHmac('sha256', secret).update(`${canonicalPath}\n${exp}`).digest('base64url') +} + +/** + * Build the `?exp=&sig=` suffix binding `canonicalPath + exp`. + * Returns `''` when disabled. Always signs with the active `config.secret`. + * + * `canonicalPath` MUST be an invariant resource id (e.g. `/upload/{id}`) built + * from the raw route param — never `event.path` — so the route alias and any + * reverse-proxy/base-path prefix all verify against the same signed material. + */ +export function signQuery(canonicalPath: string, config: UrlSigningConfig): string { + if (!config.enabled) return '' + + const exp = Date.now() + URL_SIGNING_TTL_MS + const sig = computeSignature(canonicalPath, exp, config.secret) + return `?exp=${exp}&sig=${sig}` +} + +/** + * Strictly verify the `exp`/`sig` query params against `canonicalPath`. No-op + * when disabled. Throws 401 for every failure (missing/malformed params, mismatch, + * expiry). Tries the active secret then the secondary. + * + * Excludes mutable query params (Azure `blockid`, `comp=blocklist`), so one + * `CreateCacheEntry`-issued signature validates every chunk PUT and the final + * blocklist PUT within the TTL. + */ +export function verifySignedRequest( + event: H3Event, + canonicalPath: string, + config: UrlSigningConfig, +): void { + if (!config.enabled) return + + const query = getQuery(event) + const exp = query.exp + const sig = query.sig + + // Validate before any HMAC/expiry work. + if ( + typeof exp !== 'string' || // reject array-valued exp (repeated query param) + typeof sig !== 'string' || // reject array-valued sig (repeated query param) + !/^[1-9]\d*$/.test(exp) || // digits only, no leading zeros so exp round-trips through Number() + sig.length === 0 // reject empty sig + ) + throw createError({ statusCode: 401 }) + + const expMs = Number(exp) + if (!Number.isSafeInteger(expMs) || expMs <= 0) throw createError({ statusCode: 401 }) + + const provided = Buffer.from(sig, 'base64url') + const matched = verifySecrets(config).some((secret) => { + const expected = Buffer.from(computeSignature(canonicalPath, expMs, secret), 'base64url') + return expected.length === provided.length && timingSafeEqual(expected, provided) + }) + if (!matched) throw createError({ statusCode: 401 }) + + if (Date.now() > expMs) throw createError({ statusCode: 401 }) +} diff --git a/plugins/setup.ts b/plugins/setup.ts index 783d73e2..a0f71daa 100644 --- a/plugins/setup.ts +++ b/plugins/setup.ts @@ -6,6 +6,9 @@ import { getDatabase } from '~/lib/db' import { env } from '~/lib/env' import { logger } from '~/lib/logger' import { getStorage } from '~/lib/storage' +import { redactSignedPath } from '~/lib/url-signing' + +const logPath = (event: { path: string }) => redactSignedPath(event.path) export default defineNitroPlugin(async (nitro) => { const version = useRuntimeConfig().version @@ -47,17 +50,17 @@ export default defineNitroPlugin(async (nitro) => { } logger.error( - `Response: ${event.method} ${event.path} > ${error instanceof H3Error ? error.statusCode : '[no status code]'}\n`, + `Response: ${event.method} ${logPath(event)} > ${error instanceof H3Error ? error.statusCode : '[no status code]'}\n`, error, ) }) if (env.DEBUG) { nitro.hooks.hook('request', (event) => { - logger.debug(`Request: ${event.method} ${event.path}`) + logger.debug(`Request: ${event.method} ${logPath(event)}`) }) nitro.hooks.hook('afterResponse', (event) => { - logger.debug(`Response: ${event.method} ${event.path} > ${getResponseStatus(event)}`) + logger.debug(`Response: ${event.method} ${logPath(event)} > ${getResponseStatus(event)}`) }) } diff --git a/routes/[...path].ts b/routes/[...path].ts index 18062329..4bddfb62 100644 --- a/routes/[...path].ts +++ b/routes/[...path].ts @@ -1,7 +1,13 @@ import { env } from '~/lib/env' import { logger } from '~/lib/logger' +import { redactSignedPath } from '~/lib/url-signing' export default defineEventHandler(async (event) => { - logger.debug('proxying unknown path', event.path, 'to', env.DEFAULT_ACTIONS_RESULTS_URL) + logger.debug( + 'proxying unknown path', + redactSignedPath(event.path), + 'to', + env.DEFAULT_ACTIONS_RESULTS_URL, + ) return proxyRequest(event, `${env.DEFAULT_ACTIONS_RESULTS_URL}${event.path}`) }) diff --git a/routes/devstoreaccount1/upload/[uploadId].put.ts b/routes/devstoreaccount1/upload/[uploadId].put.ts index 58b35018..18a39b6f 100644 --- a/routes/devstoreaccount1/upload/[uploadId].put.ts +++ b/routes/devstoreaccount1/upload/[uploadId].put.ts @@ -6,12 +6,17 @@ import { z } from 'zod' import { logger } from '~/lib/logger' import { getStorage } from '~/lib/storage' +import { urlSigningConfigFromEnv, verifySignedRequest } from '~/lib/url-signing' const pathParamsSchema = z.object({ uploadId: z.coerce.number(), }) export default defineEventHandler(async (event) => { + // Verify before the comp=blocklist short-circuit so finalization is protected too. + // Use the raw param (not the zod-coerced number) to match the signed canonical path. + verifySignedRequest(event, `/upload/${event.context.params?.uploadId}`, urlSigningConfigFromEnv()) + const parsedPathParams = pathParamsSchema.safeParse(event.context.params) if (!parsedPathParams.success) throw createError({ diff --git a/routes/download/[cacheEntryId].ts b/routes/download/[cacheEntryId].ts index a4bdf9af..28b776ab 100644 --- a/routes/download/[cacheEntryId].ts +++ b/routes/download/[cacheEntryId].ts @@ -2,6 +2,7 @@ import { Readable } from 'node:stream' import { z } from 'zod' import { logger } from '~/lib/logger' import { getStorage } from '~/lib/storage' +import { urlSigningConfigFromEnv, verifySignedRequest } from '~/lib/url-signing' const pathParamsSchema = z.object({ cacheEntryId: z.string(), @@ -17,6 +18,8 @@ export default defineEventHandler(async (event) => { const { cacheEntryId } = parsedPathParams.data + verifySignedRequest(event, `/download/${cacheEntryId}`, urlSigningConfigFromEnv()) + const storage = await getStorage() const stream = await storage.download(cacheEntryId) if (!stream) diff --git a/routes/twirp/github.actions.results.api.v1.CacheService/CreateCacheEntry.post.ts b/routes/twirp/github.actions.results.api.v1.CacheService/CreateCacheEntry.post.ts index f8b752d7..d9be4b7e 100644 --- a/routes/twirp/github.actions.results.api.v1.CacheService/CreateCacheEntry.post.ts +++ b/routes/twirp/github.actions.results.api.v1.CacheService/CreateCacheEntry.post.ts @@ -3,6 +3,7 @@ import { env } from '~/lib/env' import { getCacheScope } from '~/lib/scope' import { getStorage } from '~/lib/storage' import { readTwirpRequest, sendTwirpResponse, TwirpMessage } from '~/lib/twirp' +import { signQuery, urlSigningConfigFromEnv } from '~/lib/url-signing' const bodySchema = z.object({ key: z.string().min(1), @@ -28,7 +29,15 @@ export default defineEventHandler(async (event) => { return sendTwirpResponse( event, upload - ? { ok: true, signed_upload_url: `${env.API_BASE_URL}/devstoreaccount1/upload/${upload.id}` } + ? { + ok: true, + // Emitted path is /devstoreaccount1/upload/{id}; signed canonical path + // is the invariant /upload/{id} (see signQuery). + signed_upload_url: `${env.API_BASE_URL}/devstoreaccount1/upload/${upload.id}${signQuery( + `/upload/${upload.id}`, + urlSigningConfigFromEnv(), + )}`, + } : { ok: false }, TwirpMessage.CreateCacheEntryResponse, ) diff --git a/tests/setup.ts b/tests/setup.ts index 18c1712e..5c8e9594 100644 --- a/tests/setup.ts +++ b/tests/setup.ts @@ -44,6 +44,7 @@ const TESTING_ENV_BASE = { | 'BENCHMARK' | 'SKIP_TOKEN_VALIDATION' | 'ACTIONS_TOKEN_ISSUER' + | 'URL_SIGNING_ENABLED' > & Record diff --git a/tests/url-signing-e2e.test.ts b/tests/url-signing-e2e.test.ts new file mode 100644 index 00000000..942388b2 --- /dev/null +++ b/tests/url-signing-e2e.test.ts @@ -0,0 +1,118 @@ +/* eslint-disable unicorn/no-top-level-assignment-in-function */ +import type { ResultPromise } from 'execa' + +import crypto from 'node:crypto' +import fs from 'node:fs/promises' +import path from 'node:path' + +import { restoreCache, saveCache } from '@actions/cache' +import { execa } from 'execa' +import { SignJWT } from 'jose' +import { afterAll, beforeAll, describe, expect, test } from 'vitest' + +import { TEST_TEMP_DIR } from './setup' + +// A second server booted from the same build with URL signing on, on its own +// port (the shared harness server stays signing-off). The only place the enabled +// verify-on-handler path runs end-to-end against the real `@actions/cache` client. +const SIGNED_PORT = 3101 +const SIGNED_BASE_URL = `http://localhost:${SIGNED_PORT}` +const URL_SIGNING_SECRET = 'test-url-signing-secret-0123456789' + +const MB = 1024 * 1024 +const testFilePath = path.join(TEST_TEMP_DIR, 'url-signing-e2e.bin') + +let signedServer: ResultPromise<{ node: true; stdio: 'inherit' }> + +// @actions/cache reads these from the environment at call time. Point the client +// at the signed server for this file, then restore the originals so later suites +// keep hitting the harness server. +const savedClientEnv: Record = {} +function setClientEnv(values: Record) { + for (const [key, value] of Object.entries(values)) { + savedClientEnv[key] = process.env[key] + process.env[key] = value + } +} +function restoreClientEnv() { + for (const [key, value] of Object.entries(savedClientEnv)) { + if (value === undefined) delete process.env[key] + else process.env[key] = value + } +} + +describe('signed cache URLs end-to-end (URL_SIGNING_ENABLED=true)', () => { + beforeAll(async () => { + signedServer = execa({ + node: true, + stdio: 'inherit', + env: { + ...process.env, + PORT: String(SIGNED_PORT), + NITRO_PORT: String(SIGNED_PORT), + API_BASE_URL: SIGNED_BASE_URL, + URL_SIGNING_ENABLED: 'true', + URL_SIGNING_SECRET, + }, + })`.output/server/index.mjs` + signedServer.on('exit', (code) => { + if (code === 0 || code === null) return + console.error('Signed Nitro server exited with code', code) + }) + await new Promise((resolve, reject) => { + signedServer.on('error', reject) + signedServer.on('exit', (code) => + reject(new Error(`signed server exited before ready (code ${code})`)), + ) + signedServer.on('message', (message) => { + if (message === 'nitro:ready') resolve() + }) + }) + + setClientEnv({ + ACTIONS_RESULTS_URL: `${SIGNED_BASE_URL}/`, + ACTIONS_CACHE_URL: `${SIGNED_BASE_URL}/`, + ACTIONS_CACHE_SERVICE_V2: 'true', + ACTIONS_RUNTIME_TOKEN: await new SignJWT({ + ac: JSON.stringify([{ Scope: 'refs/heads/main', Permission: 3 }]), + repository_id: '123', + }) + .setProtectedHeader({ alg: 'HS256' }) + .sign(crypto.createSecretKey('mock-secret-key', 'ascii')), + }) + }, 60_000) + + afterAll(async () => { + restoreClientEnv() + await signedServer?.kill() + await fs.rm(testFilePath, { force: true }) + }) + + // 64MB forces a multi-block upload + blocklist commit, proving one signature + // covers every block PUT and the finalize (i.e. the Azure SDK keeps the + // `exp`/`sig` query params when it appends `blockid`/`comp`). + test('saves and restores through signed upload/download URLs', { timeout: 90_000 }, async () => { + const key = 'url-signing-e2e-key' + const expectedContents = crypto.randomBytes(64 * MB) + await fs.writeFile(testFilePath, expectedContents) + + await saveCache([testFilePath], key) + await fs.rm(testFilePath) + + const cacheHitKey = await restoreCache([testFilePath], key) + expect(cacheHitKey).toBe(key) + + const restoredContents = await fs.readFile(testFilePath) + expect(restoredContents.compare(expectedContents)).toBe(0) + }) + + // Enforcement is actually on the handlers (not just minted into URLs): an + // unsigned request to either proxied route is rejected with 401. + test('rejects unsigned requests to the proxied routes with 401', async () => { + const download = await fetch(`${SIGNED_BASE_URL}/download/does-not-matter`) + expect(download.status).toBe(401) + + const upload = await fetch(`${SIGNED_BASE_URL}/devstoreaccount1/upload/1`, { method: 'PUT' }) + expect(upload.status).toBe(401) + }) +}) diff --git a/tests/url-signing.test.ts b/tests/url-signing.test.ts new file mode 100644 index 00000000..0142678c --- /dev/null +++ b/tests/url-signing.test.ts @@ -0,0 +1,308 @@ +import type { H3Event } from 'h3' +import type { UrlSigningConfig } from '~/lib/url-signing' + +import { createHmac } from 'node:crypto' + +import arkenv from 'arkenv' +import { describe, expect, test } from 'vitest' +import { envSchema, envSchemaValidated } from '~/lib/schemas' +import { + redactSignedPath, + signQuery, + URL_SIGNING_TTL_MS, + verifySignedRequest, +} from '~/lib/url-signing' + +const SECRET_A = 'secret-aaaaaaaaaaaaaaaaaaaa' +const SECRET_B = 'secret-bbbbbbbbbbbbbbbbbbbb' + +function enabled(secret: string, secondary?: string): UrlSigningConfig { + return { enabled: true, secret, secondary } +} +const disabled: UrlSigningConfig = { enabled: false, secret: '' } + +/** Build a minimal H3Event — `getQuery(event)` only reads `event.path`. */ +function eventFrom(query: Record): H3Event { + const usp = new URLSearchParams() + for (const [key, value] of Object.entries(query)) { + if (Array.isArray(value)) for (const v of value) usp.append(key, v) + else usp.append(key, value) + } + const qs = usp.toString() + return { path: `/whatever${qs ? `?${qs}` : ''}` } as unknown as H3Event +} + +/** Mirror of the implementation's HMAC, so we can craft expired/tampered URLs. */ +function sign(canonicalPath: string, exp: number, secret: string) { + return createHmac('sha256', secret).update(`${canonicalPath}\n${exp}`).digest('base64url') +} + +/** Parse the `?exp=..&sig=..` suffix produced by `signQuery` back into a map. */ +function queryFromSuffix(suffix: string): Record { + return Object.fromEntries(new URLSearchParams(suffix.replace(/^\?/, ''))) +} + +function expect401(fn: () => void) { + try { + fn() + } catch (err) { + expect((err as { statusCode?: number }).statusCode).toBe(401) + return + } + throw new Error('expected verifySignedRequest to throw a 401') +} + +describe('signQuery', () => { + test('returns an empty string when disabled', () => { + expect(signQuery('/upload/1', disabled)).toBe('') + }) + + test('produces exp+sig bound to the canonical path, signed with the active secret', () => { + const before = Date.now() + const query = queryFromSuffix(signQuery('/upload/1', enabled(SECRET_A, SECRET_B))) + const exp = Number(query.exp) + + expect(exp).toBeGreaterThanOrEqual(before + URL_SIGNING_TTL_MS) + // Signs with the active secret, never the secondary. + expect(query.sig).toBe(sign('/upload/1', exp, SECRET_A)) + expect(query.sig).not.toBe(sign('/upload/1', exp, SECRET_B)) + }) +}) + +describe('redactSignedPath', () => { + test('redacts sig while leaving the rest of the path and query intact', () => { + expect(redactSignedPath('/download/abc?exp=123&sig=deadbeef')).toBe( + '/download/abc?exp=123&sig=[redacted]', + ) + // sig as the first param, followed by others. + expect(redactSignedPath('/upload/1?sig=deadbeef&comp=blocklist')).toBe( + '/upload/1?sig=[redacted]&comp=blocklist', + ) + }) + + test('is a no-op on paths without a sig param', () => { + expect(redactSignedPath('/download/abc?exp=123')).toBe('/download/abc?exp=123') + expect(redactSignedPath('/health')).toBe('/health') + }) +}) + +describe('verifySignedRequest', () => { + test('passes for a valid signature', () => { + const config = enabled(SECRET_A) + const query = queryFromSuffix(signQuery('/upload/1', config)) + expect(() => verifySignedRequest(eventFrom(query), '/upload/1', config)).not.toThrow() + }) + + test('rejects a tampered signature (401)', () => { + const config = enabled(SECRET_A) + const query = queryFromSuffix(signQuery('/upload/1', config)) + expect401(() => + verifySignedRequest(eventFrom({ ...query, sig: `${query.sig}x` }), '/upload/1', config), + ) + }) + + test('rejects a tampered path/id (401)', () => { + const config = enabled(SECRET_A) + const query = queryFromSuffix(signQuery('/upload/1', config)) + // Same signature, verified against a different canonical path. + expect401(() => verifySignedRequest(eventFrom(query), '/upload/2', config)) + expect401(() => verifySignedRequest(eventFrom(query), '/download/1', config)) + }) + + test('rejects a tampered exp — the signature no longer matches (401)', () => { + const config = enabled(SECRET_A) + const query = queryFromSuffix(signQuery('/upload/1', config)) + const bumped = String(Number(query.exp) + 1000) + expect401(() => verifySignedRequest(eventFrom({ ...query, exp: bumped }), '/upload/1', config)) + }) + + test('rejects an expired signature (401)', () => { + const config = enabled(SECRET_A) + const exp = Date.now() - 1000 + const sig = sign('/upload/1', exp, SECRET_A) + expect401(() => verifySignedRequest(eventFrom({ exp: String(exp), sig }), '/upload/1', config)) + }) + + test('rejects missing exp or sig (401)', () => { + const config = enabled(SECRET_A) + const query = queryFromSuffix(signQuery('/upload/1', config)) + expect401(() => verifySignedRequest(eventFrom({ sig: query.sig }), '/upload/1', config)) + expect401(() => verifySignedRequest(eventFrom({ exp: query.exp }), '/upload/1', config)) + expect401(() => verifySignedRequest(eventFrom({}), '/upload/1', config)) + }) + + test('rejects malformed inputs (401): array-valued params, non-integer exp, empty sig', () => { + const config = enabled(SECRET_A) + const query = queryFromSuffix(signQuery('/upload/1', config)) + + // Duplicated query params → array-valued exp/sig. + expect401(() => + verifySignedRequest( + eventFrom({ exp: [query.exp, query.exp], sig: query.sig }), + '/upload/1', + config, + ), + ) + expect401(() => + verifySignedRequest( + eventFrom({ exp: query.exp, sig: [query.sig, query.sig] }), + '/upload/1', + config, + ), + ) + // Non-integer / non-/^\d+$/ exp. + expect401(() => + verifySignedRequest(eventFrom({ exp: 'abc', sig: query.sig }), '/upload/1', config), + ) + expect401(() => + verifySignedRequest(eventFrom({ exp: '12.5', sig: query.sig }), '/upload/1', config), + ) + expect401(() => + verifySignedRequest(eventFrom({ exp: '-1', sig: query.sig }), '/upload/1', config), + ) + // Leading zeros are rejected: `Number('0')` would normalize away the + // prefix and no longer match the signed string. + expect401(() => + verifySignedRequest(eventFrom({ exp: `0${query.exp}`, sig: query.sig }), '/upload/1', config), + ) + // Empty sig. + expect401(() => + verifySignedRequest(eventFrom({ exp: query.exp, sig: '' }), '/upload/1', config), + ) + }) + + test('ignores mutable query params (blockid, comp=blocklist)', () => { + const config = enabled(SECRET_A) + const query = queryFromSuffix(signQuery('/upload/1', config)) + + expect(() => + verifySignedRequest(eventFrom({ ...query, blockid: 'AAAA' }), '/upload/1', config), + ).not.toThrow() + expect(() => + verifySignedRequest(eventFrom({ ...query, comp: 'blocklist' }), '/upload/1', config), + ).not.toThrow() + }) + + describe('secret rotation (secondary)', () => { + // Rotation state: SECRET_B is the new active, SECRET_A demoted to secondary. + test('accepts a URL signed with the active secret', () => { + const query = queryFromSuffix(signQuery('/upload/1', enabled(SECRET_B))) + expect(() => + verifySignedRequest(eventFrom(query), '/upload/1', enabled(SECRET_B, SECRET_A)), + ).not.toThrow() + }) + + test('accepts a URL signed with the previous (secondary) secret', () => { + const query = queryFromSuffix(signQuery('/upload/1', enabled(SECRET_A))) + expect(() => + verifySignedRequest(eventFrom(query), '/upload/1', enabled(SECRET_B, SECRET_A)), + ).not.toThrow() + }) + + test('rejects a signature from a since-removed secret (401)', () => { + const query = queryFromSuffix(signQuery('/upload/1', enabled(SECRET_B))) + // SECRET_B has been fully rotated out; only SECRET_A remains, no secondary. + expect401(() => verifySignedRequest(eventFrom(query), '/upload/1', enabled(SECRET_A))) + }) + }) + + describe('when disabled', () => { + test('is a no-op even for unsigned or garbage requests', () => { + expect(() => verifySignedRequest(eventFrom({}), '/upload/1', disabled)).not.toThrow() + expect(() => + verifySignedRequest(eventFrom({ exp: 'nope', sig: '' }), '/upload/1', disabled), + ).not.toThrow() + }) + }) +}) + +describe('envSchemaValidated cross-field validation', () => { + // Full intersection ⇒ each map must supply a complete valid storage + db env. + const BASE_ENV = { + API_BASE_URL: 'http://localhost:3000', + STORAGE_DRIVER: 'filesystem', + STORAGE_FILESYSTEM_PATH: '/tmp/storage', + DB_DRIVER: 'sqlite', + DB_SQLITE_PATH: '/tmp/test.sqlite', + } + // Mirror `lib/env.ts`: validate via arkenv, then `envSchema.assert` to apply the + // morphs arkenv drops on a narrowed root. Invalid cases throw at the arkenv step. + const validate = (override: Record) => + envSchema.assert(arkenv(envSchemaValidated, { env: { ...BASE_ENV, ...override } })) + + test('throws when enabled but URL_SIGNING_SECRET is missing', () => { + expect(() => validate({ URL_SIGNING_ENABLED: 'true' })).toThrow() + }) + + test('throws when enabled but URL_SIGNING_SECRET is shorter than 16 chars', () => { + expect(() => validate({ URL_SIGNING_ENABLED: 'true', URL_SIGNING_SECRET: 'short' })).toThrow() + }) + + test('throws when enabled with only URL_SIGNING_SECRET_SECONDARY set (no primary)', () => { + // A valid secondary cannot substitute for the missing active signer. + expect(() => + validate({ URL_SIGNING_ENABLED: 'true', URL_SIGNING_SECRET_SECONDARY: SECRET_B }), + ).toThrow() + }) + + test('throws when the secondary is set but shorter than 16 chars', () => { + expect(() => + validate({ + URL_SIGNING_ENABLED: 'true', + URL_SIGNING_SECRET: SECRET_A, + URL_SIGNING_SECRET_SECONDARY: 'short', + }), + ).toThrow() + }) + + test('passes when enabled with a valid primary (no secondary)', () => { + expect(() => + validate({ URL_SIGNING_ENABLED: 'true', URL_SIGNING_SECRET: SECRET_A }), + ).not.toThrow() + }) + + test('passes when enabled with a valid primary and secondary', () => { + expect(() => + validate({ + URL_SIGNING_ENABLED: 'true', + URL_SIGNING_SECRET: SECRET_A, + URL_SIGNING_SECRET_SECONDARY: SECRET_B, + }), + ).not.toThrow() + }) + + test('passes when explicitly disabled with no secret', () => { + expect(() => validate({ URL_SIGNING_ENABLED: 'false' })).not.toThrow() + }) + + test('passes when unset with no secret (default-before-narrow ordering guard)', () => { + // Pins the arkenv ordering assumption: the narrow observes the applied + // default `false`, not `undefined`. An arkenv upgrade that changes this + // fails here loudly instead of silently disabling the boot check. + const result = validate({}) + expect(result.URL_SIGNING_ENABLED).toBe(false) + }) + + test('applies object morphs (arkenv drops them on a narrowed root)', () => { + // Regression guard: switching `env` to the narrowed schema previously + // silently disabled every morph. The two-pass in `lib/env.ts` must restore + // them — here the trailing-slash strip on API_BASE_URL. + const result = validate({ API_BASE_URL: 'http://localhost:3000///' }) + expect(result.API_BASE_URL).toBe('http://localhost:3000') + }) + + test('trims secrets in the parsed output', () => { + // A valid secret with surrounding whitespace (e.g. a trailing newline from a + // mounted secret file) is accepted and stored trimmed. + const result = validate({ URL_SIGNING_ENABLED: 'true', URL_SIGNING_SECRET: ` ${SECRET_A}\n` }) + expect(result.URL_SIGNING_SECRET).toBe(SECRET_A) + }) + + test('rejects a whitespace-padded secret whose trimmed length is < 16', () => { + // The narrow must trim before the length check (it sees the raw value), or a + // padded-short secret would slip through. + expect(() => + validate({ URL_SIGNING_ENABLED: 'true', URL_SIGNING_SECRET: `${' '.repeat(20)}ab` }), + ).toThrow() + }) +}) From d0bacddf3f9f73e6fd8374309acade515c4226f2 Mon Sep 17 00:00:00 2001 From: Isaac Su Date: Fri, 14 Aug 2026 07:58:21 +1000 Subject: [PATCH 2/2] fix linting --- README.md | 10 +++++----- lib/url-signing.ts | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 91812d4a..a5ecefa7 100644 --- a/README.md +++ b/README.md @@ -34,11 +34,11 @@ The server-proxied upload and download URLs (`/devstoreaccount1/upload/{id}` and `/download/{id}`) are unauthenticated by default. You can enable an expiring HMAC signature on them: -| Env var | Description | -| - | - | -| `URL_SIGNING_ENABLED` | `boolean`, default `false`. When `true`, upload/download URLs are signed on generation and strictly verified on the handlers (no unsigned fallback). | -| `URL_SIGNING_SECRET` | The active signing secret, **≥ 16 chars**. Signs every issued URL and is the first candidate on verification. Required when signing is enabled — boot fails otherwise. | -| `URL_SIGNING_SECRET_SECONDARY` | Optional verify-only rotation secret, **≥ 16 chars when set**. Never signs; accepted on verification so URLs minted with the previous secret keep working. | +| Env var | Description | +| ------------------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `URL_SIGNING_ENABLED` | `boolean`, default `false`. When `true`, upload/download URLs are signed on generation and strictly verified on the handlers (no unsigned fallback). | +| `URL_SIGNING_SECRET` | The active signing secret, **≥ 16 chars**. Signs every issued URL and is the first candidate on verification. Required when signing is enabled — boot fails otherwise. | +| `URL_SIGNING_SECRET_SECONDARY` | Optional verify-only rotation secret, **≥ 16 chars when set**. Never signs; accepted on verification so URLs minted with the previous secret keep working. | Notes: diff --git a/lib/url-signing.ts b/lib/url-signing.ts index 1315d3aa..6840888b 100644 --- a/lib/url-signing.ts +++ b/lib/url-signing.ts @@ -93,7 +93,7 @@ export function verifySignedRequest( typeof exp !== 'string' || // reject array-valued exp (repeated query param) typeof sig !== 'string' || // reject array-valued sig (repeated query param) !/^[1-9]\d*$/.test(exp) || // digits only, no leading zeros so exp round-trips through Number() - sig.length === 0 // reject empty sig + sig.length === 0 // reject empty sig ) throw createError({ statusCode: 401 })