From 80b87c55fcb6b598b4212c2b2ddd74513ed055e6 Mon Sep 17 00:00:00 2001 From: Mattsson <111893710+mattssonn@users.noreply.github.com> Date: Thu, 3 Sep 2026 17:47:02 +0200 Subject: [PATCH] fix(skatteverket): treat AGI kvittens gateway refusals as errors and name the connector operator (#2234) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(skatteverket): treat AGI kvittens gateway refusals as errors and name the connector operator The AGI kvittens cron bucketed ACCESS_DENIED as apigw_config with a warn-once suppression because the APIGW client was known to lack the AGI hantera subscription in Utvecklarportalen (#963). With that subscription being put in place (#2226), a gateway refusal is a regression and belongs in the ordinary error path, so the bucket, its "known configuration gap" comment and the apigwConfig response field are gone. The connector-mode gateway-refusal message said "kontakta supporten"; hosted is now itself a Connect installation for the canary companies, so the message names the connector operator by host instead. Refs #2226 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0159Mi1sTAHfUDZkSysntYz2 * fix(skatteverket): keep the ACCESS_DENIED code in the kvittens cron body Skeptic finding on PR #2234: the generic error path ran the gateway refusal through getErrorMessage, whose Swedish keyword heuristic misses the gateway wording and collapsed it into "Något gick fel". Echo the machine-readable code instead, as the expired_token and grant_revoked rows already do; the full guidance stays in the error log. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0159Mi1sTAHfUDZkSysntYz2 --------- Co-authored-by: Claude Fable 5.1 --- DECISIONS.md | 1 + .../kvittenser/cron/__tests__/route.test.ts | 84 ++++--------------- .../skatteverket/agi/kvittenser/cron/route.ts | 53 +++++------- .../__tests__/api-client-connector.test.ts | 6 ++ .../general/skatteverket/lib/api-client.ts | 37 +++++--- 5 files changed, 70 insertions(+), 111 deletions(-) diff --git a/DECISIONS.md b/DECISIONS.md index 86d0e7c6..77b8cf5d 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -1527,3 +1527,4 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-09-03] Old-address social identities are unlinked by a BEFORE UPDATE trigger on auth.users (migration 20260903110000), not by the /auth/callback done path: the callback never runs for a completing click from a browser without a session, and admin-side changes bypass it entirely; the trigger covers every path and keeps the email identity, password and BankID intact. [2026-09-03] AGI redovisningsperiod = the payout month (agiReportingPeriod on payment_date), not salary_runs.period_*: Skatteverket files per the month the pay went out (kontantprincipen), so lön i efterskott (August work paid 25 September) is declared in September. The in-period payment-date guard (dashboard PATCH, lib/salary/update-run.ts, v1 PATCH, RunHeader min/max) is lifted rather than widened: its only stated reason was that the AGI keyed on period_*, and any residual month window would bite the next efterskott variant. Existing agi_declarations rows keep their stored period (no backfill): a declaration already filed under the earned month is a real-world correction with Skatteverket, not a re-key. New AGI_PERIOD_CONFLICT (409) refuses to overwrite a live run's declaration for the same payout month, since one month's AGI must cover every payment that month and the generator cannot merge runs. Issue #2191. [2026-09-03] The cursor:// deeplink is its own allowlist provider (cursor_deeplink) rendered "Din egen dator" and never "Verifierad", after the skeptic, CodeRabbit and Superagent all made the same point: a custom scheme can be claimed by any local app (RFC 8252 section 8.4), so it carries loopback trust, not vendor trust, and the consent page must not say otherwise; https://www.cursor.com/... keeps the verified label. Same pass fixed the consent-page CSP for custom schemes: new URL('cursor://...').origin is the string "null", so form-action became `'self' null` and Chromium would have blocked the post-consent 303 (correctness skeptic refutation); the header now uses the scheme-source (`cursor:`) when the origin is opaque. Not done: rejecting a missing code_challenge at /authorize. A code minted without one is unexchangeable (verifyPkce against an empty challenge is always false, now pinned by a test), so it is fail-closed; making it fail earlier is a separate change touching every client. +[2026-09-03] AGI kvittens cron: dropped the apigw_config bucket (#963) and its warn-once suppression for ACCESS_DENIED (#2226). The bucket existed because the APIGW client was known to lack the AGI hantera subscription in Utvecklarportalen; with that subscription expected in place, a gateway refusal is a regression and belongs in the ordinary error path (error level, generic 'error' status) rather than a status that hides it as a known gap. Same pass: the connector-mode gateway-refusal message names the connector operator by host instead of "kontakta supporten", because hosted is itself a Connect installation for the canary companies and "support" no longer says whose. Merging before the portal subscription is active means the 15-minute cron logs at error level per pending declaration until it is. diff --git a/app/api/extensions/skatteverket/agi/kvittenser/cron/__tests__/route.test.ts b/app/api/extensions/skatteverket/agi/kvittenser/cron/__tests__/route.test.ts index d92f0cd7..2fd1ec2c 100644 --- a/app/api/extensions/skatteverket/agi/kvittenser/cron/__tests__/route.test.ts +++ b/app/api/extensions/skatteverket/agi/kvittenser/cron/__tests__/route.test.ts @@ -16,7 +16,7 @@ vi.mock('@/lib/auth/cron', () => ({ // The route and the reconcile helper log through '@/lib/logger'. The real // logger suppresses info/warn under NODE_ENV=test, so warn/error output is // observed via these recorders instead of console spies. Console spies stay -// for the route's remaining console.* lines (APIGW warn, summary, skip). +// for the route's remaining console.* lines (summary, skip). const { warnRecorder, errorRecorder } = vi.hoisted(() => ({ warnRecorder: vi.fn(), errorRecorder: vi.fn(), @@ -304,7 +304,6 @@ describe('AGI kvittenser cron', () => { expect(body.processed).toBe(1) expect(body.grantRevoked).toBe(1) expect(body.errors).toBe(0) - expect(body.apigwConfig).toBe(0) expect(body.results[0]).toMatchObject({ status: 'grant_revoked', error: 'OMBUD_GRANT_MISSING' }) expect(mockMarkGrantRevoked).toHaveBeenCalledWith('comp-1', expect.any(String), 'lasombud', 'OMBUD_GRANT_MISSING') @@ -312,7 +311,7 @@ describe('AGI kvittenser cron', () => { expect(summaryLine).toContain('1 grants revoked') }) - it('logs a warn (not error) and records apigw_config on ACCESS_DENIED', async () => { + it('treats ACCESS_DENIED as a real error now that the APIGW subscription is expected (#2226)', async () => { mockCreateClient.mockReturnValueOnce(stubHappyTables()) mockAgiGetKvittenser.mockRejectedValueOnce( new SkatteverketAuthError('Skatteverkets API-gateway nekade anropet.', 'ACCESS_DENIED'), @@ -322,80 +321,35 @@ describe('AGI kvittenser cron', () => { const body = await res.json() expect(body.processed).toBe(1) - expect(body.apigwConfig).toBe(1) - expect(body.errors).toBe(0) + expect(body.errors).toBe(1) + // The old known-gap bucket is gone from the response contract. + expect(body).not.toHaveProperty('apigwConfig') expect(body.results[0]).toMatchObject({ declarationId: 'decl-1', - status: 'apigw_config', + status: 'error', + // Machine-readable code, never the generic "Något gick fel" fallback. error: 'ACCESS_DENIED', }) // companyId is internal log context, never response payload. expect(body.results[0]).not.toHaveProperty('companyId') - // Warn carries the actionable config hint plus the context to act on it. - expect(warnSpy).toHaveBeenCalledTimes(1) - const [warnMessage, warnContext] = warnSpy.mock.calls[0] - expect(warnMessage).toContain('Utvecklarportalen') - expect(warnMessage).toContain('SKATTEVERKET_APIGW_CLIENT_ID') - expect(warnContext).toMatchObject({ + // A gateway refusal is a regression, not a known gap: error level with + // the gateway's message in context, no warn-once suppression, no + // reconsent flagging. + expect(errorRecorder).toHaveBeenCalledTimes(1) + expect(String(errorRecorder.mock.calls[0][0])).toContain('Reconciliation failed') + expect(errorRecorder.mock.calls[0][1]).toMatchObject({ declarationId: 'decl-1', companyId: 'comp-1', - period: expect.any(String), + message: 'Skatteverkets API-gateway nekade anropet.', }) - - // The whole point: no error-level log for a config gap retries cannot heal. - expect(errorSpy).not.toHaveBeenCalled() - expect(errorRecorder).not.toHaveBeenCalled() + expect(warnSpy).not.toHaveBeenCalled() + expect(warnRecorder).not.toHaveBeenCalled() expect(mockMarkNeedsReconsent).not.toHaveBeenCalled() - // The config gap stays visible in the run summary. const summaryLine = logSpy.mock.calls.map(c => String(c[0])).find(m => m.includes('Processed')) - expect(summaryLine).toContain('1 apigw config gaps') - }) - - it('warns once per run on ACCESS_DENIED but records apigw_config for every declaration', async () => { - mockCreateClient.mockReturnValueOnce( - makeSupabaseStub({ - agi_declarations: { - data: [ - PENDING_DECLARATION, - { ...PENDING_DECLARATION, id: 'decl-2', company_id: 'comp-2' }, - { ...PENDING_DECLARATION, id: 'decl-3', company_id: 'comp-3' }, - ], - }, - skatteverket_tokens: { data: [{ user_id: 'user-1', status: 'active' }] }, - company_settings: { data: { org_number: '556123-4567', entity_type: 'aktiebolag' } }, - }), - ) - for (let i = 0; i < 3; i++) { - mockAgiGetKvittenser.mockRejectedValueOnce( - new SkatteverketAuthError('Skatteverkets API-gateway nekade anropet.', 'ACCESS_DENIED'), - ) - } - - const res = await GET(makeRequest()) - const body = await res.json() - - // Every affected declaration still gets its apigw_config outcome. - expect(body.processed).toBe(3) - expect(body.apigwConfig).toBe(3) - expect(body.errors).toBe(0) - expect(body.results.map((r: { declarationId: string }) => r.declarationId)).toEqual([ - 'decl-1', - 'decl-2', - 'decl-3', - ]) - expect(body.results.every((r: { status: string }) => r.status === 'apigw_config')).toBe(true) - expect(body.results.every((r: Record) => !('companyId' in r))).toBe(true) - - // But the identical config-gap warning is logged exactly once per run. - expect(warnSpy).toHaveBeenCalledTimes(1) - expect(String(warnSpy.mock.calls[0][0])).toContain('Utvecklarportalen') - expect(errorSpy).not.toHaveBeenCalled() - expect(errorRecorder).not.toHaveBeenCalled() - - const summaryLine = logSpy.mock.calls.map(c => String(c[0])).find(m => m.includes('Processed')) - expect(summaryLine).toContain('3 apigw config gaps') + expect(summaryLine).toContain('1 errors') + expect(summaryLine).not.toContain('apigw') }) it('still flags reconsent codes as expired_token and marks the connection', async () => { @@ -408,7 +362,6 @@ describe('AGI kvittenser cron', () => { const body = await res.json() expect(body.expired).toBe(1) - expect(body.apigwConfig).toBe(0) expect(body.results[0]).toMatchObject({ status: 'expired_token', error: 'SESSION_EXPIRED' }) expect(mockMarkNeedsReconsent).toHaveBeenCalledWith(expect.anything(), 'user-1', 'comp-1', 'SESSION_EXPIRED') expect(errorSpy).not.toHaveBeenCalled() @@ -443,7 +396,6 @@ describe('AGI kvittenser cron', () => { const body = await res.json() expect(body.errors).toBe(1) - expect(body.apigwConfig).toBe(0) expect(body.results[0]).toMatchObject({ status: 'error', error: 'Du har inte behörighet.' }) expect(errorRecorder).toHaveBeenCalledTimes(1) expect(String(errorRecorder.mock.calls[0][0])).toContain('Reconciliation failed') diff --git a/app/api/extensions/skatteverket/agi/kvittenser/cron/route.ts b/app/api/extensions/skatteverket/agi/kvittenser/cron/route.ts index 08837123..9d018038 100644 --- a/app/api/extensions/skatteverket/agi/kvittenser/cron/route.ts +++ b/app/api/extensions/skatteverket/agi/kvittenser/cron/route.ts @@ -18,8 +18,8 @@ ensureInitialized() export const maxDuration = 60 // Failure logs route through the structured logger so third-party error -// strings pass its redaction. The APIGW warn / budget + summary logs / -// capability skip stay on console.*: their content is fixed internal strings. +// strings pass its redaction. The budget + summary logs / capability skip +// stay on console.*: their content is fixed internal strings. const log = createLogger('agi-kvittenser-cron') // Cron responses must never be cached: they report a point-in-time run. @@ -104,14 +104,10 @@ export async function GET(request: Request) { type Result = { declarationId: string period: string - status: 'signed' | 'still_pending' | 'already_claimed' | 'no_token' | 'no_company_settings' | 'expired_token' | 'grant_revoked' | 'apigw_config' | 'error' + status: 'signed' | 'still_pending' | 'already_claimed' | 'no_token' | 'no_company_settings' | 'expired_token' | 'grant_revoked' | 'error' error?: string } const results: Result[] = [] - // The APIGW subscription gap is one run-level configuration problem, not a - // per-declaration one: warn once per run instead of spamming an identical - // warning for every affected declaration. - let apigwAccessDeniedWarned = false for (const decl of pending) { if (Date.now() - startTime > TIME_BUDGET_MS) { @@ -181,30 +177,23 @@ export async function GET(request: Request) { results.push({ declarationId, period, status: 'expired_token', error: err.code }) continue } - if (err instanceof SkatteverketAuthError && err.code === 'ACCESS_DENIED') { - // Skatteverkets API gateway rejected our client credentials before - // the user's bearer was ever evaluated: the APIGW client - // (SKATTEVERKET_APIGW_CLIENT_ID) lacks an Utvecklarportalen - // subscription for the AGI hantera API. Retrying every run cannot - // heal this and the user reconnecting via BankID does not help, so - // log at warn level instead of error to keep the 2h cron from - // producing error-noise for a known configuration gap. The distinct - // status keeps the gap visible in the run summary until fixed. The - // warn is emitted once per run (context is the first affected - // declaration); every affected declaration still lands in results. - if (!apigwAccessDeniedWarned) { - apigwAccessDeniedWarned = true - console.warn( - '[agi-kvittenser-cron] APIGW client lacks Utvecklarportalen subscription for the AGI hantera API; check SKATTEVERKET_APIGW_CLIENT_ID subscriptions. Skipping affected declarations until the subscription is added.', - { declarationId, companyId, period, message }, - ) - } - results.push({ declarationId, period, status: 'apigw_config', error: err.code }) - continue - } - + // ACCESS_DENIED (Skatteverket's gateway refusing the APIGW client) is + // no longer bucketed as a known configuration gap: the AGI hantera + // subscription is expected to be in place (#2226), so a gateway refusal + // is a real regression and lands in the error path below like any + // other failure. log.error('Reconciliation failed', { declarationId, companyId, period, message }) - results.push({ declarationId, period, status: 'error', error: getErrorMessage(err) }) + // A gateway refusal keeps its machine-readable code in the body (as the + // expired_token / grant_revoked rows do): the keyword heuristic in + // getErrorMessage misses the gateway wording and would collapse it + // into "Något gick fel". The full guidance is in the error log above. + const isGatewayRefusal = err instanceof SkatteverketAuthError && err.code === 'ACCESS_DENIED' + results.push({ + declarationId, + period, + status: 'error', + error: isGatewayRefusal ? err.code : getErrorMessage(err), + }) } } @@ -213,11 +202,10 @@ export async function GET(request: Request) { const alreadyClaimed = results.filter(r => r.status === 'already_claimed').length const expired = results.filter(r => r.status === 'expired_token').length const grantRevoked = results.filter(r => r.status === 'grant_revoked').length - const apigwConfig = results.filter(r => r.status === 'apigw_config').length const errors = results.filter(r => r.status === 'error').length console.log( - `[agi-kvittenser-cron] Processed ${results.length}: ${signed} signed, ${stillPending} still pending, ${alreadyClaimed} already claimed, ${expired} expired, ${grantRevoked} grants revoked, ${apigwConfig} apigw config gaps, ${errors} errors`, + `[agi-kvittenser-cron] Processed ${results.length}: ${signed} signed, ${stillPending} still pending, ${alreadyClaimed} already claimed, ${expired} expired, ${grantRevoked} grants revoked, ${errors} errors`, ) return NextResponse.json( @@ -228,7 +216,6 @@ export async function GET(request: Request) { alreadyClaimed, expired, grantRevoked, - apigwConfig, errors, results, }, diff --git a/extensions/general/skatteverket/__tests__/api-client-connector.test.ts b/extensions/general/skatteverket/__tests__/api-client-connector.test.ts index 624ca030..de242061 100644 --- a/extensions/general/skatteverket/__tests__/api-client-connector.test.ts +++ b/extensions/general/skatteverket/__tests__/api-client-connector.test.ts @@ -185,6 +185,10 @@ describe('skvRequestWithAuth: connector mode', () => { expect((e as SkatteverketAuthError).code).toBe('ACCESS_DENIED') const { message } = e as SkatteverketAuthError expect(message).toMatch(/connectorn/) + // Points at the connector's operator by host, not at an ambiguous + // "support": hosted is itself a Connect installation (#2226). + expect(message).toMatch(/operatör \(app\.hosted\.example\)/) + expect(message).not.toMatch(/supporten/) expect(message).not.toMatch(/SKATTEVERKET_APIGW_CLIENT_ID|Utvecklarportalen/) } }) @@ -198,6 +202,8 @@ describe('skvRequestWithAuth: connector mode', () => { expect((e as SkatteverketAuthError).code).toBe('ACCESS_DENIED') const { message } = e as SkatteverketAuthError expect(message).toMatch(/connectorn/) + expect(message).toMatch(/operatör \(app\.hosted\.example\)/) + expect(message).not.toMatch(/supporten/) expect(message).not.toMatch(/Utvecklarportalen/) } }) diff --git a/extensions/general/skatteverket/lib/api-client.ts b/extensions/general/skatteverket/lib/api-client.ts index 7461bdc1..609f3053 100644 --- a/extensions/general/skatteverket/lib/api-client.ts +++ b/extensions/general/skatteverket/lib/api-client.ts @@ -317,20 +317,33 @@ function apigwOrScopeMessage(url: string): string { /** * Connector-mode variant of the gateway-refusal guidance: the APIGW client - * and its subscriptions belong to the HOSTED broker (Arcim), so telling a - * self-host operator to check SKATTEVERKET_APIGW_CLIENT_ID or visit - * Utvecklarportalen points at knobs their instance does not have. The token - * they hold was also minted by the broker, so the only local actions are - * checking the connector status and contacting support. + * and its subscriptions belong to the connector's OPERATOR (the broker the + * instance routes through), so telling a self-host operator to check + * SKATTEVERKET_APIGW_CLIENT_ID or visit Utvecklarportalen points at knobs + * their instance does not have. The token they hold was also minted by the + * broker, so the only local actions are checking the connector status and + * contacting the operator. The message names the operator by the connector + * host rather than saying "support": hosted is itself a Connect + * installation for the canary companies (#2209), so "support" no longer + * says whose (#2226). */ -function connectorGatewayMessage(url: string): string { +function connectorGatewayMessage(url: string, connectBaseUrl: string): string { return ( `Skatteverkets API-gateway nekade anropet till tjänsten "${apiHintFromUrl(url)}" via connectorn. ` + - 'Detta är ett konfigurationsproblem på värdtjänstens sida (gateway-prenumeration eller scope), ' + - 'inte på din instans: kontakta supporten. Anslutningsläget syns på /api/connector/status.' + `Felet ligger hos connectorns operatör (${connectorHost(connectBaseUrl)}), i gateway-prenumerationen ` + + 'eller scope-listan, inte på din instans: kontakta operatören. Anslutningsläget syns på /api/connector/status.' ) } +/** The connector operator's host, for messages that have to say WHOM to contact. */ +function connectorHost(connectBaseUrl: string): string { + try { + return new URL(connectBaseUrl).host + } catch { + return connectBaseUrl + } +} + /** * A genuine token-scope rejection: the stored access token predates a scope * the service now requires, and only a fresh consent can widen it. @@ -567,7 +580,7 @@ export async function skvRequestWithAuth( // so either verdict re-arms the banner the user just tried to clear. if (isApigwScopeContractError(text)) { throw new SkatteverketAuthError( - connector ? connectorGatewayMessage(url) : apigwOrScopeMessage(url), + connector ? connectorGatewayMessage(url, connector.baseUrl) : apigwOrScopeMessage(url), 'ACCESS_DENIED' ) } @@ -631,7 +644,7 @@ export async function skvRequestWithAuth( // Connector mode: the gateway client is the broker's, not the instance's. throw new SkatteverketAuthError( connector - ? connectorGatewayMessage(url) + ? connectorGatewayMessage(url, connector.baseUrl) : `Skatteverkets API-gateway nekade anropet till "${apiHintFromUrl(url)}". ` + 'Kontrollera att din APIGW-klient (SKATTEVERKET_APIGW_CLIENT_ID) har ' + 'prenumeration på denna tjänst i Utvecklarportalen.', @@ -649,7 +662,7 @@ export async function skvRequestWithAuth( // likely fix instead. if (!text) { if (connector) { - throw new SkatteverketAuthError(connectorGatewayMessage(url), 'ACCESS_DENIED') + throw new SkatteverketAuthError(connectorGatewayMessage(url, connector.baseUrl), 'ACCESS_DENIED') } const apiHint = apiHintFromUrl(url) throw new SkatteverketAuthError( @@ -717,7 +730,7 @@ export async function skvRequestWithAuth( // scope actually exists, so it can never be automatic. if (isApigwScopeContractError(text)) { throw new SkatteverketAuthError( - connector ? connectorGatewayMessage(url) : apigwOrScopeMessage(url), + connector ? connectorGatewayMessage(url, connector.baseUrl) : apigwOrScopeMessage(url), 'ACCESS_DENIED' ) }