fix(security): audit remediation 2026-09-01: api_keys identity, viewer gates, OAuth binding, XSS, MFA gate (#2155)
* fix(security): bind api_keys to the caller, lock hash-as-bearer RPCs and provider token tables Security audit 2026-09-01, critical items. - api_keys INSERT requires user_id = auth.uid() again (an admin could forge a key for any co-member and act as them in every company they belong to); SELECT is own-keys-or-admin; a BEFORE trigger freezes the identity and credential columns against user-session UPDATEs. - rotate_mcp_refresh_token and validate_and_increment_api_key become service_role only: they match rows by a presented SHA-256, so a hash readable by co-members was a bearer credential. - validate_and_increment_api_key fails closed when the key's user is no longer a member of the key's company. - provider_consent_tokens and provider_otc: the DELETE policies collapsed to "caller has any team row" (correlated subquery on a non-existent team_members.company_id). All member policies dropped; service_role only, matching every existing code path. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(security): role gates, ownership guards and posting integrity in the database Security audit 2026-09-01, high items at the database layer. - One table-level guard, enforce_company_writer_role(), blocks the read-only viewer role on 55 company-scoped tables including through the 15 membership-only SECURITY DEFINER writers. Keyed on the JWT role claim so it fires inside definer bodies; no-op for service_role and trigger cascades. - company_members user_id/company_id immutable from user sessions; invitations can never grant owner; team_members gains a transition guard (admins keep non-owner role moves); companies team_id and archiving are owner-only and team attachment needs team membership. - Direct statements (current_user = authenticated) can no longer insert posted headers, add lines under posted verifikat, or post a draft with a voucher number the sequence never issued. Sanctioned RPCs run as the definer and are untouched; the engine's own draft-then-post shapes still pass. - create_document_version refuses viewers and foreign storage paths; validate_version_chain needs membership and loses anon EXECUTE; match_documents / match_booking_templates lose anon; cron maintenance RPCs become service_role only; the production-only seed_asset_categories is dropped. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * build: pin tsx as an exact devDependency instead of fetching it with npx at build time prebuild ran "npx tsx" with no lockfile entry, so every Vercel, Docker and CI build downloaded tsx@latest and its transitive tree from the registry with no integrity check, inside the build environment. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(security): refuse the viewer role on API-key and MCP write paths The v1 wrapper and the MCP company routing checked company membership but never role, and both run as service role, so a read-only viewer holding an API key could post vouchers and change settings through the API. Mutating methods and non-read scopes now return 403 ROLE_READ_ONLY for viewers on v1; MCP write tools refuse viewers the same way. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(security): stop serving uploaded SVG, XML and HTML as executable content on the app origin Uploads persisted the browser-declared mime type and the inline proxy served it verbatim, sandboxing only text/html; the storage proxy forwarded the uploader's Content-Type. Any writer, or any Peppol sender, could plant a scripted SVG or XHTML that executed on app.gnubok.se. - inline route: allow-list of natively safe types (PDF, raster images) served as before; everything else gets the opaque sandbox CSP. - storage proxy: octet-stream + attachment + sandbox unless the DB mime for the key is on the allow-list. - document-service: the stored mime is the magic-byte validated type. - logo upload: magic-byte validation, SVG refused. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(security): byrå brand logo upload decides the type by magic bytes and drops SVG Same pattern as the company logo route: the logos bucket is public, so a scripted SVG (or anything declared as an image) must never land there. The upload pickers stop advertising SVG. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(security): bind Enable Banking, Stripe and WooCommerce callbacks to the initiating user The callbacks resolved the pending row by oauth_state alone, so a victim who completed an attacker-initiated consent had their bank account, merchant account or store attached to the attacker's company. requireFlowInitiator() now requires the cookie session of the user who started the flow: no session redirects to login with the callback URL preserved, a different user is refused and nothing is exchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(security): guard tenant-controlled outbound fetches and surface the disabled rate limiter WooCommerce and Shopify syncs fetched a member-editable store URL with plain fetch() and redirect following under the service role, and the invoice PDF renderer fetched company_settings.logo_url unguarded. All three go through a new safeFetch() (public-IP validation via url-guard, https only, redirect: 'manual', body size cap) and re-normalise the stored host at use time. checkRateLimit() keeps failing open on hosted but logs one error per process when Upstash is not configured and exports isRateLimiterConfigured(). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(security): decide the API MFA gate from server-authenticated factors, not the session cookie getAuthenticatorAssuranceLevel() without arguments derives nextLevel from session.user.factors, which comes from the unsigned sb-*-auth-token cookie. Deleting factors from the cookie made an enrolled account look like it had nothing to step up to, on every /api route and in requireAuth. Both gates now read factors from the getUser() result or listFactors() and the level from the verified JWT claim, and fail closed on errors. Page-branch gate hardened the same way. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(security): bind Fortnox/Visma, Gmail and Skatteverket callbacks to the initiating user The arcim-migration callback exchanged the provider code onto whatever consent the one-time state named, with no check of who completed the flow and no org-number comparison, so a phished Fortnox admin handed their ledger to the attacker's company. provider_otc now records the initiating user (migration 20260902100000); the callback requires that session and, after the exchange, refuses a provider company whose org number differs from the consent's company. The Gmail and Skatteverket callbacks enforce the same initiator check. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(security): BankID signup confirms the email before linking the identity Signup created an email-confirmed, MFA-exempt account for any address the caller typed and returned a magic link, so an attacker could pre-register a victim's email and keep a permanent BankID login into the account the victim later adopted. The user is now created unconfirmed, the identity carries email_verified_at NULL (migration 20260902101000), bankid_linked is not set until the mailed confirmation is clicked, and BankID login of a pending identity is refused with the confirmation re-sent. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(security): bind MCP OAuth redirect URIs to the consenting user and cap scopes A user-registered redirect URI was allowlisted globally, the consent page named no client, and all scopes were pre-checked, so one phishing link handed an attacker a full-scope key for the victim's company. Registered URIs now resolve only for the registrant or a colleague sharing a company; the consent page shows the client identity and redirect host; non-built-in clients default to read-only pre-checks; scopes are capped by the user's role (viewer: read only) at consent and at /token. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(auth): client follow-ups for BankID confirmation, callback mismatch copy and decision log - register client handles the new confirmation_sent response from BankID signup with the existing inbox screen instead of calling verifyOtp. - BankID login surfaces the email_unconfirmed explanation. - WooCommerce settings map woocommerce_error=wrong_user to its own copy. - Logo help text no longer advertises SVG. - DECISIONS.md records the audit remediation choices. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(mcp-oauth): literal SoD columns in the api_keys insert so the phantom-column scanner resolves them Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * test(logo): type the upload fixtures as Uint8Array<ArrayBuffer> so they are valid BlobParts Fixes the typecheck ratchet on PR #2155 and ratchets the baseline down by the one legacy error the change removed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Jakob Wennberg <311770904+jakobwennberg-oss@users.noreply.github.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Jakob Wennberg
co-authored by
Claude Fable 5.1
Jakob Wennberg
Claude Fable 5.1
Jakob Wennberg
parent
6e8d76a9cb
commit
18cbc4c30a
@@ -9,15 +9,31 @@ vi.mock('@/extensions/general/enable-banking/lib/api-client', () => ({
|
||||
}))
|
||||
|
||||
// Use hoisted to safely create mock objects referenced in vi.mock factories
|
||||
const { mockFrom, mockUpsertFromPsd2, mockAllocate, mockSupersede, mockCrossCompanyContext } =
|
||||
vi.hoisted(() => {
|
||||
const mockFrom = vi.fn()
|
||||
const mockUpsertFromPsd2 = vi.fn()
|
||||
const mockAllocate = vi.fn()
|
||||
const mockSupersede = vi.fn()
|
||||
const mockCrossCompanyContext = vi.fn()
|
||||
return { mockFrom, mockUpsertFromPsd2, mockAllocate, mockSupersede, mockCrossCompanyContext }
|
||||
})
|
||||
const {
|
||||
mockFrom,
|
||||
mockUpsertFromPsd2,
|
||||
mockAllocate,
|
||||
mockSupersede,
|
||||
mockCrossCompanyContext,
|
||||
mockGetUser,
|
||||
} = vi.hoisted(() => {
|
||||
const mockFrom = vi.fn()
|
||||
const mockUpsertFromPsd2 = vi.fn()
|
||||
const mockAllocate = vi.fn()
|
||||
const mockSupersede = vi.fn()
|
||||
const mockCrossCompanyContext = vi.fn()
|
||||
// The cookie session the callback binds the completion to. Every pending
|
||||
// row in this suite belongs to 'user-1', so that is the default session.
|
||||
const mockGetUser = vi.fn()
|
||||
return {
|
||||
mockFrom,
|
||||
mockUpsertFromPsd2,
|
||||
mockAllocate,
|
||||
mockSupersede,
|
||||
mockCrossCompanyContext,
|
||||
mockGetUser,
|
||||
}
|
||||
})
|
||||
|
||||
// The supersede pass has its own unit tests (extensions/general/enable-banking/
|
||||
// __tests__/supersede.test.ts); here it is mocked so these tests assert the
|
||||
@@ -43,6 +59,9 @@ vi.mock('@/lib/supabase/server', () => ({
|
||||
createServiceClient: vi.fn().mockResolvedValue({
|
||||
from: mockFrom,
|
||||
}),
|
||||
createClient: vi.fn().mockResolvedValue({
|
||||
auth: { getUser: mockGetUser },
|
||||
}),
|
||||
}))
|
||||
|
||||
const CURRENCY_DEFAULTS: Record<string, string> = {
|
||||
@@ -104,6 +123,7 @@ function mockChain(result: { data?: unknown; error?: unknown }) {
|
||||
describe('GET /api/extensions/enable-banking/callback', () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
mockGetUser.mockResolvedValue({ data: { user: { id: 'user-1' } }, error: null })
|
||||
mockUpsertFromPsd2.mockResolvedValue(undefined)
|
||||
mockSupersede.mockResolvedValue({ supersededIds: [], dedupScopeByIban: new Map() })
|
||||
// No sibling company claims anything by default; individual tests override.
|
||||
@@ -149,6 +169,120 @@ describe('GET /api/extensions/enable-banking/callback', () => {
|
||||
expect(decodeURIComponent(location)).toContain('Starta bankkopplingen på nytt')
|
||||
})
|
||||
|
||||
// The state token proves the callback belongs to a flow WE started, not that
|
||||
// the browser completing it is the initiator's. A victim lured into
|
||||
// approving a consent someone else started must not have their bank
|
||||
// attached to that someone's company.
|
||||
describe('initiator binding', () => {
|
||||
const PENDING_ROW = {
|
||||
id: 'conn-1',
|
||||
user_id: 'user-1',
|
||||
company_id: 'company-1',
|
||||
bank_name: 'TestBank',
|
||||
status: 'pending',
|
||||
session_id: null,
|
||||
accounts_data: null,
|
||||
}
|
||||
|
||||
it('refuses a consent completed by a different user and leaves the row untouched', async () => {
|
||||
const chain = mockChain({ data: PENDING_ROW, error: null })
|
||||
mockFrom.mockReturnValue(chain)
|
||||
mockGetUser.mockResolvedValue({ data: { user: { id: 'user-2' } }, error: null })
|
||||
|
||||
const response = await GET(makeRequest({ code: 'auth-code', state: 'valid-state' }))
|
||||
|
||||
expect(response.status).toBe(307)
|
||||
const location = new URL(response.headers.get('location') || '')
|
||||
expect(location.pathname).toBe('/settings/banking')
|
||||
expect(location.searchParams.get('bank_error')).toContain('annat användarkonto')
|
||||
expect(location.searchParams.get('bank_name')).toBe('TestBank')
|
||||
expect(location.searchParams.has('select_accounts')).toBe(false)
|
||||
|
||||
// The code was never exchanged and nothing was written: the row keeps
|
||||
// waiting for its initiator (only the lookup touched the table).
|
||||
expect(mockCreateSession).not.toHaveBeenCalled()
|
||||
expect(mockFrom).toHaveBeenCalledTimes(1)
|
||||
expect(chain.update).not.toHaveBeenCalled()
|
||||
expect(chain.delete).not.toHaveBeenCalled()
|
||||
expect(mockUpsertFromPsd2).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('sends an anonymous browser to /login with the callback URL preserved', async () => {
|
||||
const chain = mockChain({ data: PENDING_ROW, error: null })
|
||||
mockFrom.mockReturnValue(chain)
|
||||
mockGetUser.mockResolvedValue({ data: { user: null }, error: null })
|
||||
|
||||
const response = await GET(makeRequest({ code: 'auth-code', state: 'valid-state' }))
|
||||
|
||||
expect(response.status).toBe(307)
|
||||
const location = new URL(response.headers.get('location') || '')
|
||||
expect(location.origin).toBe('http://localhost:3000')
|
||||
expect(location.pathname).toBe('/login')
|
||||
expect(location.searchParams.get('next')).toBe(
|
||||
'/api/extensions/enable-banking/callback?code=auth-code&state=valid-state',
|
||||
)
|
||||
|
||||
expect(mockCreateSession).not.toHaveBeenCalled()
|
||||
expect(chain.update).not.toHaveBeenCalled()
|
||||
expect(chain.delete).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('finalizes as before when the session belongs to the initiator', async () => {
|
||||
// mockConnectionFlow is the suite's standard script for the finalize
|
||||
// path (function declaration below, hoisted into this scope).
|
||||
mockConnectionFlow(PENDING_ROW)
|
||||
mockGetUser.mockResolvedValue({ data: { user: { id: 'user-1' } }, error: null })
|
||||
mockCreateSession.mockResolvedValue({
|
||||
session_id: 'sess-1',
|
||||
accounts: [],
|
||||
access: { valid_until: '2027-12-31T00:00:00Z' },
|
||||
aspsp: { name: 'TestBank', country: 'SE' },
|
||||
})
|
||||
|
||||
const response = await GET(makeRequest({ code: 'auth-code', state: 'valid-state' }))
|
||||
|
||||
expect(response.status).toBe(200)
|
||||
expect(mockGetUser).toHaveBeenCalledTimes(1)
|
||||
expect(mockCreateSession).toHaveBeenCalledWith('auth-code', undefined)
|
||||
expect(await response.text()).toContain('select_accounts=conn-1')
|
||||
})
|
||||
|
||||
it('does not consult the session for an unknown state (nothing to bind to)', async () => {
|
||||
mockFrom.mockImplementation(() => mockChain({ data: null, error: { message: 'not found' } }))
|
||||
|
||||
await GET(makeRequest({ code: 'auth-code', state: 'unknown-state' }))
|
||||
|
||||
expect(mockGetUser).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('leaves the hosted connector bounce alone (server-to-server, HMAC-verified)', async () => {
|
||||
// The hosted proxy callback never finalizes anything: it verifies the
|
||||
// signed connector state and bounces the browser to the instance, whose
|
||||
// own callback then runs the binding against ITS session.
|
||||
vi.stubEnv('CONNECTOR_STATE_SECRET', 'test-connector-secret')
|
||||
const { signConnectorState } = await import('@/lib/connect/hosted/state')
|
||||
const connectorState = signConnectorState({
|
||||
kid: 'key-1',
|
||||
svc: 'bank',
|
||||
ret: 'https://instance.example.se/api/extensions/enable-banking/callback',
|
||||
st: 'instance-state',
|
||||
cref: 'company-ref',
|
||||
})
|
||||
|
||||
const response = await GET(makeRequest({ code: 'auth-code', state: connectorState }))
|
||||
|
||||
expect(response.status).toBe(307)
|
||||
const location = new URL(response.headers.get('location') || '')
|
||||
expect(location.origin).toBe('https://instance.example.se')
|
||||
expect(location.searchParams.get('state')).toBe('instance-state')
|
||||
expect(location.searchParams.get('code')).toBe('auth-code')
|
||||
expect(mockGetUser).not.toHaveBeenCalled()
|
||||
expect(mockFrom).not.toHaveBeenCalled()
|
||||
vi.unstubAllEnvs()
|
||||
vi.stubEnv('NEXT_PUBLIC_APP_URL', 'http://localhost:3000')
|
||||
})
|
||||
})
|
||||
|
||||
it('threads connector_state from the query into createSession (connector mode)', async () => {
|
||||
// In connector mode the hosted callback bounces the browser back here with
|
||||
// the signed connector_state echoed alongside code + the instance's own
|
||||
|
||||
@@ -20,6 +20,10 @@ import { supersedeSiblingConnections } from '@/extensions/general/enable-banking
|
||||
import { getBankConnectionErrorMessage } from '@/lib/errors/get-error-message'
|
||||
import { renderFinalizeShell, renderFinalizeRedirect } from './finalize-page'
|
||||
import { isConnectorState, verifyConnectorState } from '@/lib/connect/hosted/state'
|
||||
import {
|
||||
requireFlowInitiator,
|
||||
FLOW_INITIATOR_MISMATCH_MESSAGE,
|
||||
} from '@/lib/auth/oauth-flow-binding'
|
||||
|
||||
// This route emits bank_connection.consent_granted / .cash_account_mirror_failed
|
||||
// (ASVS V16 / GDPR Art.30 audit events). ensureInitialized() must run at module
|
||||
@@ -269,6 +273,31 @@ export async function GET(request: Request) {
|
||||
)
|
||||
}
|
||||
|
||||
// The state token proves this callback belongs to a flow we started; it
|
||||
// says nothing about WHO is completing it. Bind the completion to the
|
||||
// initiator's own cookie session before any finalize work: otherwise a
|
||||
// victim lured into approving a consent someone else started would have
|
||||
// their bank accounts attached to that someone's company. The connector
|
||||
// branch above is exempt on purpose (server-to-server, HMAC-verified).
|
||||
const initiator = await requireFlowInitiator(request, pendingConnection.user_id, {
|
||||
flow: 'enable-banking.callback',
|
||||
})
|
||||
if (!initiator.ok) {
|
||||
if (initiator.reason === 'no_session') {
|
||||
// Session expired mid-flow: sign in and the callback re-runs with the
|
||||
// same code + state. Nothing on the row changes.
|
||||
return initiator.response
|
||||
}
|
||||
// A different user completed it. Refuse without exchanging the code and
|
||||
// without touching the row: it keeps waiting for its initiator and the
|
||||
// stale-pending cleanup reaps it if nobody comes back.
|
||||
const params = new URLSearchParams({
|
||||
bank_error: FLOW_INITIATOR_MISMATCH_MESSAGE,
|
||||
...(pendingConnection.bank_name ? { bank_name: pendingConnection.bank_name } : {}),
|
||||
})
|
||||
return NextResponse.redirect(`${baseUrl}/settings/banking?${params.toString()}`)
|
||||
}
|
||||
|
||||
// Kick the finalize work off eagerly, decoupled from the response stream:
|
||||
// if the user closes the tab mid-stream, the stream is cancelled but this
|
||||
// promise keeps running, so the session persistence, cash-account mirror
|
||||
|
||||
@@ -9,10 +9,16 @@ vi.mock('@/extensions/general/stripe/lib/connect', () => ({
|
||||
fetchAccountDisplayName: (...args: unknown[]) => mockFetchAccountDisplayName(...args),
|
||||
}))
|
||||
|
||||
const { mockFrom } = vi.hoisted(() => ({ mockFrom: vi.fn() }))
|
||||
const { mockFrom, mockGetUser } = vi.hoisted(() => ({
|
||||
mockFrom: vi.fn(),
|
||||
// The cookie session the callback binds the completion to. Every pending
|
||||
// row in this suite belongs to 'user-1', so that is the default session.
|
||||
mockGetUser: vi.fn(),
|
||||
}))
|
||||
|
||||
vi.mock('@/lib/supabase/server', () => ({
|
||||
createServiceClient: vi.fn().mockResolvedValue({ from: mockFrom }),
|
||||
createClient: vi.fn().mockResolvedValue({ auth: { getUser: mockGetUser } }),
|
||||
}))
|
||||
|
||||
vi.mock('@/lib/init', () => ({ ensureInitialized: vi.fn() }))
|
||||
@@ -50,6 +56,7 @@ describe('GET /api/extensions/stripe/callback', () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
eventBus.clear()
|
||||
mockGetUser.mockResolvedValue({ data: { user: { id: 'user-1' } }, error: null })
|
||||
mockExchangeCodeForAccount.mockResolvedValue({
|
||||
stripeAccountId: 'acct_123',
|
||||
livemode: false,
|
||||
@@ -146,4 +153,64 @@ describe('GET /api/extensions/stripe/callback', () => {
|
||||
)
|
||||
expect(mockFrom).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
// The state token proves the callback belongs to a flow WE started, not that
|
||||
// the browser completing it is the initiator's. A victim lured into
|
||||
// approving a Connect someone else started must not have their Stripe
|
||||
// account attached to that someone's company.
|
||||
describe('initiator binding', () => {
|
||||
const PENDING_ROW = { id: CONNECTION_ID, user_id: 'user-1', company_id: 'company-1' }
|
||||
|
||||
it('refuses a consent completed by a different user without burning the code or marking the row', async () => {
|
||||
const findChain = mockChain({ data: PENDING_ROW })
|
||||
mockFrom.mockReturnValue(findChain)
|
||||
mockGetUser.mockResolvedValue({ data: { user: { id: 'user-2' } }, error: null })
|
||||
|
||||
const response = await GET(makeRequest({ code: 'ac_123', state: OAUTH_STATE }))
|
||||
|
||||
expect(response.status).toBe(307)
|
||||
const location = decodeURIComponent(response.headers.get('location') || '')
|
||||
expect(location.startsWith('http://localhost:3000/import?mode=stripe&stripe_error=')).toBe(true)
|
||||
// The panel shows an unknown stripe_error value verbatim, so the param
|
||||
// carries the Swedish explanation itself.
|
||||
expect(location).toContain('annat användarkonto')
|
||||
expect(location).not.toContain('stripe_connected')
|
||||
|
||||
// Only the lookup touched the database: no oauth_used_codes insert (the
|
||||
// code stays valid for the initiator), no update on the row.
|
||||
expect(mockFrom).toHaveBeenCalledTimes(1)
|
||||
expect(findChain.insert).not.toHaveBeenCalled()
|
||||
expect(findChain.update).not.toHaveBeenCalled()
|
||||
expect(mockExchangeCodeForAccount).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('sends an anonymous browser to /login with the callback URL preserved', async () => {
|
||||
const findChain = mockChain({ data: PENDING_ROW })
|
||||
mockFrom.mockReturnValue(findChain)
|
||||
mockGetUser.mockResolvedValue({ data: { user: null }, error: null })
|
||||
|
||||
const response = await GET(makeRequest({ code: 'ac_123', state: OAUTH_STATE }))
|
||||
|
||||
expect(response.status).toBe(307)
|
||||
const location = new URL(response.headers.get('location') || '')
|
||||
expect(location.origin).toBe('http://localhost:3000')
|
||||
expect(location.pathname).toBe('/login')
|
||||
expect(location.searchParams.get('next')).toBe(
|
||||
`/api/extensions/stripe/callback?code=ac_123&state=${OAUTH_STATE}`,
|
||||
)
|
||||
|
||||
expect(mockFrom).toHaveBeenCalledTimes(1)
|
||||
expect(findChain.insert).not.toHaveBeenCalled()
|
||||
expect(findChain.update).not.toHaveBeenCalled()
|
||||
expect(mockExchangeCodeForAccount).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('does not consult the session for an unknown state (nothing to bind to)', async () => {
|
||||
mockFrom.mockReturnValueOnce(mockChain({ data: null, error: { code: 'PGRST116' } }))
|
||||
|
||||
await GET(makeRequest({ code: 'ac_123', state: 'unknown-state' }))
|
||||
|
||||
expect(mockGetUser).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@@ -3,6 +3,10 @@ import { NextResponse } from 'next/server'
|
||||
import { ensureInitialized } from '@/lib/init'
|
||||
import { eventBus } from '@/lib/events/bus'
|
||||
import { hashAuthCode } from '@/lib/auth/oauth-codes'
|
||||
import {
|
||||
requireFlowInitiator,
|
||||
FLOW_INITIATOR_MISMATCH_MESSAGE,
|
||||
} from '@/lib/auth/oauth-flow-binding'
|
||||
import {
|
||||
exchangeCodeForAccount,
|
||||
fetchAccountDisplayName,
|
||||
@@ -90,6 +94,29 @@ export async function GET(request: Request) {
|
||||
)
|
||||
}
|
||||
|
||||
// The state token proves this callback belongs to a flow we started; it
|
||||
// says nothing about WHO is completing it. Bind the completion to the
|
||||
// initiator's own cookie session BEFORE the code is burned below:
|
||||
// otherwise a victim lured into approving a Stripe Connect someone else
|
||||
// started would have their Stripe account attached to that someone's
|
||||
// company. Stripe's redirect is a top-level navigation, so the
|
||||
// initiator's cookies are present on the legitimate path.
|
||||
const initiator = await requireFlowInitiator(request, pendingConnection.user_id, {
|
||||
flow: 'stripe.callback',
|
||||
})
|
||||
if (!initiator.ok) {
|
||||
if (initiator.reason === 'no_session') {
|
||||
// Session expired mid-flow: sign in and the callback re-runs with
|
||||
// the same (still unused) code + state. The row is untouched.
|
||||
return initiator.response
|
||||
}
|
||||
// A different user completed it. Refuse without exchanging the code
|
||||
// and without marking the row: it stays pending for its initiator.
|
||||
return NextResponse.redirect(
|
||||
`${returnUrl}&stripe_error=${encodeURIComponent(FLOW_INITIATOR_MISMATCH_MESSAGE)}`,
|
||||
)
|
||||
}
|
||||
|
||||
// Replay protection (OAuth 2.1 §4.1.2): a code may be exchanged once.
|
||||
// The PRIMARY KEY on oauth_used_codes rejects a second insert.
|
||||
const { error: replayError } = await supabase
|
||||
|
||||
@@ -150,7 +150,11 @@ describe('POST /api/extensions/woocommerce/callback', () => {
|
||||
const activation = updates[0][0] as Record<string, string | boolean | null>
|
||||
expect(activation.status).toBe('active')
|
||||
expect(activation.transaction_sync_enabled).toBe(true)
|
||||
expect(activation.oauth_state).toBeNull()
|
||||
// The state survives activation on purpose: this POST has no browser
|
||||
// session, so the browser leg (../return) binds the completion to the
|
||||
// initiator by looking the row up with this same state and consumes it
|
||||
// there. Replay is blocked by the status 'pending' scope instead.
|
||||
expect(activation).not.toHaveProperty('oauth_state')
|
||||
expect(activation.store_name).toBe('Testbutiken')
|
||||
// Secrets never stored in plaintext, and they decrypt back.
|
||||
expect(String(activation.consumer_key_encrypted)).not.toContain('ck_new')
|
||||
|
||||
@@ -0,0 +1,194 @@
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'
|
||||
|
||||
vi.mock('@/lib/supabase/server', () => ({
|
||||
createServiceClient: vi.fn(),
|
||||
createClient: vi.fn(),
|
||||
}))
|
||||
vi.mock('@/lib/extensions/loader', () => ({ loadExtensions: vi.fn() }))
|
||||
vi.mock('@/lib/extensions/registry', () => ({ extensionRegistry: { get: vi.fn() } }))
|
||||
|
||||
import { GET } from '../return/route'
|
||||
import { createServiceClient, createClient } from '@/lib/supabase/server'
|
||||
import { extensionRegistry } from '@/lib/extensions/registry'
|
||||
import { createQueuedMockSupabase } from '@/tests/helpers'
|
||||
|
||||
const STATE = '123e4567-e89b-12d3-a456-426614174000'
|
||||
const BASE = 'http://localhost:3000'
|
||||
const CONNECTED = `${BASE}/import?mode=woocommerce&woocommerce_connected=true`
|
||||
|
||||
function makeReturnRequest(params: Record<string, string>): Request {
|
||||
const url = new URL(`${BASE}/api/extensions/woocommerce/return`)
|
||||
for (const [k, v] of Object.entries(params)) url.searchParams.set(k, v)
|
||||
return new Request(url.toString())
|
||||
}
|
||||
|
||||
function mockServiceClient() {
|
||||
const queued = createQueuedMockSupabase()
|
||||
vi.mocked(createServiceClient).mockResolvedValue(
|
||||
queued.supabase as unknown as Awaited<ReturnType<typeof createServiceClient>>,
|
||||
)
|
||||
return queued
|
||||
}
|
||||
|
||||
function mockSession(userId: string | null) {
|
||||
const getUser = vi
|
||||
.fn()
|
||||
.mockResolvedValue({ data: { user: userId ? { id: userId } : null }, error: null })
|
||||
vi.mocked(createClient).mockResolvedValue({ auth: { getUser } } as never)
|
||||
return getUser
|
||||
}
|
||||
|
||||
const ROW = (status: 'pending' | 'active') => ({
|
||||
id: 'conn-1',
|
||||
user_id: 'user-1',
|
||||
status,
|
||||
})
|
||||
|
||||
describe('GET /api/extensions/woocommerce/return', () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
vi.stubEnv('NEXT_PUBLIC_APP_URL', BASE)
|
||||
vi.mocked(extensionRegistry.get).mockReturnValue(
|
||||
{ id: 'woocommerce' } as ReturnType<typeof extensionRegistry.get>,
|
||||
)
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
vi.unstubAllEnvs()
|
||||
})
|
||||
|
||||
it('refuses with 503 when the extension is disabled', async () => {
|
||||
vi.mocked(extensionRegistry.get).mockReturnValue(undefined)
|
||||
const res = await GET(makeReturnRequest({ success: '1', user_id: STATE }))
|
||||
expect(res.status).toBe(503)
|
||||
})
|
||||
|
||||
it('closes the pending row and reports the denial when the store says no', async () => {
|
||||
const { supabase, enqueue, findCall } = mockServiceClient()
|
||||
enqueue({ data: null })
|
||||
|
||||
const res = await GET(makeReturnRequest({ success: '0', user_id: STATE }))
|
||||
|
||||
expect(res.headers.get('location')).toBe(
|
||||
`${BASE}/import?mode=woocommerce&woocommerce_error=denied`,
|
||||
)
|
||||
const update = findCall('woocommerce_connections', 'update')?.[0] as Record<string, unknown>
|
||||
expect(update).toMatchObject({ status: 'error', oauth_state: null })
|
||||
expect(supabase.from).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
describe('approved leg (success=1)', () => {
|
||||
it('hands an active row over to its initiator and consumes the state', async () => {
|
||||
const { enqueue, findCalls } = mockServiceClient()
|
||||
const getUser = mockSession('user-1')
|
||||
enqueue({ data: ROW('active') }) // lookup by oauth_state
|
||||
enqueue({ data: null }) // consume update
|
||||
|
||||
const res = await GET(makeReturnRequest({ success: '1', user_id: STATE }))
|
||||
|
||||
expect(res.status).toBe(307)
|
||||
expect(res.headers.get('location')).toBe(CONNECTED)
|
||||
expect(getUser).toHaveBeenCalledTimes(1)
|
||||
const updates = findCalls('woocommerce_connections', 'update')
|
||||
expect(updates).toHaveLength(1)
|
||||
expect(updates[0][0]).toEqual({ oauth_state: null })
|
||||
})
|
||||
|
||||
it('leaves a still-pending row untouched for the initiator (the callback still needs the state)', async () => {
|
||||
const { enqueue, findCalls } = mockServiceClient()
|
||||
mockSession('user-1')
|
||||
enqueue({ data: ROW('pending') })
|
||||
|
||||
const res = await GET(makeReturnRequest({ success: '1', user_id: STATE }))
|
||||
|
||||
expect(res.headers.get('location')).toBe(CONNECTED)
|
||||
expect(findCalls('woocommerce_connections', 'update')).toHaveLength(0)
|
||||
})
|
||||
|
||||
it('revokes an already-activated row when a different user completes the handshake', async () => {
|
||||
const { enqueue, findCalls, calls } = mockServiceClient()
|
||||
mockSession('user-2')
|
||||
enqueue({ data: ROW('active') })
|
||||
enqueue({ data: null }) // revoke update
|
||||
|
||||
const res = await GET(makeReturnRequest({ success: '1', user_id: STATE }))
|
||||
|
||||
expect(res.status).toBe(307)
|
||||
expect(res.headers.get('location')).toBe(
|
||||
`${BASE}/import?mode=woocommerce&woocommerce_error=wrong_user`,
|
||||
)
|
||||
const updates = findCalls('woocommerce_connections', 'update')
|
||||
expect(updates).toHaveLength(1)
|
||||
// The store's keys the callback stored for the wrong company are gone,
|
||||
// the state is consumed, and the row says why.
|
||||
expect(updates[0][0]).toMatchObject({
|
||||
status: 'error',
|
||||
oauth_state: null,
|
||||
consumer_key_encrypted: null,
|
||||
consumer_secret_encrypted: null,
|
||||
})
|
||||
expect(String((updates[0][0] as Record<string, unknown>).error_message)).toContain(
|
||||
'annat användarkonto',
|
||||
)
|
||||
// Scoped to this row, never a blanket update.
|
||||
const eqCalls = calls.filter((c) => c.method === 'eq').map((c) => c.args)
|
||||
expect(eqCalls).toContainEqual(['id', 'conn-1'])
|
||||
})
|
||||
|
||||
it('closes a still-pending row when a different user completes it, so the late callback cannot activate it', async () => {
|
||||
const { enqueue, findCalls } = mockServiceClient()
|
||||
mockSession('user-2')
|
||||
enqueue({ data: ROW('pending') })
|
||||
enqueue({ data: null })
|
||||
|
||||
const res = await GET(makeReturnRequest({ success: '1', user_id: STATE }))
|
||||
|
||||
expect(res.headers.get('location')).toBe(
|
||||
`${BASE}/import?mode=woocommerce&woocommerce_error=wrong_user`,
|
||||
)
|
||||
const updates = findCalls('woocommerce_connections', 'update')
|
||||
expect(updates).toHaveLength(1)
|
||||
expect(updates[0][0]).toMatchObject({ status: 'error', oauth_state: null })
|
||||
})
|
||||
|
||||
it('sends an anonymous browser to /login with the return URL preserved and touches nothing', async () => {
|
||||
const { enqueue, findCalls } = mockServiceClient()
|
||||
mockSession(null)
|
||||
enqueue({ data: ROW('active') })
|
||||
|
||||
const res = await GET(makeReturnRequest({ success: '1', user_id: STATE }))
|
||||
|
||||
expect(res.status).toBe(307)
|
||||
const location = new URL(res.headers.get('location') || '')
|
||||
expect(location.origin).toBe(BASE)
|
||||
expect(location.pathname).toBe('/login')
|
||||
expect(location.searchParams.get('next')).toBe(
|
||||
`/api/extensions/woocommerce/return?success=1&user_id=${STATE}`,
|
||||
)
|
||||
expect(findCalls('woocommerce_connections', 'update')).toHaveLength(0)
|
||||
})
|
||||
|
||||
it('does not consult the session when no row carries the state (already consumed or unknown)', async () => {
|
||||
const { enqueue, findCalls } = mockServiceClient()
|
||||
const getUser = mockSession('user-2')
|
||||
enqueue({ data: null, error: { message: 'no rows', code: 'PGRST116' } })
|
||||
|
||||
const res = await GET(makeReturnRequest({ success: '1', user_id: STATE }))
|
||||
|
||||
expect(res.headers.get('location')).toBe(CONNECTED)
|
||||
expect(getUser).not.toHaveBeenCalled()
|
||||
expect(findCalls('woocommerce_connections', 'update')).toHaveLength(0)
|
||||
})
|
||||
|
||||
it('redirects without a database round trip when the state is missing or not a uuid', async () => {
|
||||
const { supabase } = mockServiceClient()
|
||||
|
||||
const res1 = await GET(makeReturnRequest({ success: '1' }))
|
||||
const res2 = await GET(makeReturnRequest({ success: '1', user_id: 'not-a-uuid' }))
|
||||
|
||||
expect(res1.headers.get('location')).toBe(CONNECTED)
|
||||
expect(res2.headers.get('location')).toBe(CONNECTED)
|
||||
expect(supabase.from).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
})
|
||||
@@ -132,7 +132,12 @@ export async function POST(request: Request) {
|
||||
status: 'active',
|
||||
connected_at: new Date().toISOString(),
|
||||
error_message: null,
|
||||
oauth_state: null, // Clear to prevent replay
|
||||
// oauth_state is deliberately KEPT here. This POST is server-to-server
|
||||
// (the store calls it, no browser session), so the initiator check has
|
||||
// to happen on the browser leg (../return), which locates the row by
|
||||
// this same state and consumes it there. Replay is still blocked: both
|
||||
// the lookup above and this update are scoped to status 'pending', so
|
||||
// an active row can never be activated again.
|
||||
// Feed-only product: connecting the store means fetching its orders, so
|
||||
// the nightly feed starts on by default; the panel toggle is the opt-out.
|
||||
transaction_sync_enabled: true,
|
||||
|
||||
@@ -3,17 +3,32 @@ import { createServiceClient } from '@/lib/supabase/server'
|
||||
import { loadExtensions } from '@/lib/extensions/loader'
|
||||
import { extensionRegistry } from '@/lib/extensions/registry'
|
||||
import { createLogger } from '@/lib/logger'
|
||||
import {
|
||||
requireFlowInitiator,
|
||||
FLOW_INITIATOR_MISMATCH_MESSAGE,
|
||||
} from '@/lib/auth/oauth-flow-binding'
|
||||
|
||||
const log = createLogger('woocommerce/return')
|
||||
|
||||
// The state is a UUID we generated; anything else cannot match a row (and the
|
||||
// column is typed uuid, which would error opaquely on a non-UUID filter).
|
||||
const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i
|
||||
|
||||
/**
|
||||
* GET /api/extensions/woocommerce/return
|
||||
*
|
||||
* Browser leg of the wc-auth handshake: WooCommerce redirects the merchant
|
||||
* here with ?success=1|0&user_id=<our oauth_state>. The credentials arrive on
|
||||
* the separate server-to-server callback (usually before this redirect, but
|
||||
* ordering is not guaranteed), so on success this route only sends the user
|
||||
* back to the import page; the panel polls /status until the row is active.
|
||||
* ordering is not guaranteed); the panel polls /status until the row is
|
||||
* active.
|
||||
*
|
||||
* This leg is the only point in the handshake where a browser session is
|
||||
* present, so it is where the completion is bound to the user who started the
|
||||
* flow: the callback POST has no cookies (the store calls it) and so leaves
|
||||
* the oauth_state on the row for this route to verify against and consume.
|
||||
* Without that binding a victim lured into approving a connect someone else
|
||||
* started would have their store's orders flowing into that someone's books.
|
||||
*/
|
||||
export async function GET(request: Request) {
|
||||
loadExtensions()
|
||||
@@ -34,7 +49,7 @@ export async function GET(request: Request) {
|
||||
const returnUrl = `${baseUrl}/import?mode=woocommerce`
|
||||
|
||||
if (success === '1') {
|
||||
return NextResponse.redirect(`${returnUrl}&woocommerce_connected=true`)
|
||||
return completeApproved(request, state, returnUrl)
|
||||
}
|
||||
|
||||
// Denied (or malformed): close out the pending row so its state can never
|
||||
@@ -60,3 +75,93 @@ export async function GET(request: Request) {
|
||||
|
||||
return NextResponse.redirect(`${returnUrl}&woocommerce_error=denied`)
|
||||
}
|
||||
|
||||
/**
|
||||
* The approved leg: find the row this state belongs to, require that the
|
||||
* browser completing it is the initiator's, then either hand the row over
|
||||
* (consume the state) or take back what the callback stored.
|
||||
*/
|
||||
async function completeApproved(
|
||||
request: Request,
|
||||
state: string | null,
|
||||
returnUrl: string,
|
||||
): Promise<Response> {
|
||||
const connectedUrl = `${returnUrl}&woocommerce_connected=true`
|
||||
|
||||
if (!state || !UUID_RE.test(state)) {
|
||||
// Nothing to bind against. The panel polls /status for the truth, and no
|
||||
// row is finalized by this route on its own.
|
||||
return NextResponse.redirect(connectedUrl)
|
||||
}
|
||||
|
||||
const supabase = await createServiceClient()
|
||||
|
||||
const { data: row, error: findError } = await supabase
|
||||
.from('woocommerce_connections')
|
||||
.select('id, user_id, status')
|
||||
.eq('oauth_state', state)
|
||||
.in('status', ['pending', 'active'])
|
||||
.single()
|
||||
|
||||
if (findError || !row) {
|
||||
// Already consumed (a re-visit of the return URL), superseded, or unknown:
|
||||
// there is nothing left to bind. The panel polls /status for the truth.
|
||||
return NextResponse.redirect(connectedUrl)
|
||||
}
|
||||
|
||||
const initiator = await requireFlowInitiator(request, row.user_id, {
|
||||
flow: 'woocommerce.return',
|
||||
})
|
||||
|
||||
if (!initiator.ok) {
|
||||
if (initiator.reason === 'no_session') {
|
||||
// Session expired mid-handshake: sign in and this route re-runs with
|
||||
// the same state. The row is untouched (it still carries the state).
|
||||
return initiator.response
|
||||
}
|
||||
// A different user completed it. The callback POST may already have
|
||||
// activated the row with the store's keys (it usually lands before this
|
||||
// redirect), so refusing means taking that back: keys wiped, state
|
||||
// consumed, row parked in 'error' with the reason. A still-pending row is
|
||||
// closed the same way so the late callback finds nothing to activate.
|
||||
const { error: revokeError } = await supabase
|
||||
.from('woocommerce_connections')
|
||||
.update({
|
||||
status: 'error',
|
||||
error_message: FLOW_INITIATOR_MISMATCH_MESSAGE,
|
||||
oauth_state: null,
|
||||
consumer_key_encrypted: null,
|
||||
consumer_secret_encrypted: null,
|
||||
})
|
||||
.eq('id', row.id)
|
||||
.in('status', ['pending', 'active'])
|
||||
if (revokeError) {
|
||||
log.error('failed to revoke connection completed by a non-initiator', {
|
||||
connectionId: row.id,
|
||||
code: revokeError.code,
|
||||
message: revokeError.message,
|
||||
})
|
||||
}
|
||||
return NextResponse.redirect(`${returnUrl}&woocommerce_error=wrong_user`)
|
||||
}
|
||||
|
||||
// Initiator confirmed. An active row has been fully handed over: consume the
|
||||
// state so the token cannot be presented again. A pending row keeps it: the
|
||||
// callback POST has not landed yet and still needs it to find the row.
|
||||
if (row.status === 'active') {
|
||||
const { error: consumeError } = await supabase
|
||||
.from('woocommerce_connections')
|
||||
.update({ oauth_state: null })
|
||||
.eq('id', row.id)
|
||||
.eq('status', 'active')
|
||||
if (consumeError) {
|
||||
log.warn('failed to consume oauth_state after handover', {
|
||||
connectionId: row.id,
|
||||
code: consumeError.code,
|
||||
message: consumeError.message,
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
return NextResponse.redirect(connectedUrl)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user