fix(api): sanitize failures from the OpenAPI spec route - #852
Merged
Merged
Conversation
`GET /scalar/json` was the one route `errorHandler` never classified, so a failed document generation answered with the raw exception message as `detail` on an unauthenticated endpoint. Two conditions had to line up. `openapi()` is registered on the outer app before `.use(api)`, while `errorHandler` lives inside the `api` instance — and a `global` hook only reaches routes declared after it. `@elysia/openapi` then puts a local `error` hook on the spec route that logs and returns nothing, consuming the failure before the later-registered arms see it, so Elysia falls back to its built-in renderer. Neither alone leaks; only the combination does. Seat the guard next to the document amendment instead, which already has to precede the plugin for the same reach reason. Moving `errorHandler` forward was not an option: the frontend `NotFound` arm is deliberately seated ahead of it, and Elysia stops at the first handler that returns something. Two adjacent fixes fall out of it: - The amendment hook gated only on "path is the spec path" and "plain object", so the sanitized error body — a plain object served from that path — would have come back merged with `components.schemas`. It now amends only a successful document, gating on the status rather than on the error shape so it stays uncoupled from the error contract. - A spec failure reaches the negotiator, so it answers `problem+json` with `Vary: Accept` like every other failure rather than being the one endpoint whose representation is decided by hook order. Covered by four cases in the API error suite, including one pinning the leak with the guard removed so the precondition is visible if the plugin changes. Signed-off-by: Julio Polycarpo <julio@polycarpo.dev>
juliopolycarpo
added
area: api
github-actions
Bot
added
type: test
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughThe OpenAPI problem-details middleware now detects failed specification responses and negotiates their error representation. A scoped error hook logs specification-generation failures and returns a generic 500 Sequence Diagram(s)sequenceDiagram
participant Client
participant OpenAPI spec route
participant openapiProblemDetails
participant Error response
Client->>OpenAPI spec route: request specification
OpenAPI spec route->>openapiProblemDetails: report generation failure
openapiProblemDetails->>openapiProblemDetails: log original exception
openapiProblemDetails->>Error response: create sanitized 500 response
Error response-->>Client: negotiated error representation
Assessment against linked issues
Possibly related PRs
Merge Risk: ⚪ Minimal · up to The change sanitizes failures from the OpenAPI specification route while preserving successful document responses and content negotiation; no actionable merge-blocking risk remains after normal checks and review. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86f4af0f45
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
PR head: Commits — 2 commitsBase
Full commit messages
|
| Metric | Base | Head | Δ |
|---|---|---|---|
| Wall clock (shared jobs) | 8m 42s | 9m 0s | 🔴 ▲ +18s |
| Critical path | Test / Unit & Integration Tests · 7m 1s |
Test / Unit & Integration Tests · 7m 0s |
🟢 ▼ -1s |
Since previous PR run: 8m 35s → 9m 0s (🔴 ▲ +25s)
Five slowest head jobs
| Job | Base | Head | Δ |
|---|---|---|---|
Test / Unit & Integration Tests |
7m 1s | 7m 0s | 🟢 ▼ -1s |
Distribution / Build immutable distribution |
3m 19s | 2m 58s | 🟢 ▼ -21s |
QA Metrics / Collect |
1m 31s | 1m 44s | 🔴 ▲ +13s |
Smoke — Browser / Chromium smoke suite |
1m 9s | 54s | 🟢 ▼ -15s |
Smoke — Binary / Binary windows-arm64 |
51s | 52s | 🔴 ▲ +1s |
QA Gate — Coverage & Quality
Base: 1e59ba9 • Head: 00e8221 • generated 2026-08-14T23:06:43.865Z
✅ No attention signals — collected metrics look healthy against base.
LoC (code): 🔴 ▲ +157 • Line coverage (all workspaces): ⚪ ▲ = 0 • Quick check: pass • Duplication: 🟢 ▼ -0pp • Bundle gzip: ⚪ ▲ = 0 • Locked deps: ⚪ ▲ = 0 • Tests passed: 🟢 ▲ +8
Metric details (coverage, LoC, bundle, dependencies, tests, duplication, tooling)
Coverage
API/shared/runtime branches and statements are source-derived from LCOV line hits because Bun LCOV does not emit branch or statement records.
| Workspace | Metric | Base | Head | Δ |
|---|---|---|---|---|
| frontend | lines | 86.13% (13,489/15,661) | 86.12% (13,487/15,661) | 🔴 ▼ -0.01pp |
| frontend | statements | 75.99% (7,663/10,084) | 75.95% (7,659/10,084) | 🔴 ▼ -0.04pp |
| frontend | functions | 72.68% (2,368/3,258) | 72.68% (2,368/3,258) | ⚪ ▲ = 0 |
| frontend | branches | 67.83% (5,507/8,119) | 67.80% (5,505/8,119) | 🔴 ▼ -0.03pp |
| api | lines | 81.94% (67,736/82,661) | 81.95% (67,753/82,680) | 🟢 ▲ +0.01pp |
| api | statements | 81.03% (27,039/33,370) | 81.03% (27,055/33,388) | ⚪ ▲ = 0 |
| api | functions | 84.68% (6,803/8,034) | 84.70% (6,807/8,037) | 🟢 ▲ +0.02pp |
| api | branches | 49.37% (10,083/20,422) | 49.37% (10,091/20,438) | ⚪ ▲ = 0 |
| shared | lines | 98.20% (14,220/14,480) | 98.20% (14,220/14,480) | ⚪ ▲ = 0 |
| shared | statements | 95.00% (2,585/2,721) | 95.00% (2,585/2,721) | ⚪ ▲ = 0 |
| shared | functions | 93.01% (426/458) | 93.01% (426/458) | ⚪ ▲ = 0 |
| shared | branches | 61.08% (885/1,449) | 61.08% (885/1,449) | ⚪ ▲ = 0 |
| runtime | lines | 81.34% (24,866/30,571) | 81.34% (24,866/30,571) | ⚪ ▲ = 0 |
| runtime | statements | 77.65% (8,938/11,510) | 77.65% (8,938/11,510) | ⚪ ▲ = 0 |
| runtime | functions | 76.45% (1,860/2,433) | 76.45% (1,860/2,433) | ⚪ ▲ = 0 |
| runtime | branches | 48.52% (3,496/7,205) | 48.52% (3,496/7,205) | ⚪ ▲ = 0 |
Lines of Code
| Workspace | Base | Head | Δ |
|---|---|---|---|
| frontend | 581 files / 66,686 lines | 581 files / 66,686 lines | files ⚪ ▲ = 0 • code ⚪ ▲ = 0 |
| api | 972 files / 138,438 lines | 973 files / 138,595 lines | files 🔴 ▲ +1 • code 🔴 ▲ +157 |
| shared | 197 files / 26,754 lines | 197 files / 26,754 lines | files ⚪ ▲ = 0 • code ⚪ ▲ = 0 |
| runtime | 824 files / 39,652 lines | 824 files / 39,652 lines | files ⚪ ▲ = 0 • code ⚪ ▲ = 0 |
| total | 2,574 files / 271,530 lines | 2,575 files / 271,687 lines | files 🔴 ▲ +1 • code 🔴 ▲ +157 |
Frontend Bundle
| Metric | Base | Head | Δ |
|---|---|---|---|
| gzip total | 839.5 KiB | 839.5 KiB | ⚪ ▲ = 0 |
| gzip JavaScript | 823.6 KiB | 823.6 KiB | ⚪ ▲ = 0 |
| gzip CSS | 15.0 KiB | 15.0 KiB | ⚪ ▲ = 0 |
| gzip HTML | 948 B | 948 B | ⚪ ▲ = 0 |
| tracked files | 35 | 35 | ⚪ ▲ = 0 |
Dependencies
| Metric | Base | Head | Δ |
|---|---|---|---|
| locked packages | 859 | 859 | ⚪ ▲ = 0 |
| direct dependencies | 43 | 43 | ⚪ ▲ = 0 |
| direct devDependencies | 41 | 41 | ⚪ ▲ = 0 |
| workspace manifests | 5 | 5 | ⚪ ▲ = 0 |
Tests
Single full-suite pass (unit + integration, from the coverage run).
| Base | Head | Δ passed |
|---|---|---|
| 6,067 passed (root 6,067 / frontend 0 / api 0 / shared 0 / runtime 0) · exit 0 · 397s | 6,075 passed (root 6,075 / frontend 0 / api 0 / shared 0 / runtime 0) · exit 0 · 394s | 🟢 ▲ +8 |
Code Duplication (jscpd)
| Metric | Base | Head | Δ |
|---|---|---|---|
| clones | 1,178 | 1,179 | 🔴 ▲ +1 |
| duplicated lines | 13,824 | 13,829 | 🔴 ▲ +5 |
| percentage | 4.11% | 4.10% | 🟢 ▼ -0pp |
Repo Tooling
| Metric | Base | Head | Δ |
|---|---|---|---|
| Full repo check | pass | pass | ⚪ ▲ = 0 |
| TS errors (total) | 0 | 0 | ⚪ ▲ = 0 |
| Circular dependencies | 0 | 0 | ⚪ ▲ = 0 |
Elysia serves GET /scalar/json/ as the same document, but path keeps the trailing slash, so the exact match skipped both the sanitizer and the amendment. A generation failure on that spelling still leaked the raw exception. The same path-only filter also rewrote POST /scalar/json from a 404 into INTERNAL. NotFound now falls through to errorHandler. The throwing-generator fixture moved to the unit suite: it never needed the assembled app, and integration tests in this workspace go through createApiTestApp or the real app. Signed-off-by: Julio Polycarpo <julio@polycarpo.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GET /scalar/jsonand its trailing-slash alias.errorHandlernever classified that route, so Elysia's built-in renderer published the raw exception message asdetailon an unauthenticated endpoint.ProblemDetails. A non-GET miss on the same path stays a 404 fromerrorHandler, not an INTERNAL rewrite.Changes
openapiProblemDetails(already mounted beforeopenapi()inapp.ts) so the spec route answers first.@elysia/openapi's local error hook logs and returns nothing, which is what used to hand the failure to Elysia's renderer./scalar/jsonand/scalar/json/as the same document path.pathkeeps the request spelling, so an exact match left the alias unhooked for both sanitizing and amending.NotFoundfall through. A path-only filter was rewritingPOST /scalar/jsonfrom 404 into INTERNAL.components.schemas. Spec failures go through the negotiator (Vary: Accept) becauseerrorHandler's copy cannot reach this route.[openapi-spec]. That is the only record of what failed.apps/api/tests/unit/server/openapi-spec-error.test.tsfor the leak (including the alias, negotiation, POST 404, and a pin with the guard removed). Integration covers POST 404 and trailing-slash amendment on the realapp.Test Plan
bun run checkpassesbun run testpassesbun run buildpasses@mangostudio/shared/i18n(no hardcoded strings)CI on this head: Check / Check (tooling + typecheck), Test / Unit & Integration Tests, and Build / Build (frontend + API) all passed. No frontend or i18n changes. Login/chat/image smoke is not the regression path; the leak and alias are covered by the unit suite plus the two new integration cases.
Screenshots / GIFs
N/A.
Notes
No env, schema, or breaking API changes.
Moving
errorHandlerto the front of the outer app is not an option. The frontendNotFoundarm is seated ahead of it on purpose. Elysia walksNotFoundhandlers in registration order and stops at the first that returns something.Whether
/scalarshould be exposed unauthenticated in production is a deployment question, not this bug.