test(audit): move audit middleware logic to unit tests, one integration suite for live-stack cases by pfreixes · Pull Request #6952 · NangoHQ/nango

pfreixes · 2026-07-30T08:44:39Z

Production code is untouched (middleware/routes are unchanged) — this is a test-only reshape of how the audit middleware is covered.

Moves the middleware's logic coverage — event shape, name/URL redaction, actor resolution (user vs api_key), outcome mapping, the disabled-account gate, and resolve-before-next — into auditable.unit.test.ts: a container-free unit suite driving the real auditable() handler with a fake req/res and the audit client mocked.
Consolidates the cases that genuinely need the live stack into a single audit.integration.test.ts: wiring order (a denied request is still recorded — proves audit runs before authorization; probed once per wiring shape, public withScope + private can), resolve-before-next against a real controller mutation (pre-change role, removed-member email), and a real authorization rejection that must not leak a cross-account email.
Deletes the per-endpoint audit.private / audit.public integration files. For the merged first set: 10 integration tests in 2 files → 5 in one integration file + 7 unit tests.

Measured (same machine + configs)

	wall-clock	containers
before — 2 integration files (10 tests)	50.77s	yes
after — 1 integration file (5 tests)	32.84s	yes
after — unit file (7 tests)	tests 4ms (import ~20s, shared/amortized in the unit suite)	no

Integration wall-clock drops ~18s (−35%), driven almost entirely by removing one file's ~19s module-graph import (container boot is one-time and shared). The saving scales with integration files removed — the same pattern applies to the in-flight audit PRs (~6 audit integration files today), where consolidating toward one suite is the bulk of the win.

What stays integration, and why

Only where a fake can't honestly reproduce the behaviour: a real controller that mutates the row it audits (the pre-change role / removed-member email are only knowable if resolved before the handler ran), and a real authz rejection. Everything else is logic and moves to unit.

Test plan

unit suite (7) — green, no containers
consolidated integration suite (5) — green (verified in a working worktree; this branch re-runs it in CI)
confirm green in CI

POC for discussion — shifts audit coverage off per-endpoint integration tests: - auditWiring.ts: tag markers + a router-stack walker + checkers that assert every audit handler is installed after auth and before authz, and that every defined audit spec is wired to a route. - auditWiring.unit.test.ts / auditable.unit.test.ts: prove the checker bites and the middleware logic (event shape, redaction, outcome, resolve-before-next) unit-tests with no stack. - auditWiring.integration.test.ts: the same checks over the real router table (one test replacing the per-endpoint denial tests). - Minimal tagging on auditable()/can()/withScope(); apiAuth/webAuth exported. See proposals/audit-testing-strategy.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Move the pure-logic cases (deleted connection, connection update, env variables, webhook URLs) to auditable.unit.test.ts and drop the denied-role-change case (now covered by the structural wiring check). Keep only the three cases that need the live stack: target/metadata resolved from a real controller mutation (pre-change role, removed-member email) or a real authz rejection (cross-account). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

cubic-dev-ai

All reported issues were addressed

_{Reply with feedback, questions, or to request a fix.

Re-trigger cubic}

…live-stack cases Revert the structural-check machinery and middleware tagging. Instead: - auditable.unit.test.ts: event shape, redaction, actor resolution, outcome mapping, the disabled-account gate, resolve-before-next — off-stack, no containers. - audit.integration.test.ts: one suite for the cases that need the live stack — wiring order (a denied request is still recorded, probed per wiring shape), resolve-before-next against a real controller mutation, cross-account no-leak. - Delete audit.private/audit.public integration files (10 tests -> 5 in one integration file + 7 unit). Measured on the merged first set: integration wall-clock 50.77s -> 32.84s. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

TBonnin · 2026-07-30T10:02:39Z

+
+// Unit-level coverage of the middleware's behavior — the part the per-endpoint integration tests were
+// really asserting (event shape, redaction, outcome, resolve-before-next). No server, no auth stack,
+// no database, no ClickHouse: the audit client is mocked and a fake req/res drives the same code path.


not sure this comment adds much value

Removed in c3c3f5f — agreed, it was narration.

TBonnin · 2026-07-30T10:17:39Z

+        expect(JSON.stringify(event)).not.toContain('shh-secret');
+    });
+
+    it('maps a 4xx response to a denied outcome', async () => {


nit: this is only testing 403. Description is a bit of an overstatement

Fixed in c3c3f5f — it now covers both mappings (403 → denied, 5xx → failure) and the name reflects that.

TBonnin · 2026-07-30T10:19:05Z

-            resource: 'connection',
-            action: 'deleted',
-            outcome: 'success',
-            accountId: account.id,


nit: new test don't seem to assert the accountId

Good catch — the migrated tests dropped it. Added accountId (and environment) back to the unit tests in c3c3f5f.

TBonnin · 2026-07-30T10:24:08Z

+            await vi.waitFor(() => {
+                expect(auditSpy).toHaveBeenCalled();
+            });
+            expect(auditSpy.mock.calls[0]?.[0]).toMatchObject({ resource: 'member', action: 'role_changed', outcome: 'denied' });


should common attribute like environment be asserted?

Added in c3c3f5f — the deny probes now assert accountId + environment (null for account-scoped role_changed, {id,display} for the env-scoped connection event) + actor, and the same on the resolve-before-next / cross-account cases.

…common attrs - Drop the narration header comment in auditable.unit.test.ts. - Broaden the outcome test to cover 403->denied and 5xx->failure (was labelled 4xx but only tested 403). - Assert accountId + environment on the migrated/consolidated tests (parity with the deleted per-endpoint tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

pfreixes and others added 2 commits July 30, 2026 10:44

cubic-dev-ai Bot reviewed Jul 30, 2026

View reviewed changes

Comment thread packages/server/lib/middleware/auditWiring.ts Outdated

Comment thread packages/server/lib/middleware/auditWiring.ts Outdated

pfreixes marked this pull request as ready for review July 30, 2026 09:47

pfreixes changed the title ~~rfc(audit): structural wiring check to cut integration-test footprint (POC)~~ test(audit): unit-test the audit middleware, one integration suite for live-stack cases Jul 30, 2026

pfreixes requested a review from TBonnin July 30, 2026 09:50

pfreixes changed the title ~~test(audit): unit-test the audit middleware, one integration suite for live-stack cases~~ test(audit): move audit middleware logic to unit tests, one integration suite for live-stack cases Jul 30, 2026

TBonnin approved these changes Jul 30, 2026

View reviewed changes

pfreixes added this pull request to the merge queue Jul 30, 2026

Merged via the queue into master with commit 750f8f6 Jul 30, 2026
36 checks passed

pfreixes deleted the pau/audit-structural-poc branch July 30, 2026 11:05

Uh oh!

Conversation

pfreixes commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Measured (same machine + configs)

What stays integration, and why

Test plan

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pfreixes commented Jul 30, 2026 •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading