From 510ed4b57d8db88418d077991272d6da4cfec486 Mon Sep 17 00:00:00 2001 From: webadderall <131426131+webadderall@users.noreply.github.com> Date: Mon, 21 Sep 2026 08:50:23 +1000 Subject: [PATCH] fix(cloud): scope owner access and protect shared response data --- docs/authentication.md | 2 +- docs/cloud-sharing.md | 4 +- .../recordly-share/worker/.dev.vars.example | 3 + services/recordly-share/worker/.env.example | 3 + services/recordly-share/worker/README.md | 4 +- services/recordly-share/worker/src/index.js | 43 ++++++++----- .../recordly-share/worker/test/api.test.js | 60 ++++++++++++++++++- services/recordly-share/worker/wrangler.jsonc | 6 +- 8 files changed, 99 insertions(+), 26 deletions(-) diff --git a/docs/authentication.md b/docs/authentication.md index fde2b3d4..6d9d2d49 100644 --- a/docs/authentication.md +++ b/docs/authentication.md @@ -49,4 +49,4 @@ Restart `npm run dev` after creating `.env.local`. Open an editor and verify: 3. Google opens the system browser and returns to Recordly. 4. Clicking **Create link** while signed out opens this modal; after successful authentication it continues to the share dialog. -Add the same `SUPABASE_URL` and `SUPABASE_PUBLISHABLE_KEY` values to the share Worker's secrets or variables. The Worker validates the user's access token with Supabase before accepting an upload. `API_SECRET` is server-side only and remains available for library administration and explicitly enabled local integration tests; it is never entered into or exposed by the desktop app. +Add the same `SUPABASE_URL` and `SUPABASE_PUBLISHABLE_KEY` values to the share Worker's secrets or variables. Set `OWNER_USER_ID` to the owner’s Supabase user ID. The Worker validates the access token with Supabase and requires that owner identity before accepting API requests. `API_SECRET` is server-side only and remains available for library administration and explicitly enabled local integration tests; it is never entered into or exposed by the desktop app. diff --git a/docs/cloud-sharing.md b/docs/cloud-sharing.md index 4850f1a2..f8e5fc23 100644 --- a/docs/cloud-sharing.md +++ b/docs/cloud-sharing.md @@ -8,7 +8,7 @@ The desktop app defaults to the local development endpoint: http://localhost:8787/api/upload ``` -The endpoint is intentionally not user-configurable. Development builds use the local service above; production builds use `https://videos.recordly.dev/api/upload`. Publishing requires the user's Recordly access token. No share API secret is exposed in the app. +The endpoint is intentionally not user-configurable. All builds currently use the local service above. Production service integration is planned but is not selected by any build. Publishing requires the user's Recordly access token. No share API secret is exposed in the app. ## Publishing protocol @@ -26,3 +26,5 @@ Anyone with a valid link can watch a public recording and leave timestamped feed ## Third-party licensing The hosting service is based on MIT-licensed open-source software. Required attribution and the complete license text are preserved in [THIRD_PARTY_NOTICES.md](../THIRD_PARTY_NOTICES.md) and [the vendored license](../services/recordly-share/LICENSE). + +The self-hosted worker is a single-owner library. Set `OWNER_USER_ID` to the owner’s Supabase user ID. Other accounts in the same Supabase project cannot administer the library. Missing owner configuration disables Supabase bearer access. diff --git a/services/recordly-share/worker/.dev.vars.example b/services/recordly-share/worker/.dev.vars.example index daacbfe7..72cfc85a 100644 --- a/services/recordly-share/worker/.dev.vars.example +++ b/services/recordly-share/worker/.dev.vars.example @@ -12,3 +12,6 @@ ALLOW_API_SECRET_UPLOADS=false # signed-in users can publish without seeing an API-secret field. SUPABASE_URL=https://YOUR_PROJECT_REF.supabase.co SUPABASE_PUBLISHABLE_KEY=sb_publishable_YOUR_KEY + +# Only this Supabase user may administer this single-owner deployment. +OWNER_USER_ID= diff --git a/services/recordly-share/worker/.env.example b/services/recordly-share/worker/.env.example index 0c8fe37a..941fbcd2 100644 --- a/services/recordly-share/worker/.env.example +++ b/services/recordly-share/worker/.env.example @@ -9,3 +9,6 @@ API_SECRET= SUPABASE_URL= SUPABASE_PUBLISHABLE_KEY= ALLOW_API_SECRET_UPLOADS=false + +# Only this Supabase user may administer this single-owner deployment. +OWNER_USER_ID= diff --git a/services/recordly-share/worker/README.md b/services/recordly-share/worker/README.md index c41a0c64..a95b2226 100644 --- a/services/recordly-share/worker/README.md +++ b/services/recordly-share/worker/README.md @@ -12,7 +12,7 @@ npm --prefix web run build npm run dev ``` -Set `SUPABASE_URL` and `SUPABASE_PUBLISHABLE_KEY` in `.dev.vars` to the same public project configuration used by the desktop app. Recordly sends the signed-in user's access token when it publishes to: +Set `OWNER_USER_ID` to the deployment owner’s Supabase user ID. Only that user may administer this single-owner library. Set `SUPABASE_URL` and `SUPABASE_PUBLISHABLE_KEY` in `.dev.vars` to the same public project configuration used by the desktop app. Recordly sends the signed-in user's access token when it publishes to: - Endpoint: `http://localhost:8787/api/upload` @@ -31,7 +31,7 @@ npx wrangler secret put SUPABASE_PUBLISHABLE_KEY npm run deploy ``` -Set `SUPABASE_URL` as a Worker variable. The ID-less configuration provisions `recordly-share-db` and `recordly-videos` for a new deployment. Add the `videos.recordly.dev` custom domain; production Recordly builds accept only that publishing origin. +Set `SUPABASE_URL` as a Worker variable. The ID-less configuration provisions `recordly-share-db` and `recordly-videos` for a new deployment. Desktop builds currently upload only to localhost; production service integration is deferred. ## Attribution diff --git a/services/recordly-share/worker/src/index.js b/services/recordly-share/worker/src/index.js index 75d53c7d..941c4b23 100644 --- a/services/recordly-share/worker/src/index.js +++ b/services/recordly-share/worker/src/index.js @@ -375,7 +375,7 @@ async function isAuthorized(request, env) { env.API_SECRET && timingSafeEqual(token, env.API_SECRET) ) return true; - if (!env.SUPABASE_URL || !env.SUPABASE_PUBLISHABLE_KEY) return false; + if (!env.SUPABASE_URL || !env.SUPABASE_PUBLISHABLE_KEY || !env.OWNER_USER_ID) return false; try { const authBase = new URL(env.SUPABASE_URL); const isLocal = authBase.hostname === 'localhost' || authBase.hostname === '127.0.0.1'; @@ -387,7 +387,9 @@ async function isAuthorized(request, env) { apikey: env.SUPABASE_PUBLISHABLE_KEY, }, }); - return response.ok; + if (!response.ok) return false; + const user = await response.json(); + return typeof user.id === 'string' && timingSafeEqual(user.id, env.OWNER_USER_ID); } catch { return false; } @@ -448,7 +450,10 @@ async function expectedSessionToken(env) { } async function isDashboardAuthed(request, env) { - if (await isAuthorized(request, env)) return true; + return (await isAuthorized(request, env)) || dashboardCookieAuthed(request, env); +} + +async function dashboardCookieAuthed(request, env) { const cookies = parseCookies(request.headers.get('Cookie') || ''); const sessionToken = cookies['voom_session']; if (!sessionToken) return false; @@ -611,8 +616,7 @@ async function handleRequest(request, env) { }); } - const cookieOk = await isDashboardAuthed(request, env); - if (!(await isAuthorized(request, env)) && !cookieOk) { + if (!(await isAuthorized(request, env)) && !(await dashboardCookieAuthed(request, env))) { return errorResponse('Unauthorized', 401); } @@ -807,6 +811,11 @@ async function handleRequest(request, env) { // --- API Handlers --- +function finiteNonnegative(value) { + const number = Number(value); + return Number.isFinite(number) && number >= 0 ? number : 0; +} + async function handleUpload(request, env) { const body = await request.json(); const { title, duration, width, height, hasWebcam, fileSize, password_hash, cta_url, cta_text } = body; @@ -836,7 +845,7 @@ async function handleUpload(request, env) { `INSERT INTO videos (share_code, title, duration, width, height, has_webcam, file_size, expires_at, password_hash, password_salt, cta_url, cta_text) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)` ) - .bind(shareCode, title, duration || 0, width || 0, height || 0, hasWebcam ? 1 : 0, fileSize || 0, expiresAt, storedHash, salt, cta_url || null, cta_text || null) + .bind(shareCode, title, finiteNonnegative(duration), finiteNonnegative(width), finiteNonnegative(height), hasWebcam ? 1 : 0, finiteNonnegative(fileSize), expiresAt, storedHash, salt, cta_url || null, cta_text || null) .run(); const baseUrl = new URL(request.url).origin; @@ -1086,7 +1095,7 @@ async function handleVideoStream(request, env, shareCode) { 'Content-Range': `bytes ${start}-${actualEnd}/${totalSize}`, 'Content-Length': String(actualEnd - start + 1), 'Accept-Ranges': 'bytes', - 'Cache-Control': 'public, max-age=3600', + 'Cache-Control': video.password_hash ? 'private, no-store' : 'public, max-age=3600', 'Access-Control-Allow-Origin': '*', 'Access-Control-Expose-Headers': 'Content-Range, Content-Length, Accept-Ranges', }, @@ -1103,7 +1112,7 @@ async function handleVideoStream(request, env, shareCode) { 'Content-Type': 'video/mp4', 'Content-Length': String(object.size), 'Accept-Ranges': 'bytes', - 'Cache-Control': 'public, max-age=3600', + 'Cache-Control': video.password_hash ? 'private, no-store' : 'public, max-age=3600', 'Access-Control-Allow-Origin': '*', 'Access-Control-Expose-Headers': 'Content-Range, Content-Length, Accept-Ranges', }, @@ -1140,7 +1149,7 @@ async function handleVTT(request, env, shareCode) { return new Response(vtt, { headers: { 'Content-Type': 'text/vtt; charset=utf-8', - 'Cache-Control': 'public, max-age=3600', + 'Cache-Control': video.password_hash ? 'private, no-store' : 'public, max-age=3600', 'Access-Control-Allow-Origin': '*', }, }); @@ -1249,8 +1258,8 @@ async function handleOGPage(request, env, shareCode) { - - + + @@ -1262,8 +1271,8 @@ async function handleOGPage(request, env, shareCode) { - - + +

${escapeHTML(video.title)}

@@ -1459,8 +1468,10 @@ async function handleGetComments(request, env, shareCode) { if (!authed) return errorResponse('Password required', 401); const url = new URL(request.url); - const page = Math.max(1, parseInt(url.searchParams.get('page') || '1', 10)); - const limit = Math.min(parseInt(url.searchParams.get('limit') || '50', 10), 100); + const requestedPage = parseInt(url.searchParams.get('page') || '1', 10); + const requestedLimit = parseInt(url.searchParams.get('limit') || '50', 10); + const page = Number.isSafeInteger(requestedPage) ? Math.max(1, Math.min(requestedPage, Math.floor(Number.MAX_SAFE_INTEGER / 100))) : 1; + const limit = Number.isFinite(requestedLimit) ? Math.max(1, Math.min(requestedLimit, 100)) : 50; const offset = (page - 1) * limit; const total = await env.DB.prepare( @@ -1545,7 +1556,7 @@ async function handleOGImage(env, shareCode) { const date = formatDate(video.created_at); const rawTitle = locked ? 'Protected video' : video.title; const title = rawTitle.length > 60 ? rawTitle.substring(0, 57) + '...' : rawTitle; - const res = !locked && video.width > 0 ? `${video.width}\u00d7${video.height}` : ''; + const res = !locked && video.width > 0 ? `${finiteNonnegative(video.width)}\u00d7${finiteNonnegative(video.height)}` : ''; const svg = ` diff --git a/services/recordly-share/worker/test/api.test.js b/services/recordly-share/worker/test/api.test.js index 7591ac93..aee4d6ad 100644 --- a/services/recordly-share/worker/test/api.test.js +++ b/services/recordly-share/worker/test/api.test.js @@ -1,6 +1,6 @@ -import { describe, it, expect, beforeAll } from 'vitest'; +import { describe, it, expect, beforeAll, vi } from 'vitest'; import { env, SELF } from 'cloudflare:test'; -import { sha256Hex } from '../src/index.js'; +import worker, { sha256Hex } from '../src/index.js'; const AUTH = { Authorization: 'Bearer test-secret' }; const BASE = 'https://share.test'; @@ -448,3 +448,59 @@ describe('error middleware', () => { expect((await res.json()).error).toBe('Internal error'); }); }); + + +describe('review security fixes', () => { + it('allows only the configured Supabase owner and performs one auth lookup', async () => { + const config = { ...env, ALLOW_API_SECRET_UPLOADS: 'false', SUPABASE_URL: 'https://auth.example.test', SUPABASE_PUBLISHABLE_KEY: 'public-test', OWNER_USER_ID: 'owner' }; + const request = () => new Request(`${BASE}/api/health`, { headers: { Authorization: 'Bearer user-token' } }); + const lookup = vi.spyOn(globalThis, 'fetch'); + try { + lookup.mockResolvedValue(new Response(JSON.stringify({ id: 'other-user' }))); + expect((await worker.fetch(request(), config, {})).status).toBe(401); + lookup.mockClear().mockResolvedValue(new Response(JSON.stringify({ id: 'owner' }))); + expect((await worker.fetch(request(), config, {})).status).toBe(200); + expect(lookup).toHaveBeenCalledTimes(1); + lookup.mockClear(); + expect((await worker.fetch(request(), { ...config, OWNER_USER_ID: '' }, {})).status).toBe(401); + expect(lookup).not.toHaveBeenCalled(); + } finally { lookup.mockRestore(); } + }); + + it('never publicly caches protected video or transcripts', async () => { + const password = 'cache-test'; + const { shareCode } = await createShare({ password_hash: await sha256Hex(password) }); + await completeUpload(shareCode); + const unlocked = await SELF.fetch(`${BASE}/s/${shareCode}/verify-password`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ password }) }); + const Cookie = unlocked.headers.get('Set-Cookie').split(';')[0]; + for (const [url, extra] of [[`/v/${shareCode}`, {}], [`/v/${shareCode}`, { Range: 'bytes=0-3' }], [`/vtt/${shareCode}`, {}]]) { + const response = await SELF.fetch(`${BASE}${url}`, { headers: { Cookie, ...extra } }); + expect([200, 206]).toContain(response.status); + expect(response.headers.get('Cache-Control')).toBe('private, no-store'); + await response.arrayBuffer(); + } + }); + + it('coerces untrusted dimensions at upload and when rendering older rows', async () => { + const payload = '\">'; + const { shareCode } = await createShare({ width: payload, height: payload, duration: 'invalid', fileSize: -1 }); + const row = await env.DB.prepare('SELECT width, height, duration, file_size FROM videos WHERE share_code = ?').bind(shareCode).first(); + expect(row).toMatchObject({ width: 0, height: 0, duration: 0, file_size: 0 }); + await completeUpload(shareCode); + await env.DB.prepare('UPDATE videos SET width = ?, height = ? WHERE share_code = ?').bind(payload, payload, shareCode).run(); + const response = await SELF.fetch(`${BASE}/s/${shareCode}`, { headers: { 'User-Agent': 'Twitterbot/1.0' } }); + expect(await response.text()).not.toContain(''); + }); +}); + + +it('normalizes invalid comment pagination and clamps zero limits', async () => { + const { shareCode } = await createShare(); + await completeUpload(shareCode); + for (const query of ['page=invalid&limit=invalid', 'page=-5&limit=0', 'page=1&limit=-1']) { + const response = await SELF.fetch(`${BASE}/s/${shareCode}/comments?${query}`); + expect(response.status).toBe(200); + const body = await response.json(); + expect(body.comments).toEqual([]); + } +}); diff --git a/services/recordly-share/worker/wrangler.jsonc b/services/recordly-share/worker/wrangler.jsonc index 51df634a..3cc1935b 100644 --- a/services/recordly-share/worker/wrangler.jsonc +++ b/services/recordly-share/worker/wrangler.jsonc @@ -4,10 +4,8 @@ // database and R2 bucket in the DEPLOYER's account. The worker creates its own // schema at runtime, so no migrations need to run. // - // wrangler picks JSON config over wrangler.toml, so a bare `wrangler deploy` - // here uses THIS file. The maintainer's own worker (with real resource IDs) - // lives in wrangler.toml — always deploy it with `npm run deploy` - // (== `wrangler deploy --config wrangler.toml`), never a bare `wrangler deploy`. + // npm run deploy uses this wrangler.jsonc configuration. + // Supply a separate explicit --config path for any deployment-specific configuration. // "name": "recordly-share", "main": "src/index.js",