fix(documents): read attachments with service client so colleague uploads open (#1207)

The documents bucket SELECT policy only covers the uploader's own folder
(documents/{uid}/...), but document_attachments rows are company-scoped.
Every surface that touched storage with the user-bound client therefore
failed for attachments uploaded by another member of the same company
(colleague uploads, email-inbox ingest attributed to the company creator):

- GET /api/documents/:id 500ed with "Failed to create download URL", so
  viewing a bilaga on a verifikat or supplier invoice was broken for
  every member except the uploader (support case: Odin Aero, where all
  40 documents live in the owner's folder and the second member could
  open none of them).
- GET /api/documents/:id/integrity 500ed the same way.
- POST /api/documents/:id/verify failed the storage download.
- invoice-inbox retry-extraction could not download the attachment.
- cloud-backup user-triggered syncs silently dropped colleague-uploaded
  documents from the Drive archive (manifest rows flipped to 'error').

Fix: authorize on the user client (RLS + explicit company filter, plus
the membership check where present), then do the storage read with the
service-role client. This is the pattern the inline proxy route and the
v1 download route already use; these five call sites were left behind.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

DECISIONS.md

@@ -384,3 +384,4 @@ One line per decision: `[YYYY-MM-DD] <decision>: <why>`. Appended by agents and
 [2026-07-25] Declined the review suggestion to 200-ack the Resend delivery webhook when RESEND_DELIVERY_WEBHOOK_SECRET is unset; kept 503. The endpoint is only ever called because an operator pointed Resend at it, so a missing secret at that moment is a live misconfiguration: Svix retry then endpoint-disable is a visible signal, whereas a silent 200 loses every delivery outcome with only a log line. The "optional" wording in docs/WHITELABEL.md describes not wiring the webhook at all, not wiring it half way.
 [2026-07-25] Reverted the settings panel-sheet redesign on bug/resend-and-invoices back to main: Emil prefers the settings UI as it stands on main. The routed sheet, the sheet/ primitives (SettingsMasterDetail, SettingsAccordion, SettingsFieldRow), the *Subsections.tsx decompositions, the cold-load sheet and the settings_sheet i18n namespace were removed; every app/(dashboard)/settings/* page, components/settings/** file and MainContainer scroll exception now matches origin/main byte for byte. Unrelated branch work (invoice delivery outcomes, Stripe feed-only, article currency/deactivation, PDF logo) is untouched.
 [2026-07-25] Settings UI on bug/resend-and-invoices now comes from feat/settings-fonster-redesign (dbae8792, Jakob) instead of the panel-sheet work reverted earlier the same day: Emil chose the Fonster concept (flat hairline rows, help behind "?", sticky dirty-only save bar, 920x680 modal, switches instead of checkboxes). Applied as a patch rather than a merge because the redesign branch forks from b5e3c476 and merging would have dragged that older main in; every file applied cleanly since no settings file changed on main since that fork point. The 10 settings_payments keys the redesign still carries (needs_review_*, reason_*, sync_done_description/transactions) were deliberately NOT restored: the Stripe feed-only commit on this branch deleted both them and their call sites.
+[2026-07-26] Fixed cross-user attachment access (Odin Aero support case) at the call sites with service-role clients after company-scoped authorization, instead of rewriting the documents bucket storage policy to be company-scoped like sie-files got in 20260416120000: the documents path layout (documents/{userId}/...) carries no company_id, so a company-scoped policy needs a per-object join against document_attachments on every storage op, and the authorize-then-service-client pattern was already the established model (inline proxy route, v1 download route, MCP tools). Sweep found and fixed the same defect in the metadata/sign route, the integrity probe, verifyIntegrity, invoice-inbox retry-extraction, and cloud-backup archive generation. Known leftover, deliberately unfixed: deleteDocument and the upload-failure cleanups call storage remove() with a user-bound client, which silently no-ops (no DELETE policy, WORM), orphaning storage objects; harmless for compliance, needs a separate decision on whether files should ever be hard-deleted.

app/api/documents/[id]/__tests__/route.test.ts

@@ -27,6 +27,17 @@ vi.mock('@/lib/init', () => ({
   ensureInitialized: vi.fn(),
 }))
 
+// The GET route signs with the service-role client: the storage SELECT
+// policy only covers the uploader's own folder, so a company member viewing
+// a colleague's upload cannot sign with their own client.
+const createSignedUrlMock = vi.fn()
+const serviceStorageFromMock = vi.fn(() => ({ createSignedUrl: createSignedUrlMock }))
+vi.mock('@/lib/supabase/server', () => ({
+  createServiceClient: () => ({
+    storage: { from: serviceStorageFromMock },
+  }),
+}))
+
 import { GET, DELETE } from '../route'
 import { requireWritePermission } from '@/lib/auth/require-write'
 import { NextResponse } from 'next/server'
@@ -40,6 +51,10 @@ beforeEach(() => {
   requireAuthMock.mockResolvedValue({ user: mockUser, supabase: mockSupabase, error: null })
   // Reset write-permission mock to default ok
   vi.mocked(requireWritePermission).mockResolvedValue({ ok: true })
+  createSignedUrlMock.mockResolvedValue({
+    data: { signedUrl: 'https://example.com/signed' },
+    error: null,
+  })
 })
 
 function makeReq(method: 'GET' | 'DELETE' = 'DELETE') {
@@ -69,9 +84,7 @@ describe('GET /api/documents/[id]', () => {
 
   it('returns 500 when the signed URL cannot be created', async () => {
     enqueue({ data: makeDocumentAttachment({ id: 'doc-1' }), error: null })
-    mockSupabase.storage.from.mockReturnValueOnce({
-      createSignedUrl: vi.fn().mockResolvedValue({ data: null, error: { message: 'boom' } }),
-    } as never)
+    createSignedUrlMock.mockResolvedValue({ data: null, error: { message: 'boom' } })
 
     const res = await GET(makeReq('GET'), createMockRouteParams({ id: 'doc-1' }))
     const { status, body } = await parseJsonResponse<{ error: string }>(res)
@@ -100,9 +113,8 @@ describe('GET /api/documents/[id]', () => {
     expect(body.data.id).toBe('doc-1')
     expect(body.data.download_url).toBe('https://example.com/signed')
 
-    expect(mockSupabase.storage.from).toHaveBeenCalledWith('documents')
-    const storageBucket = mockSupabase.storage.from.mock.results[0]?.value
-    expect(storageBucket.createSignedUrl).toHaveBeenCalledWith('documents/user-1/kvitto.pdf', 3600)
+    expect(serviceStorageFromMock).toHaveBeenCalledWith('documents')
+    expect(createSignedUrlMock).toHaveBeenCalledWith('documents/user-1/kvitto.pdf', 3600)
 
     expect(handler).toHaveBeenCalledOnce()
     expect(handler).toHaveBeenCalledWith(
@@ -113,6 +125,33 @@ describe('GET /api/documents/[id]', () => {
       }),
     )
   })
+
+  it('signs attachments stored under another company member folder', async () => {
+    // Regression: the storage SELECT policy is per-uploader-folder, so signing
+    // with the user-bound client failed for every colleague-uploaded document
+    // ("Failed to create download URL"). The service client must sign after
+    // the company-scoped row fetch has authorized access.
+    const row = makeDocumentAttachment({
+      id: 'doc-2',
+      file_name: 'leverantorsfaktura.pdf',
+      storage_path: 'documents/other-member/leverantorsfaktura.pdf',
+    })
+    enqueue({ data: row, error: null })
+
+    const res = await GET(makeReq('GET'), createMockRouteParams({ id: 'doc-2' }))
+    const { status, body } = await parseJsonResponse<{
+      data: { download_url: string }
+    }>(res)
+
+    expect(status).toBe(200)
+    expect(body.data.download_url).toBe('https://example.com/signed')
+    expect(createSignedUrlMock).toHaveBeenCalledWith(
+      'documents/other-member/leverantorsfaktura.pdf',
+      3600,
+    )
+    // The user-bound client must not be used for signing at all.
+    expect(mockSupabase.storage.from).not.toHaveBeenCalled()
+  })
 })
 
 describe('DELETE /api/documents/[id]', () => {

app/api/documents/[id]/integrity/__tests__/route.test.ts

@@ -11,8 +11,14 @@ vi.mock('@/lib/auth/require-auth', () => ({
   requireAuth: vi.fn(),
 }))
 
+// The storage download goes through the service-role client: the storage
+// SELECT policy only covers the uploader's own folder, so the user-scoped
+// client cannot read colleague-uploaded files within the same company.
+const serviceDownloadMock = vi.fn()
+const serviceStorageFromMock = vi.fn(() => ({ download: serviceDownloadMock }))
 vi.mock('@/lib/supabase/server', () => ({
   createClient: () => Promise.resolve(mockSupabase),
+  createServiceClient: () => ({ storage: { from: serviceStorageFromMock } }),
 }))
 
 vi.mock('@/lib/core/documents/document-service', () => ({
@@ -106,7 +112,7 @@ describe('GET /api/documents/[id]/integrity', () => {
     const { status, body } = await parseJsonResponse<{ data: { valid: boolean } }>(res)
     expect(status).toBe(200)
     expect(body.data.valid).toBe(true)
-    expect(mockSupabase.storage.from).not.toHaveBeenCalled()
+    expect(serviceStorageFromMock).not.toHaveBeenCalled()
   })
 
   it('returns { valid: true } when the bytes match the declared mime type', async () => {
@@ -123,9 +129,7 @@ describe('GET /api/documents/[id]/integrity', () => {
 
     const pdfBytes = new Uint8Array([0x25, 0x50, 0x44, 0x46, 0x2d, 0x31, 0x2e, 0x37])
     const blob = new Blob([pdfBytes], { type: 'application/pdf' })
-    mockSupabase.storage.from.mockReturnValue({
-      download: vi.fn().mockResolvedValue({ data: blob, error: null }),
-    } as never)
+    serviceDownloadMock.mockResolvedValue({ data: blob, error: null })
 
     vi.mocked(validateDocumentMagicBytes).mockReturnValue(null)
 
@@ -149,9 +153,7 @@ describe('GET /api/documents/[id]/integrity', () => {
     enqueue({ data: { company_id: companyId }, error: null })
 
     const blob = new Blob([new Uint8Array([0x00, 0x00, 0x00, 0x00])])
-    mockSupabase.storage.from.mockReturnValue({
-      download: vi.fn().mockResolvedValue({ data: blob, error: null }),
-    } as never)
+    serviceDownloadMock.mockResolvedValue({ data: blob, error: null })
 
     vi.mocked(validateDocumentMagicBytes).mockReturnValue('Internal /storage/v1/object error 42')
 
@@ -177,12 +179,10 @@ describe('GET /api/documents/[id]/integrity', () => {
     })
     enqueue({ data: { company_id: companyId }, error: null })
 
-    mockSupabase.storage.from.mockReturnValue({
-      download: vi.fn().mockResolvedValue({
-        data: null,
-        error: { message: 'storage internal: bucket=documents object=secret/path.pdf' },
-      }),
-    } as never)
+    serviceDownloadMock.mockResolvedValue({
+      data: null,
+      error: { message: 'storage internal: bucket=documents object=secret/path.pdf' },
+    })
 
     const res = await GET(req(), createMockRouteParams({ id: validDocId }))
     const { status, body } = await parseJsonResponse<{ error: string }>(res)
@@ -191,17 +191,22 @@ describe('GET /api/documents/[id]/integrity', () => {
     expect(JSON.stringify(body)).not.toContain('secret/path.pdf')
   })
 
-  it('does not import the service-role supabase client', async () => {
-    // The download must go through the per-request user-scoped client so
-    // RLS on storage.objects can act as defense-in-depth. Statically
-    // verifying the source is the cleanest check: runtime mocking of the
-    // service-client export would not catch a future regression where
-    // someone added an import but conditionally used it.
+  it('authorizes on the user-scoped client before the service-role download', async () => {
+    // The storage SELECT policy on the documents bucket only covers the
+    // uploader's own folder, so the download must use the service-role
+    // client or colleague-uploaded files 500 for every other company
+    // member. The compensating control is that both the row fetch and the
+    // explicit membership check run on the user-scoped client first.
+    // Source-level check: the queued mock collapses chained calls, so
+    // runtime introspection cannot distinguish which client ran the fetch.
     const fs = await import('node:fs/promises')
     const path = await import('node:path')
     const routePath = path.resolve(__dirname, '../route.ts')
     const source = await fs.readFile(routePath, 'utf8')
-    expect(source).not.toMatch(/createServiceClient(\b|NoCookies)/)
+    const membershipIdx = source.indexOf("from('company_members')")
+    const downloadIdx = source.indexOf('createServiceClient()')
+    expect(membershipIdx).toBeGreaterThan(-1)
+    expect(downloadIdx).toBeGreaterThan(membershipIdx)
   })
 
   it('filters the document lookup to the current version', async () => {

app/api/documents/[id]/integrity/route.ts

@@ -1,6 +1,7 @@
 import { NextResponse } from 'next/server'
 import { z } from 'zod'
 import { requireAuth } from '@/lib/auth/require-auth'
+import { createServiceClient } from '@/lib/supabase/server'
 import { validateDocumentMagicBytes } from '@/lib/core/documents/document-service'
 import { createLogger } from '@/lib/logger'
 
@@ -74,11 +75,14 @@ export async function GET(
     return NextResponse.json({ data: { valid: true } })
   }
 
-  // Use the user-scoped supabase client so the storage download is subject
-  // to RLS on storage.objects, not just the application-layer membership
-  // check above. A logic bug in the membership check would still be
-  // arrested at the storage layer.
-  const { data: blob, error: downloadError } = await supabase.storage
+  // Download via the service-role client: the storage SELECT policy only
+  // covers the uploader's own folder (documents/{uid}/...), so the
+  // user-scoped client cannot read colleague-uploaded files even within
+  // the same company. The document_attachments RLS fetch plus the explicit
+  // membership check above are the authorization (same model as the
+  // inline proxy route).
+  const serviceClient = createServiceClient()
+  const { data: blob, error: downloadError } = await serviceClient.storage
     .from('documents')
     .download(doc.storage_path)
 

app/api/documents/[id]/route.ts

@@ -1,6 +1,7 @@
 import { NextResponse } from 'next/server'
 import { ensureInitialized } from '@/lib/init'
 import { withRouteContext } from '@/lib/api/with-route-context'
+import { createServiceClient } from '@/lib/supabase/server'
 import { deleteDocument } from '@/lib/core/documents/document-service'
 import { eventBus } from '@/lib/events'
 import { getErrorMessage as getUserErrorMessage } from '@/lib/errors/get-error-message'
@@ -34,8 +35,17 @@ export const GET = withRouteContext<{ params: Promise<{ id: string }> }>(
     // not race Vercel function suspension) and never rejects (the bus
     // settles handlers via Promise.allSettled), so it cannot fail this
     // Promise.all.
+    //
+    // Sign with the service-role client: the storage SELECT policy only
+    // covers the uploader's own folder (documents/{uid}/...), while
+    // document_attachments rows are company-scoped. Signing with the
+    // user-bound client fails for every attachment uploaded by another
+    // member of the same company. The row fetch above (RLS + explicit
+    // company filter) is the authorization, mirroring the inline proxy
+    // route.
+    const serviceClient = createServiceClient()
     const [signResult] = await Promise.all([
-      supabase.storage.from('documents').createSignedUrl(doc.storage_path, 3600),
+      serviceClient.storage.from('documents').createSignedUrl(doc.storage_path, 3600),
       eventBus.emit({
         type: 'document.accessed',
         payload: {

extensions/general/cloud-backup/lib/__tests__/sync.test.ts

@@ -12,6 +12,14 @@ vi.mock('@/lib/reports/archive-readme', () => ({
   buildDriveFolderReadme: vi.fn().mockReturnValue('README TEXT'),
 }))
 
+// performSync builds archives with a service-role client (the documents
+// bucket SELECT policy is per-uploader-folder, so a user-bound client would
+// drop colleague-uploaded files from the backup). The archive generators are
+// mocked above, so a bare stub is enough here.
+vi.mock('@/lib/supabase/server', () => ({
+  createServiceClient: vi.fn(() => ({ storage: { from: vi.fn() } })),
+}))
+
 vi.mock('@/lib/branding/service', () => ({
   getBranding: () => ({ appName: 'Accounted', appUrl: 'https://app.test' }),
 }))

extensions/general/cloud-backup/lib/sync.ts

@@ -7,6 +7,7 @@ import {
 } from '@/lib/reports/full-archive-export'
 import { buildDriveFolderReadme } from '@/lib/reports/archive-readme'
 import { getBranding } from '@/lib/branding/service'
+import { createServiceClient } from '@/lib/supabase/server'
 import { fetchAllRows } from '@/lib/supabase/fetch-all'
 import {
   getOAuthEnv,
@@ -126,6 +127,15 @@ interface PeriodRow {
 export async function performSync(params: PerformSyncParams): Promise<PerformSyncResult> {
   const { supabase, companyId, userId, origin } = params
 
+  // Archive generation reads the private `documents` bucket, whose SELECT
+  // policy only covers the uploader's own folder. User-triggered syncs pass
+  // a user-bound client, which would silently drop every colleague-uploaded
+  // document from the backup (manifest rows flip to 'error'). The service
+  // client is used for archive generation only; authorization happened at
+  // the extension dispatcher (or the cron), and every archive query filters
+  // by the explicit companyId.
+  const archiveClient = createServiceClient()
+
   const connection = await loadExtensionData<GoogleDriveConnection>(
     supabase,
     companyId,
@@ -322,7 +332,7 @@ export async function performSync(params: PerformSyncParams): Promise<PerformSyn
       ].join('|'),
       includeDocuments,
       generate: () =>
-        generateFullArchive(supabase, companyId, {
+        generateFullArchive(archiveClient, companyId, {
           scope: 'period',
           period_id: period.id,
           include_documents: includeDocuments,
@@ -355,7 +365,7 @@ export async function performSync(params: PerformSyncParams): Promise<PerformSyn
     ].join('|'),
     includeDocuments: baseDecision.includeDocuments,
     generate: () =>
-      generateBaseDataArchive(supabase, companyId, {
+      generateBaseDataArchive(archiveClient, companyId, {
         include_documents: baseDecision.includeDocuments,
       }),
   })

extensions/general/invoice-inbox/__tests__/retry-extraction.test.ts

@@ -23,6 +23,16 @@ vi.mock('@/lib/entitlements/has-capability', async (importOriginal) => {
   return { ...actual, hasCapability: vi.fn().mockResolvedValue(true) }
 })
 
+// The attachment download runs on the service-role client (the storage
+// SELECT policy is per-uploader-folder, and inbox documents are attributed
+// to the company creator, not the caller).
+const serviceDownloadMock = vi.fn()
+vi.mock('@/lib/supabase/server', () => ({
+  createServiceClient: () => ({
+    storage: { from: vi.fn(() => ({ download: serviceDownloadMock })) },
+  }),
+}))
+
 import { extractInvoiceFields } from '@/extensions/general/invoice-inbox/lib/extract-invoice-fields'
 import { checkInboxUploadRateLimit } from '@/lib/rate-limits/inbox'
 import { hasCapability } from '@/lib/entitlements/has-capability'
@@ -155,11 +165,9 @@ describe('POST /items/:id/retry-extraction', () => {
     })
     enqueue({ data: null, error: null }) // inbox update on success
 
-    supabase.storage.from = vi.fn().mockReturnValue({
-      download: vi.fn().mockResolvedValue({
-        data: new Blob([new Uint8Array([1, 2, 3])], { type: 'application/pdf' }),
-        error: null,
-      }),
+    serviceDownloadMock.mockResolvedValue({
+      data: new Blob([new Uint8Array([1, 2, 3])], { type: 'application/pdf' }),
+      error: null,
     })
 
     vi.mocked(extractInvoiceFields).mockResolvedValueOnce(EXTRACTION_SUCCESS as never)
@@ -183,11 +191,9 @@ describe('POST /items/:id/retry-extraction', () => {
     })
     enqueue({ data: null, error: null }) // error-state update
 
-    supabase.storage.from = vi.fn().mockReturnValue({
-      download: vi.fn().mockResolvedValue({
-        data: new Blob([new Uint8Array([1])], { type: 'application/pdf' }),
-        error: null,
-      }),
+    serviceDownloadMock.mockResolvedValue({
+      data: new Blob([new Uint8Array([1])], { type: 'application/pdf' }),
+      error: null,
     })
 
     vi.mocked(extractInvoiceFields).mockRejectedValueOnce(new Error('pdfjs blew up'))

extensions/general/invoice-inbox/index.ts

@@ -3,6 +3,7 @@ import { NextResponse } from 'next/server'
 import { createClient } from '@supabase/supabase-js'
 import { z } from 'zod'
 import { uploadDocument } from '@/lib/core/documents/document-service'
+import { createServiceClient } from '@/lib/supabase/server'
 import { extractInvoiceFields, ExtractionSchema, emptyResult } from './lib/extract-invoice-fields'
 import {
   verifyInboundWebhook,
@@ -1179,7 +1180,12 @@ export const invoiceInboxExtension: Extension = {
           return NextResponse.json({ error: 'Bilagan kunde inte hittas.' }, { status: 404 })
         }
 
-        const { data: blob, error: dlError } = await ctx.supabase.storage
+        // Download via the service-role client: the storage SELECT policy
+        // only covers the uploader's own folder, and inbox documents are
+        // attributed to the company creator, so ctx.supabase (user-bound)
+        // cannot read them for other members. The company-scoped row fetch
+        // above is the authorization.
+        const { data: blob, error: dlError } = await createServiceClient().storage
           .from('documents')
           .download(doc.storage_path)
 

lib/core/documents/__tests__/document-service.test.ts

@@ -43,8 +43,13 @@ function makeClient(storageOverrides: Record<string, unknown> = {}) {
   }
 }
 
+// verifyIntegrity downloads via the service-role client (the storage SELECT
+// policy is per-uploader-folder); tests set this override to control the
+// downloaded bytes.
+let serviceClientOverride: ReturnType<typeof makeClient> | null = null
+
 vi.mock('@/lib/auth/api-keys', () => ({
-  createServiceClientNoCookies: vi.fn(() => makeClient()),
+  createServiceClientNoCookies: vi.fn(() => serviceClientOverride ?? makeClient()),
 }))
 
 import {
@@ -66,6 +71,7 @@ beforeEach(() => {
   _resetBucketVerified()
   resultIdx = 0
   results = []
+  serviceClientOverride = null
 })
 
 describe('validateDocumentMagicBytes: application/xhtml+xml', () => {
@@ -298,15 +304,15 @@ describe('verifyIntegrity', () => {
       { data: { storage_path: 'docs/test.pdf', sha256_hash: expectedHash }, error: null },
     ]
 
-    // Create a client with matching download content
-    const supabase = makeClient({
+    // The download runs on the service-role client; give it matching bytes.
+    serviceClientOverride = makeClient({
       download: vi.fn().mockResolvedValue({
         data: new Blob([content]),
         error: null,
       }),
     })
 
-    const result = await verifyIntegrity(supabase as never, 'user-1', 'doc-1')
+    const result = await verifyIntegrity(makeClient() as never, 'user-1', 'doc-1')
     expect(result.valid).toBe(true)
     expect(result.storedHash).toBe(expectedHash)
     expect(result.computedHash).toBe(expectedHash)
@@ -317,14 +323,14 @@ describe('verifyIntegrity', () => {
       { data: { storage_path: 'docs/test.pdf', sha256_hash: 'stored-hash-abc' }, error: null },
     ]
 
-    const supabase = makeClient({
+    serviceClientOverride = makeClient({
       download: vi.fn().mockResolvedValue({
         data: new Blob(['different content']),
         error: null,
       }),
     })
 
-    const result = await verifyIntegrity(supabase as never, 'user-1', 'doc-1')
+    const result = await verifyIntegrity(makeClient() as never, 'user-1', 'doc-1')
     expect(result.valid).toBe(false)
     expect(result.storedHash).toBe('stored-hash-abc')
     expect(result.computedHash).not.toBe('stored-hash-abc')

lib/core/documents/document-service.ts

@@ -488,8 +488,12 @@ export async function verifyIntegrity(
     throw new Error('Document not found')
   }
 
-  // Download file from storage
-  const { data: fileData, error: downloadError } = await supabase.storage
+  // Download via the service-role client: the storage SELECT policy only
+  // covers the uploader's own folder (documents/{uid}/...), so a caller-bound
+  // client cannot read colleague-uploaded files. The company-filtered row
+  // fetch above (RLS on document_attachments) is the authorization.
+  const serviceClient = createServiceClientNoCookies()
+  const { data: fileData, error: downloadError } = await serviceClient.storage
     .from('documents')
     .download(doc.storage_path)