feat(agent-bff): serve the OpenAPI document in a browser through Redoc - #1829
Conversation
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (6)
🛟 Help
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| <body> | ||
| <form id="unlock"> | ||
| <label for="key">BFF API key</label> | ||
| <input id="key" name="key" type="password" autocomplete="off" spellcheck="false" /> |
There was a problem hiding this comment.
The form has no method and no action, so with inline scripts blocked the preventDefault at line 99 never runs and the browser navigates to /docs?key=<secret>, putting the key in browser history, BFF access logs and any proxy in between: let's drop name="key" since the script reads the input by id, and add method="post" as a belt.
There was a problem hiding this comment.
Done in 749ce16 — differently: the <form> is gone entirely rather than neutralised.
method="post" still leaves a form that submits. The POST would land on 404 (READ_METHODS only holds GET/HEAD), so no key in a URL, but the form element stays and that is what Chrome reads as a login in your next comment. With a plain <div id="unlock">, a <button type="button"> and an explicit keydown/Enter listener there is no default action to prevent: without the inline script the button does nothing at all instead of navigating. name="key" is gone with it.
| <body> | ||
| <form id="unlock"> | ||
| <label for="key">BFF API key</label> | ||
| <input id="key" name="key" type="password" autocomplete="off" spellcheck="false" /> |
There was a problem hiding this comment.
A password input, a form hidden on success and a cleared value are exactly Chrome's successful-login heuristic, which ignores autocomplete="off" on password fields and offers to save the key, so the "key is never persisted" claim in the header comment breaks as soon as the user accepts: let's use autocomplete="new-password", or state that limit in the header comment instead of claiming the key is never persisted.
There was a problem hiding this comment.
Done in 749ce16, via the form removal rather than autocomplete.
new-password would not have helped — it is the signup/change-password marker, so Chrome still offers to save, and additionally suggests a generated password on a field that is not one. What actually drives the heuristic is the form submit, and there is no form any more.
The header comment now states the reasoning instead of just the claim.
| } | ||
|
|
||
| form.style.display = 'none'; | ||
| Redoc.init(result.body, { hideDownloadButton: true }, document.getElementById('redoc')); |
There was a problem hiding this comment.
Redoc 2.x renders markdown descriptions as HTML without sanitizing unless untrustedSpec is set, and DOMPurify already ships inside redoc's tree, so the option costs nothing on a page that holds a credential in memory: let's pass untrustedSpec: true to Redoc.init.
There was a problem hiding this comment.
Done in 749ce16 — untrustedSpec: true on Redoc.init. The descriptions come from the agent's own schema, which is customer-authored, so it is the right default here regardless of cost.
| form.style.display = 'none'; | ||
| Redoc.init(result.body, { hideDownloadButton: true }, document.getElementById('redoc')); | ||
| }) | ||
| .catch(function (fetchError) { |
There was a problem hiding this comment.
The catch is chained after the second then, so a throw from Redoc.init at line 91, including the ReferenceError when the bundle script did not load, is reported as "Could not reach /agent/openapi.json" and points the reader at the wrong thing: let's check typeof Redoc === 'undefined' before init with its own message, or catch around the init separately.
There was a problem hiding this comment.
Done in 749ce16. The init moved out of the fetch chain into a render() with a typeof Redoc === 'undefined' guard (its own message, naming the bundle path) and its own try/catch around Redoc.init. The chain's .catch now only ever reports a fetch failure, which is what it says.
| ...oauthMiddlewares, | ||
| // Outside the agent-scoped chain on purpose: the viewer is a public page, the document it fetches | ||
| // is not. | ||
| createDocsRoutes({ enabled: config.openapiEnabled, documentPath: OPENAPI_PATH, logger }), |
There was a problem hiding this comment.
Without FOREST_AUTH_SECRET, buildAgentMiddlewares returns [] so no OpenAPI route is mounted, yet the docs routes are gated on config.openapiEnabled alone, so /docs serves a page whose fetch can only ever get a bare Koa 404: the exact case docs-routes.ts:51-52 says the 404 fall-through exists to avoid. Let's gate it like the error middleware two lines above, with enabled: config.openapiEnabled && agentMiddlewares.length > 0.
There was a problem hiding this comment.
Done in 749ce16 — enabled: config.openapiEnabled && agentMiddlewares.length > 0, exactly as the error middleware two lines above.
Covered in test/cli-core.test.ts: with FOREST_AUTH_SECRET unset, /docs and /agent/openapi.json both answer 404.
| }); | ||
| }); | ||
|
|
||
| describe('when the viewer bundle is requested without credentials', () => { |
There was a problem hiding this comment.
The page's no-store is asserted at lines 52-56 but the bundle's public, max-age=3600 is not, and it is the only cache header in the package that diverges from no-store: let's assert expect(response.headers['cache-control']).toBe('public, max-age=3600') in the bundle describe.
There was a problem hiding this comment.
Done in 749ce16 — expect(response.headers['cache-control']).toBe('public, max-age=3600') in the bundle describe.
c01807b to
e242dc6
Compare
1 new issue
|
749ce16 to
6ce3f42
Compare
59f68d3 to
475bec7
Compare
e242dc6 to
90b37ef
Compare
Adds GET /docs and GET /docs/redoc.standalone.js, both public and both outside the agent chain: /agent/* answers 401 to a request with no credential, and a browser sends none when it navigates. The page carries no schema. It asks for a BFF API key, fetches the gated document with it, and hands the parsed object to Redoc, so the document stays unreachable unauthenticated. The key is never persisted. The bundle is self-hosted rather than loaded from a CDN: the page holds a credential in memory, and a third-party script in that page could read it. redoc is a devDependency whose bundle is copied into dist at build time, so no consumer of the BFF installs its dependency tree. Fixes PRD-965 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bundle lookup becomes a seam, so the state a broken build leaves behind — the viewer disabled with a warning naming the missing file, both routes on 404 — is asserted rather than assumed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8091dad to
6518b88
Compare
The prompt was a `<form>` with no `action`, so anything that kept the inline script from running — a CSP on the deployment is enough — turned the submit into a navigation to `/docs?key=<the key>`: the credential in the browser history, in the BFF access log and in every proxy on the way. A form submit is also what Chrome reads as a login, and it offers to save the key whatever `autocomplete` says, which broke the "never persisted" claim in the header comment. `autocomplete="new-password"` would not have helped: it is the signup marker, and Chrome still offers to save. So there is no form at all. The prompt is a div, the button is a plain button, and Enter on the input is wired explicitly — the only thing the form gave. Without the script the button now does nothing instead of leaking. Also on that page: `untrustedSpec` on `Redoc.init`, since the descriptions in the document come from the agent's own schema and Redoc renders their markdown as HTML unsanitized otherwise; and the init moved out of the fetch chain, so a missing bundle no longer reports itself as "could not reach the document". Two mount problems around it: - `/docs` was gated on `openapiEnabled` alone, so an install with no `FOREST_AUTH_SECRET` — no agent chain, no document mounted — served a page whose fetch could only ever reach a bare Koa 404. Gated on the edge being mounted too, like the error middleware above it. - `readFileSync` on the bundle ran outside any error handling, so a file that resolved at boot and became unreadable answered a bare 500 on a path no error middleware covers. It falls through now, like a missing bundle. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The viewer was stock Redoc on a bare shell. It now carries the palette the frontend defines in `app/styles/common/palette.css`: the lime ramp as the accent, slate as the neutrals, dark chrome on the sidebar and the right panel the way the product's own chrome reads. The palette is copied into `docs-theme.ts` rather than shared — this package depends on nothing in the frontend, and a viewer trailing a shade behind a redesign is not a defect. Lime 500 is the brand colour and it carries 1.96:1 against white, so it is never text here: it is a fill, with slate 1000 on it (9.18:1). Lime 700 is the lightest shade usable as text on white (4.54:1) and takes the links and the accents; the dark chrome takes lime 400 (11.5:1 on slate 1000). Inter and Source Code Pro lead the font stacks but are NOT fetched. A page holding an API key in memory must not talk to a font CDN, for the same reason the Redoc bundle is served from here instead of from unpkg. A machine without them gets the system UI font, which is the price. A test now asserts the page carries no `https?://` at all, so that reasoning is mechanical rather than stated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`info.title` is the heading Redoc renders and the name that lands in any client generated from the document, so it is API metadata rather than page chrome — kept in its own commit for that reason. No test asserted the old value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The frontend's `public/img/logo.svg` verbatim, minus its XML prolog — the mark itself rather than a redrawing of it, so it cannot drift in geometry, and a diff against the source asset stays trivial. 410 bytes, 558 once encoded. Inline as a data URI rather than a served file: this page must request nothing off-origin, and an icon file is a request like any other. It also needs no route of its own and no bundle to exist, unlike everything else the page pulls. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two submissions in quick succession — a mistyped key corrected straight away — resolve in whatever order the network gives them. Nothing checked which one was still current, so an abandoned attempt answering late applied its result over the live one: a stale 401 painting an error box over a rendered document, or a stale document rendering over the one the reader actually asked for. Both `then` and `catch` now drop completions that are not the latest attempt. A counter rather than an AbortController: aborting fires the same `catch` that would then need filtering anyway, so the check is the whole fix and the abort only saves a request already in flight. The page script had no executable test — asserting a guard by substring proves nothing about ordering. It now runs in a `vm` against a stub DOM and a fetch whose responses are resolved by hand, which is what lets the three race cases be driven at all. Verified to bite: with the two checks removed, those three fail and the other three pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ment A 200 whose body is not JSON — a gateway page, a truncated response — built the `unreadable_response` placeholder and then kept `ok: response.ok`, so the success branch handed that placeholder to `Redoc.init` as if it were a spec. The message describing the real problem was already there and could never be shown. The parse failure now sets `ok: false`, which is the only status that matches what happened. Also fixes the flake I introduced with the previous commit's harness: `flush()` awaited a single `process.nextTick`, and the nextTick queue runs BEFORE the microtask queue, so one tick does not settle a three-hop fetch chain. Proven rather than guessed — a bare probe shows a 3-deep chain unsettled after nextTick and settled after setImmediate, which is what it uses now. Three full suite runs clean since. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The unfolded operations carried no tags, so a viewer had no structure to group by and rendered one flat list of everything. On a 16-collection schema that is 82 entries in a row; a real one runs into the hundreds, and the reader cannot find a collection in it. Each operation now carries its collection as its tag, and the document declares the tag list. Relation and action operations take their PARENT collection rather than the foreign one, so a group answers "what can I do with this collection", which is the question the reader arrives with. The tag list is declared rather than left to first appearance: it fixes the grouping order, and it hands a consumer the collection list without parsing paths for it. The generic document declares none — one operation per shape has nothing to group. `tag` is required on `OperationOptions` rather than optional, so the compiler pins every call site and a future operation cannot be added untagged by omission. Measured on a real 16-collection schema: 82 operations, 16 declared tags, no untagged operation, no undeclared tag, and `redocly lint` still clean. Two of the tests are those last invariants rather than examples, which is what stops the flat list from creeping back one operation at a time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The docs page now decorates the fetched document with `x-codeSamples`, which
Redoc renders as one tab per language in the right panel.
In the page rather than in the document, deliberately. Three samples per
operation weigh ~73 KB on a 16-collection schema and several hundred KB on a
large one, which every consumer of `/agent/openapi.json` would pay for an
extension only a viewer reads — where this costs one function whatever the
operation count. It also lets a sample carry the REAL origin: the document
declares `servers: [{ url: '/' }]`, so a sample built into it could only hold a
placeholder host, while the page knows where it is served from and emits a
command that runs as pasted.
The generator reads the document rather than assuming the routes. The auth
header comes from the security scheme the operation names — apiKey in header
takes its name, http bearer becomes `Authorization: Bearer` — and the body
carries exactly the properties the request schema makes required, flattening
`allOf`. That yields `parentId` for a relation, `recordIds` for an action and an
empty body for a list without those three families being written down anywhere.
The timezone header is always emitted: `resolveTimezone` throws
`missing_timezone` when the header, the body field and the deployment default
are all absent, so a sample without it is a 400 — which is exactly what a
hand-written snippet forgets.
The key is never inlined. Each language reads it from the environment, so a
copied sample cannot carry a credential into a shell history.
Two guards the tests forced out. `withSamples` swallows its own failures: the
decoration first sat inside `Redoc.init`'s try, so a generator bug would have
surfaced as "Redoc could not render the document" and sent the reader looking in
the wrong place; a shape the generator cannot walk now costs the snippets, never
the page. And placeholder resolution is depth-bounded, because a filter is a
condition tree and a schema can reference itself.
Verified beyond "renders something": the 17 tests assert the exact curl, node
and ruby sources, that no sample carries the key the reader typed, and that a
self-referential schema still renders. Against the real 82-operation document,
all 82 get three samples with no missing auth or timezone header, and every
generated node and ruby sample parses (`ruby -c`, 82/82 each).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sample URLs were `origin + path`, so on the GENERIC document — every path a
template — all three languages pointed at a literal `/agent/v1/{collection}/list`.
That document is reachable whenever the deployment cannot unfold: no AGENT_URL,
or no read-model configuration.
Each path parameter the operation declares now becomes the same `<name>`
placeholder the bodies already use, resolving a `$ref`'d parameter too. One
notation across a snippet reads as "replace this", where `{collection}` could be
mistaken for syntax the API expects.
Deliberately NOT a "usable sample value": the generic document is served
precisely because the deployment cannot enumerate its collections, so no real
name exists to substitute, and an invented one would read as runnable and answer
404. The unfolded document is untouched by this — its segments are already the
real names, URL-encoded — which a test pins.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two defects in the generated samples, both in how values were interpolated.
The curl secret header was single-quoted, so the shell never expanded it and the
request carried the literal `$BFF_KEY` — a sample that looks runnable and earns a
401. It is double-quoted now, with the fixed text around the variable escaped for
that context; every other header keeps single quotes, where expansion would be
wrong.
And an apostrophe in a name broke all three languages. `encodeURIComponent` does
NOT encode `'`, so a collection called `John's orders` reaches the samples with
its apostrophe intact, and a field name or enum value can carry one into a body.
Interpolation now goes through the quoting of the target language: POSIX
close-reopen (`'\''`) for shell, `JSON.stringify` for every JavaScript literal,
backslash escaping for Ruby single quotes, and `#{` neutralised in the Ruby body
since a JSON literal is double-quoted and Ruby interpolates there.
Verified against real parsers rather than by eye: 164 samples per language — the
82-operation document plus a copy whose collection and field names carry
apostrophes — all pass `bash -n`, JavaScript parsing and `ruby -c`. Running one
through a stub `curl` shows the key expanded to its real value and the URL
arriving as a single argument, apostrophe included.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
What
GET /docsrenders the BFF's dynamic OpenAPI document in a browser, with Redoc served by the BFF itself.GET /docs/redoc.standalone.jsserves the viewer bundle.Why it is shaped this way
BFF auth is header-only —
Authorization: Bearer <bff_access>orX-Forest-Bff-Key(src/auth/auth-mode-middleware.ts), no cookie anywhere in the package.resolveAuthModethrowsunauthorized()when neither header is present (src/auth/auth-mode.ts:24-30), and that middleware covers every/agent/*path. A browser navigating to a page sends neither header, and Redoc cannot attach one to its own spec fetch. So:/agent, next to the existing public/oauth/*routes;/agent/openapi.jsonwith that header itself, and passes the parsed object toRedoc.init.The document therefore stays exactly as unreachable as before:
test/cli-core.test.tsasserts/agent/openapi.jsonstill answers 401 while/docsanswers 200, which is the interaction the mount invariant of #1827 cannot see on its own.The key is held in memory only — passed as an argument, input cleared, never written to
localStorageorsessionStorage, never put in a URL.Bundle provenance
redocis a devDependency; itsredoc.standalone.jsis copied intodistat build time (build:copy, the patternforest-cloudalready uses for its templates). Consequences:files: dist/**/*.jsalready covers the copied asset — no packaging change needed;src(tests,build:watch) there is nothing to copy to, so the route falls back to resolving the devDependency. Both paths are covered by the tests.Not a CDN, deliberately: the page holds a credential in memory, and a third-party script in that page could read it. Pinning an SRI hash would mitigate that at the cost of a manual hash bump per version.
Disabled state
Same flag as the document,
BFF_OPENAPI_ENABLED. When it is off both routes fall through to 404 rather than throw:/docsis outside the agent-scoped error middleware (src/cli-core.tswrapscreateErrorMiddlewareinagentScoped), so a thrownopenapiDisabled()would surface as a bare Koa 500 instead of the BFF error contract. A 404 also keeps a disabled deployment from advertising a page it does not serve.Structural guarantee
src/docs/imports nothing fromsrc/openapi/— the document path is passed in fromcli-core. The existingopenapi-mount-invariant.test.tsenforces this mechanically: any such import would show up inopenapiImportsOutsideTheOpenapiDir()and fail. That is a stronger statement than the substring assertion on the page body, and both are in place.Tests
10 route tests plus 3 full-chain tests: page and bundle public, page carries no schema, page never cached, bundle served as a script, document still 401, both routes 404 when disabled, non-docs paths and writes passed through. Package suite: 71 suites / 1106 tests green,
yarn buildcopies the bundle intodist/docs/.Not in scope
No "try it out" console, no write path from the page, and no OAuth login flow in the page — the BFF's OAuth is a registered-client authorization server, so the page would have to be registered as a client first.
Fixes PRD-965
🤖 Generated with Claude Code
Note
Serve OpenAPI docs viewer at
/docsvia Redoc inagent-bff/docsand serves the Redoc standalone bundle at/docs/redoc.standalone.js, both outside the agent-scoped auth chain whenopenapiEnabledis true.X-Forest-Bff-Keyheader, and never persists the key.require.resolve('redoc/bundles/redoc.standalone.js'); contents are read once and cached in memory.dist/docs.openapiEnabledis false or the bundle is missing,/docsrequests fall through tonext()instead of returning a dedicated response, resulting in 404s upstream. The OpenAPI document at/agent/openapi.jsonremains credential-gated.Changes since #1829 opened
Macroscope summarized 6518b88.