Repository navigation
fix(middleware): make KYC access auditing deterministic at failure boundaries - #2819
Divine639700 wants to merge 3 commits into
Conversation
|
@Divine639700 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Hi maintainers — a note on the failing Backend CI check for this PR: the PR run stops during The lockfile is unchanged by this PR, and the failure reproduces on |
Description / Overview
KYC auditing could prevent a JSON response when a console sink failed, label a throwing response writer as successful, duplicate records when middleware was installed twice, and attribute authenticated access to a spoofed header. This change records each synchronous JSON attempt safely, preserves application responses/errors, and adds deterministic failure-boundary regressions.
Closes #2720
Type of Change
Changes Made
Files Modified
backend/src/middleware/access-log.ts: isolate metadata/console/audit failures, preserve the exact writer exception, classify the final synchronous status, deduplicate installation and field names, prefer the authenticated actor, and reject invalid runtime actions.backend/README.md: document synchronous audit scope, diagnostic codes, compatibility, privacy, bounded retention, and limitations.backend/scripts/lib/audit-utils.js: correct the severity constant used by the dependency scan.backend/scripts/.secret-scan-allow.json: update the documented development key's line after its source moved.New Files Added
backend/src/tests/kyc-access-log-boundaries.test.ts: 38 deterministic cases covering success, denial, invalid inputs, capacity/status boundaries, duplicate installation, retries, simultaneous failures, getters, and concurrent request contexts.Key Changes
req.actortakes precedence over the legacyx-user-idaudit hint.read,write,update, ordelete.Testing / Verification Results
npx jest --runInBand src/tests/kyc-access-log-boundaries.test.ts src/tests/access-log.test.ts src/tests/apiKeyAuth.test.ts: 67/67 tests passed, including all 38 new cases, 20 existing access-log cases, and 9 existing auth cases. On Windows this usednode_modules/.bin/jest.cmddirectly.npm run test:coverage -- --runInBand: 51 suites passed / 20 failed; 1,282 tests passed / 73 failed. This run included 35 new cases; the three subsequently added retention/combined-failure/downstream-error cases passed in the final focused run. The configured event-ingest coverage gate also failed. An isolated unchangedupstream/mainworktree produced 50 suites passed / the same 20 failed; 1,247 tests passed / the same 73 failed (identical failing suite names), with the same event-ingest coverage failure. The patched run added 35 passing tests and no failing suites.npx tsc --noEmit: fails on the same four errors observed before editing, in unchangedsrc/services/api-key-service.ts: missingcypto, missing./api-key-errors, and twodbKeyreferences wheredbkeyis defined.npm audit --jsonandnode scripts/dependency-scan.js <local-audit-report> high: existing lockfile reports 4 high and 2 moderate findings; the high gate fails. No dependencies were changed.node scripts/secret-scan.jsfailed on an existing fixture insrc/tests/conditional-write.test.ts:386; no new finding or credentials were added.node scripts/generate-sbom.js <local-output>: cannot run on this Windows shell because its bareexecFileSync("npx", ...)resolves toENOENT.git diff --check: passed.mainto resolve the conflict, the expanded access-log, KYC boundary, and API-key auth suites passed: 127/127 tests, 3 suites. This run used a local-only Jest shim to bypass a duplicateDatabaseConstructordeclaration in unchanged upstreambackend/src/lib/database.ts; without that shim, the two access-log suites fail during import. The shim is not part of the PR.tsc --noEmitis blocked by parse errors in unchanged upstreamentityId.test.ts,migrations/policy.ts, andmodels/api-key.ts.71b235ed, audit utility assertions pass;node scripts/dependency-scan.js <saved-audit-report> criticalpasses with 0 critical findings, andnode scripts/secret-scan.jspasses. The saved report is historical; the current GitHub workflow does not run this scan.Acceptance Criteria / Test Coverage
npm cibecause the backend lockfile is out of sync withpackage.json; the latest upstreammainrun fails on the same missing lockfile entries. Prior local broad test/typecheck findings are disclosed above.Contract-Specific Checks / Contract Testing Details
Not applicable to this backend middleware change; no contract, WASM, frontend, or on-chain interface was changed. The current GitHub contract build passes. Contract/WASM and frontend checks were not run locally. The PR is ready for review with the backend CI blocker disclosed below.
Review Checklist
Code Quality
No dependency changes, broad refactors, generated artifacts, or weakened test/coverage/security configuration. Rust clippy/rustfmt checks do not apply to these TypeScript changes.
Performance & Security
Bounded retention remains 10,000 records; duplicate installation uses a weak response map. Authorization remains downstream. The in-memory audit store retains existing identity/IP metadata and must be protected; it is not durable storage. If all audit sinks fail, a response/error is preserved but an audit record may be unavailable. JSON auditing confirms synchronous writer behavior, not delivery over the network or non-JSON responses.
Documentation
README and code comments describe the failure contracts and compatibility changes. No endpoint/schema migration or changelog change is needed.
Related Issues
Closes #2720
Additional Notes
Prepared and published as assigned scout Divine639700. Broad backend failures and security findings are not presented as passing checks.
Current GitHub checks at
71b235ed: contract build passes. Backend Type check, lint, and tests fails duringnpm cibecausebackend/package-lock.jsonlacks packages required bybackend/package.json(starting withajv). The latest upstreammainBackend CI run fails at the same install step. The PR does not change dependencies or the lockfile. This PR also corrects the earlier security scan's undefinedSEFERITY_ORDERreferences and a stale line in the documented development-key allowlist; both scan stages pass locally with the saved audit report. The current upstream workflow has replaced that security scan with a typecheck/test job, so these fixes do not resolve the present lockfile failure.How to Test
backend, install the lockfile dependencies usingnpm ci.npx jest --runInBand src/tests/kyc-access-log-boundaries.test.ts src/tests/access-log.test.ts src/tests/apiKeyAuth.test.ts.npx tsc --noEmitandnpm run test:coverage -- --runInBandto inspect the disclosed repository blockers.Screenshots
Not applicable: no UI changes.
Breaking Changes / Migration Steps
No typed API migration. Untyped callers must supply a supported action. Consumers parsing console summaries should use structured audit records instead; console summaries now omit identity/IP values and include outcome.
Reviewer Checklist