Skip to content

fix(billing): route Soroban simulation failures through the standard error envelope - #1429

Merged
greatest0fallt1me merged 3 commits into
CalloraOrg:mainfrom
iyanumajekodunmi756:fix/soroban-simulation-error-envelope
Oct 1, 2026
Merged

greatest0fallt1me merged 3 commits into
CalloraOrg:mainfrom
iyanumajekodunmi756:fix/soroban-simulation-error-envelope

Conversation

@iyanumajekodunmi756

Copy link
Copy Markdown
Contributor

Closes #1285

Summary

sendSimulationFailure and the SorobanRpcError catch branch in
src/routes/billing/deduct.ts wrote { error, code, simulationDetails }
directly to the response with res.status(502).json(...). That bypassed
buildErrorEnvelope and the request-id middleware, so this was the one failure
path in the billing API that a client could not parse with the standard
success / error / requestId / timestamp envelope — and support had no
requestId to correlate a simulation failure with.

Both paths now throw SimulationFailedError, a BadGatewayError (502)
carrying the canonical SIMULATION_FAILED code and a redacted simulation
summary, and let the global error handler render the envelope. console.warn is
replaced by logger.warn.

Affected modules

File Change
src/routes/billing/deduct.ts sendSimulationFailure replaced by simulationFailureError + logSimulationFailure; both 502-write paths now next(...) a SimulationFailedError; console.warn → logger.warn.
src/errors/index.ts New SimulationFailedError extends BadGatewayError that redacts in its constructor, plus an isSimulationFailedError guard.
src/errors/errorEnvelopePolicy.ts SIMULATION_FAILED added to PUBLIC_ERROR_CODES; new safeSimulationDetails whitelist; normalizeError accepts and sanitises simulation details.
src/middleware/envelope.ts errorEnvelopeSchema and buildErrorEnvelope gain an optional, additive error.simulationDetails; envelopeMiddleware preserves it for responses that emit the canonical shape directly.
src/middleware/errorHandler.ts Reads the redacted summary off SimulationFailedError and passes it through normalizeError / buildErrorEnvelope.
src/types/ResponseEnvelope.ts ErrorEnvelope documents the optional simulationDetails member.
src/routes/billing/deduct.test.ts Re-authenticated with real JWTs; added deterministic envelope/redaction/failure-mode cases.

Proposed state / invariant changes

  • SimulationFailedError is redacted at construction. Redaction lives in the
    constructor rather than at the call site, so it is impossible to construct this
    error with unredacted input. Cost: a caller cannot choose to publish raw
    diagnostics; that is intentional — raw RPC payloads contain account addresses,
    balances, XDR and signatures.
  • The envelope re-validates the summary against an explicit whitelist.
    safeSimulationDetails accepts only errorCode (string|finite number),
    errorMessage (string), eventCount (non-negative finite integer) and
    footprintPresent (boolean), and bounds each field. Anything else is dropped,
    so a future caller cannot widen the response body by accident.
  • The envelope key is additive and optional. error.simulationDetails is
    only present for simulation failures and is validated by the existing
    envelopeSchema, so existing clients that ignore unknown keys are unaffected.

Why SIMULATION_FAILED had to be whitelisted

normalizeError maps the error code through normalizePublicCode, which
substitutes publicCodeForStatus(502) = BAD_GATEWAY for any code that is not in
PUBLIC_ERROR_CODES. SIMULATION_FAILED was absent from that list, so throwing
the new error without adding it would have produced a correctly-shaped envelope
with the wrong code. SIMULATION_FAILED is a canonical entry in the error
catalog (src/errors/codes.ts, docs/openapi.json, generated from
docs/error-codes.yaml), so adding it restores the documented code rather than
inventing one.

Note: instanceof does not work for AppError subclasses

AppError's constructor calls Object.setPrototypeOf(this, AppError.prototype),
which severs the prototype chain of every subclass. Consequently
err instanceof SimulationFailedError is always false. isAppError already
works around this with an isAppError flag; SimulationFailedError mirrors that
pattern with an isSimulationFailedError marker. This is noted in the class
doc-comment because it is a trap for the next author.

Compatibility

  • Response shape change is additive only: one optional key inside error.
    The success / error.code / error.message / requestId / timestamp
    skeleton is untouched, and previously there was no envelope at all on this
    path, so nothing that parsed this response can regress.
  • HTTP status is unchanged (502); the code changes from a bare
    SIMULATION_FAILED string in a non-standard body to the same code in the
    standard envelope, which is the point of the issue.
  • The other SorobanRpcError categories (INSUFFICIENT_BALANCE → 402,
    TIMEOUT → 504, CONTRACT_ERROR/NETWORK_ERROR → 502) keep their existing
    mappings, asserted by a new test.
  • src/routes/billing.ts has a parallel sendSimulationFailure with the same
    shape. It is deliberately not touched here because the issue scopes the
    change to deduct.ts; it is an obvious follow-up.

Security and failure-mode handling

  • No diagnostic leakage. The response body is asserted not to contain the raw
    contract address, balance, contract id or secret seed from the input payload.
    Two independent redaction passes run (constructor, then envelope whitelist).
  • Server-side diagnosability retained. logger.warn still records the
    failure, with the same redacted summary, so operators keep the signal without
    the sensitive material. The logging helper receives the raw details and
    redacts them for the log, so eventCount / footprintPresent are preserved
    (re-redacting an already-redacted summary is lossy, which is why the log path
    and the response path deliberately redact different inputs).
  • No console.* in the route. Asserted by test, not just by inspection.
  • 503/502 semantics unchanged — the client contract for "upstream simulation
    failed" is still a 502, now machine-parseable.

Test strategy

deduct.test.ts previously authenticated with the x-user-id header. Since
requireAuth stopped trusting forwarded headers without a signed gateway
assertion (computeGatewaySignature / TRUST_FORWARDED_USER_ID), four of its
five cases were 401s that never reached the code under test — the suite was
green-by-accident-inverted, i.e. failing. It now mints real HS256 tokens with a
test JWT_SECRET, matching the pattern used by src/routes/credits.test.ts.

12 tests, all passing:

Test Covers
4 × developerId validation Existing behaviour, now actually exercised (null / empty / non-string / omitted).
returns 401 without auth Missing credentials.
returns 401 for an x-user-id header without an authenticated token Regression guard for the trusted-header removal.
returns the standard envelope with SIMULATION_FAILED and a requestId Acceptance criterion. Also asserts envelopeSchema.safeParse(body).success.
publishes only redacted simulation details Acceptance criterion. Deep-equals the redacted summary and asserts the raw address / balance / contract id / secret appear nowhere in the serialized body.
routes a SorobanRpcError carrying simulation details through the same envelope The catch branch, same assertions.
still maps non-simulation SorobanRpcError categories to their own codes 402 INSUFFICIENT_BALANCE, no simulationDetails.
keeps a plain deduction failure on PaymentRequiredError 402 BILLING_DEDUCTION_FAILED, no simulationDetails.
never writes simulation diagnostics to the console Acceptance criterion "no console.* remains".

Verification

Environment: Node 20.20.2 / npm 10.8.2 (matching CI's node-version: 20).

$ npm test -- src/routes/billing/deduct.test.ts src/lib/simulationDiagnostics.test.ts
Test Suites: 2 passed, 2 total
Tests:       15 passed, 15 total

Regression check against the unmodified baseline (measured by stashing this
branch's src/ changes and re-running the full suite):

before: 148 failed suites / 876 failed tests
after : 147 failed suites / 872 failed tests

The difference is exactly this suite going from 4 failures to 12 passes. No
new failures were introduced
— a diff of the failing-suite lists is empty in
the "newly failing" direction.

$ npx tsc --noEmit                       # 273 errors, unchanged from baseline; 0 in the files touched here
$ npx eslint <changed files>             # 0 problems

Prerequisite commits (included, clearly separated)

Both commits below are required for anything in this repository to build or
test, and are not part of the fix itself. They are separate commits so they can
be reviewed, cherry-picked or dropped independently.

  1. fix: restore accidentally deleted package.json and jest env setup —
    package.json and jest.env-setup.cjs were deleted by 599ab6e
    ("security: Clarify which tests need Postgres or containers (Clarify which tests need Postgres or containers #1338)"), a
    docs/test-scoping change that also dropped README.md. Without the manifest
    there is no npm ci, no npm run build, no npm test, and all CI jobs
    fail. Both files are restored verbatim from the commit before the deletion
    (95f3700).
  2. fix: restore deleted adminAuth middleware and repair broken module wiring
    — src/middleware/adminAuth.ts was deleted by 092ece9 while seven modules
    still import it, and three more files contain ESM-fatal defects (a re-export
    of a symbol that does not exist, a duplicated const declaration, and
    require.main inside an ES module). Full detail in that commit's message.

Known pre-existing failures (not caused by, and not fixed by, this PR)

main is currently mid-repair. The following are reproducible on a pristine
checkout and are out of scope for this issue:

  • npx tsc --noEmit reports 273 type errors (140 in production source across
    37 files). Upstream CI marks its typecheck and build steps
    continue-on-error: true for this reason.
  • The full Jest run has ~148 failing suites / ~876 failing tests on main.
  • scripts/check-migrations.ts — the only step in .github/workflows/ci.yml
    without continue-on-error — fails on main: Duplicate new migration prefix 24 (both 0024_hash_api_keys.sql and
    0024_idempotency_store_scope.sql are tracked) and Destructive migration "0024_idempotency_store_scope.sql" requires -- destructive-approved: #<issue>
    (it contains DROP CONSTRAINT / DROP INDEX). Because this job fails on
    main itself, it fails for every PR, including this one. Fixing it means
    renaming a migration and adding an approval marker, which is a migration-policy
    decision for the maintainers rather than a change to fold into this issue.
  • src/services/auditService.ts no longer exports AuditService /
    defaultAuditService although six modules import them, and
    src/routes/gatewayRoutes.ts references correlationMiddleware, env and
    CircuitBreakerOpenError that are not defined or imported. These need the
    deleted code restored and are not safely inferable.

Acceptance criteria mapping

Criterion Where
Simulation failures return the standard envelope with code SIMULATION_FAILED and requestId simulationFailureError → SimulationFailedError → errorHandler; asserted with envelopeSchema.safeParse and requestId equality
simulationDetails remain redacted SimulationFailedError constructor + safeSimulationDetails whitelist; leakage assertion over the serialized body
No console.* calls remain in deduct.ts Both paths use logger.warn; never writes simulation diagnostics to the console
Existing deduct tests are updated Re-authenticated with JWTs and extended from 5 to 12 tests
npm test -- src/routes/billing/deduct.test.ts src/lib/simulationDiagnostics.test.ts 2 suites / 15 tests passing

Non-goals respected

No typo-only or cosmetic changes, no unrelated refactors, no dependency
upgrades, and no validation weakened to make tests pass.

`package.json` and `jest.env-setup.cjs` were removed by commit 599ab6e
("security: Clarify which tests need Postgres or containers (CalloraOrg#1338)"), a
docs/test-scoping change that also dropped README.md. The result is that the
repository cannot build, lint, typecheck or run a single test on main: every
npm script is missing and `npm ci` fails outright, which also fails CI.

Restore both files verbatim from the commit before the deletion (95f3700).
`jest.config.cjs` still references `jest.env-setup.cjs` through
`setupFiles`, so test runs are broken without it as well.

README.md is intentionally not restored here: it is a 561-line document with
no effect on the build, and re-adding it does not belong in this change.
…elope

`sendSimulationFailure` and the `SorobanRpcError` catch branch in
`routes/billing/deduct.ts` wrote `{ error, code, simulationDetails }`
straight to the response with `res.status(502).json(...)`. That bypassed
`buildErrorEnvelope` and the request-id middleware, so this one failure path
was the only place a client could not parse with the standard
`success/error/requestId/timestamp` envelope, and support had no
`requestId` to correlate a simulation failure with.

Both paths now throw `SimulationFailedError` — a `BadGatewayError` (502)
carrying the canonical `SIMULATION_FAILED` code — and let the global error
handler render it. `console.warn` is replaced by `logger.warn`, which logs
the redacted summary so the diagnostic detail is still available
server-side.

Redaction is enforced in the error constructor, not at the call site: raw
RPC diagnostics contain account addresses, balances, XDR and signatures, so
constructing the error with unredacted input is impossible to get wrong.
`normalizeError` and `buildErrorEnvelope` then re-validate the summary
against an explicit four-field whitelist (`errorCode`, `errorMessage`,
`eventCount`, `footprintPresent`), so no future caller can widen the
response body by accident.

Supporting changes:
- `SIMULATION_FAILED` is added to `PUBLIC_ERROR_CODES`; without it
  `normalizePublicCode` would have downgraded the code to `BAD_GATEWAY`.
- `errorEnvelopeSchema` and `ErrorEnvelope` gain an optional, additive
  `error.simulationDetails`, and `envelopeMiddleware` preserves it for
  responses that emit the canonical shape directly.
- `SimulationFailedError` is detected by a marker flag rather than
  `instanceof`: `AppError` re-points `this` at `AppError.prototype`, which
  severs every subclass prototype, so `instanceof` is always false. This is
  the same reason `isAppError` uses a flag.

Tests: `deduct.test.ts` used the `x-user-id` header, which `requireAuth`
stopped trusting, so four of its five cases were 401s that never reached the
code under test. They now mint real HS256 tokens, and deterministic cases
were added for: the standard envelope with code/requestId, redaction (the
raw address, balance, contract id and secret must not appear anywhere in the
body), the `SorobanRpcError` path, unchanged mapping of the other
`SorobanRpcError` categories, the plain `PaymentRequiredError` path, and an
assertion that no `console.*` call is made. Suite went from 4 failures to
12 passing; the full repository test run improves by exactly this suite with
no new failures.
@drips-wave

drips-wave Bot commented Sep 30, 2026

Copy link
Copy Markdown

@iyanumajekodunmi756 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

# Conflicts:
#	src/middleware/errorHandler.ts
#	src/routes/billing/deduct.test.ts
#	src/routes/billing/deduct.ts
@greatest0fallt1me
greatest0fallt1me merged commit ff637a4 into CalloraOrg:main Oct 1, 2026
2 checks passed
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.

Route Soroban simulation failures through the error envelope

2 participants