Skip to content

fix(middleware): make KYC access auditing deterministic at failure boundaries - #2819

Open
Divine639700 wants to merge 3 commits into
QuickLendX:mainfrom
Divine639700:fix/2720-kyc-access-log-boundaries
Open

Divine639700 wants to merge 3 commits into
QuickLendX:mainfrom
Divine639700:fix/2720-kyc-access-log-boundaries

Conversation

@Divine639700

@Divine639700 Divine639700 commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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

  • Bug fix
  • Documentation update
  • Security enhancement
  • Breaking 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

  • One record per JSON attempt, including failed attempts and retries; one wrapper per response/resource/action.
  • Logging never authenticates a caller or bypasses downstream authorization. A verified req.actor takes precedence over the legacy x-user-id audit hint.
  • Inspect names without reading KYC values. Keep credentials hashed and identities/IPs out of console summaries. Emit fixed failure codes without caught exception messages.
  • Preserve the existing exports, typed action union, response body/reference/return value, and newest-10,000 retention contract. Untyped unsupported actions now fail at construction; callers must use read, write, update, or delete.

Testing / Verification Results

  • Baseline reproduction: the initial 33-case regression suite had 17 failures against the original middleware.
  • 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 used node_modules/.bin/jest.cmd directly.
  • 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 unchanged upstream/main worktree 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 unchanged src/services/api-key-service.ts: missing cypto, missing ./api-key-errors, and two dbKey references where dbkey is defined.
  • npm audit --json and node 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.
  • At initial publication, node scripts/secret-scan.js failed on an existing fixture in src/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 bare execFileSync("npx", ...) resolves to ENOENT.
  • Focused coverage report (command-line collection only, without changing repository gates): access-log.ts 99.04% statements, 97.22% branches, 100% functions and lines.
  • git diff --check: passed.
  • After merging current main to 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 duplicate DatabaseConstructor declaration in unchanged upstream backend/src/lib/database.ts; without that shim, the two access-log suites fail during import. The shim is not part of the PR.
  • On the merged tree, tsc --noEmit is blocked by parse errors in unchanged upstream entityId.test.ts, migrations/policy.ts, and models/api-key.ts.
  • After commit 71b235ed, audit utility assertions pass; node scripts/dependency-scan.js <saved-audit-report> critical passes with 0 critical findings, and node scripts/secret-scan.js passes. The saved report is historical; the current GitHub workflow does not run this scan.

Acceptance Criteria / Test Coverage

Criterion Implementation and observed evidence
Valid, invalid, duplicate, boundary inputs Four actions; six unsupported runtime actions; scalar/null/array bodies; malformed headers; nine status boundaries; duplicate installation and 10,000-record capacity tests.
Authorization / validation / transitions Real Express + API-key middleware rejects an unauthorized request, preserves the handler boundary, and attributes an authorized request to the verified actor. Final writer status and downstream errors are tested.
Retries / partial failure / concurrency Exact original writer error survives audit failure; retry records remain distinct; sink/metadata failures recover; asynchronous barriers prove late responses retain their own correlation/action/status.
Focused regression coverage 67 focused tests passed; 17 initial regression failures reproduced before the fix.
Compatibility Existing 20 access-log and 9 API-key tests pass; public exports and typed signatures remain unchanged. Runtime invalid-action behavior and console format changes are documented.
Safe diagnostics Static failure codes; no KYC values or bearer keys in captured audit/console surfaces; authenticated actor beats spoofed identity hints.
Repository CI Contract build passes. The current backend CI job fails at npm ci because the backend lockfile is out of sync with package.json; the latest upstream main run 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

  • Project style followed; changes scoped to KYC auditing.
  • Documentation and invariant comments updated.
  • Sensitive values omitted from new diagnostics.
  • Failure handling and edge cases tested.
  • All repository checks pass (see observed failures above).

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 during npm ci because backend/package-lock.json lacks packages required by backend/package.json (starting with ajv). The latest upstream main Backend 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 undefined SEFERITY_ORDER references 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

  1. In backend, install the lockfile dependencies using npm ci.
  2. Run npx jest --runInBand src/tests/kyc-access-log-boundaries.test.ts src/tests/access-log.test.ts src/tests/apiKeyAuth.test.ts.
  3. Run npx tsc --noEmit and npm run test:coverage -- --runInBand to 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

  • Confirm per-attempt auditing and original error/response preservation.
  • Review authorization attribution and safe diagnostics.
  • Review deterministic failure, retry, capacity, and concurrency tests.
  • Confirm documentation and compatibility expectations.
  • Resolve existing repository CI blockers before merge.

@drips-wave

drips-wave Bot commented Sep 29, 2026

Copy link
Copy Markdown

@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! 🚀

Learn more about application limits

@Divine639700
Divine639700 marked this pull request as ready for review October 1, 2026 14:05
@Divine639700

Copy link
Copy Markdown
Contributor Author

Hi maintainers — a note on the failing Backend CI check for this PR: the PR run stops during npm ci because backend/package-lock.json is out of sync with backend/package.json (the first missing entry is ajv@8.20.0). The latest upstream main run fails at the same install step with the same missing entry.

The lockfile is unchanged by this PR, and the failure reproduces on main. The contract build passes. Please let me know whether you would prefer the backend lockfile fixed separately upstream or addressed here before review. Thanks!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add deterministic failure-boundary coverage for kycAccessLogMiddleware in ./backend/src/middleware/access-log.ts

1 participant