feat(server): add least-privilege data-plane catalog endpoint - #1557
feat(server): add least-privilege data-plane catalog endpoint#1557LeoWang331 wants to merge 18 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
1/4 boxes ticked. This pull request was already a draft. Its draft status will be preserved after every issue above is resolved. |
Wibias
left a comment
There was a problem hiding this comment.
The least-privilege route itself looks well designed and the negative management-plane coverage is strong, but I have one blocking contract issue on this head: /v1/catalog is deliberately omitted from the shipped AUTH_MATRIX in src/server/auth-cors.ts.
That matrix is explicitly the server-driven source of truth for which credential headers each data-plane endpoint accepts, is shipped to the GUI, and is backed by real-request matrix tests. Adding a new authenticated data-plane endpoint while documenting it only in prose leaves that machine-readable/user-facing contract incomplete. The PR body says the row was reverted to avoid putting the PR on the sponsored auth surface; that is not a good reason to let the source of truth drift. This issue is already maintainer-approved architecture, so please add /v1/catalog to AUTH_MATRIX with the behavior the route actually implements (bearer: accepted, dedicated: accepted, xApiKey: accepted) and extend the existing matrix/request coverage accordingly. Handle the maintainer-sponsored gate rather than working around it by omitting the contract row.
Separately, this branch is currently 15 commits behind dev (6c14e343), and Cross-platform CI is still running. After the matrix fix, rebase onto current dev and rerun exact-head CI.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…lane Co-authored-by: Cursor <cursoragent@cursor.com>
…tion Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…k probes Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…ed HEAD Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
… real-catalog compatibility Co-authored-by: Cursor <cursoragent@cursor.com>
…ng refusal Co-authored-by: Cursor <cursoragent@cursor.com>
…nippet Co-authored-by: Cursor <cursoragent@cursor.com>
…wnload snippet Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
93d83e3 to
86c0636
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@Wibias @Ingwannu — Issue #809 / PR #1557 now needs maintainer policy and security decisions before it can leave Draft. Current head is The mechanical requests from the existing review are implemented:
The remaining blocker is the catalog-distribution safety policy. The current implementation is a name/shape-based heuristic denylist. It is explicitly non-guaranteeing:
Our recommendation is:
If you prefer to keep the heuristic temporarily, we propose a schema- and type-aware fail-closed exception layer only for individually approved sensitive-looking fields. We would not broadly accept Please decide:
Process actions also needed:
Nothing further will be implemented until a maintainer chooses the policy. The PR will remain Draft. |
Wibias
left a comment
There was a problem hiding this comment.
Re-review on exact head 5586e4e01b63e5076bee9fedc3eb980b499c925a.
The previous AUTH_MATRIX blocker is resolved. /v1/catalog is now present in the shipped matrix with the implemented bearer / dedicated / x-api-key admission, and the real-request matrix test exercises it as a GET route. I am not carrying that finding forward.
There is, however, a more fundamental blocker on the current design:
[P1] The data-plane distribution boundary does not currently enforce the no-leakage invariant required by #809.
/v1/catalog exposes the persisted catalog to a lower-privilege remote credential class. The current materializer passes the parsed RawCatalog through after a heuristic denylist check, while the PR itself documents verified false negatives and explicitly states that the no-leakage invariant is not fully enforced. RawCatalog also permits arbitrary additional top-level/model fields, so this is not a closed distribution schema.
For a new least-privilege security boundary, a best-effort detector is not sufficient when the accepted issue contract says credentials, account identity, provider configuration, filesystem paths, and other management-only state must not be distributed. Please make the data-plane boundary fail closed before enabling the route. A versioned allowlisted distribution DTO / projection, combined with exact filtering of known live secrets and identities where needed, would be a reasonable direction; unknown persisted fields should not automatically become remotely distributable.
There is a second policy regression to resolve at the same time: the shared materializer now applies the same heuristic refusal to existing management GET /api/catalog, so a false positive can turn a previously valid management response into a 500. The PR body correctly calls this an unapproved policy decision. Keep the authoritative reader/serializer shared, but do not silently change the management route's acceptance semantics unless maintainers explicitly choose that behavior.
Separately, this PR still needs to be brought back to current review-ready state after the design fix: rebase/update against current dev, resolve the disclosed test timeouts, get exact-head repository CI green, and complete the required security review / maintainer-sponsored gate for the auth-surface change.
Once the distribution boundary actually enforces the accepted no-leakage contract, I can re-review the resulting exact head.
|
Triage note (2026-08-15, maintainer): keeping as draft. The least-privilege goal is right, but the central invariant is not enforced: RawCatalog allows arbitrary fields and the materializer serializes them behind a heuristic denylist instead of projecting through a closed allowlisted DTO, so a lower-privilege credential can receive unknown future/injected fields. Also: separate the management /api/catalog acceptance policy from the data-plane one, resolve the three local timeout failures, and this needs security sponsorship + full CI. |
리뷰 · 우선순위 37 / 80Draft이고 다만 점수 한가운데에 두지 못하는 이유는 안전 판정이 저자가 스스로 적은 그대로 휴리스틱 denylist이기 때문이다.
크기 거절은 의도가 분명하다. 디스크 원본은
해결방안: 배포 문서는 denylist가 아니라 필드 allowlist 투영으로 직렬화하거나, 후보 문자열을 살아 있는 config/자격 증명 저장소와 정확히 대조하는 쪽으로 정책을 먼저 고정하라. 전자는 알 수 없는 Codex 필드를 떨어뜨릴 수 있고 후자는 materializer가 config에 의존하게 되므로, 그 트레이드오프를 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
left a comment
There was a problem hiding this comment.
Reviewed exact head 5586e4e. The direction remains valuable, but this draft is not a merge candidate. It is 1,386 dev commits behind and currently conflicts. The Grok review blockers also remain material: the data-plane catalog disclosure boundary is still based on a heuristic denylist with documented gaps, auth-cors changes need explicit maintainer sponsorship, and the unsafe or oversized response status contract plus the timing and hygiene failures are unresolved. Please rebase first, choose and document a defensible projection policy, then rerun exact-head CI and security review. This is preliminary review feedback, not approval.
|
Thank you for the #809 design work — the HEAD/CORS handling and the docs were useful groundwork. The data-plane catalog route is now being landed as part of the remote-hub stack in #2772, which adds Closing in favour of #2772 rather than asking you to rebase a branch that is ~2190 commits behind |
Closes #809
Summary
GET/HEAD /v1/catalog, a read-only data-plane projection of the generated Codexcatalog, so a remote client can fetch model metadata with the credential it already uses
for inference.
/api/*is untouched and gains no data-plane exception; the data-planecredential is still denied on every management route, including
GET /api/catalog.src/codex/catalog/distribution.ts: ahard-bounded source read before
JSON.parse, a safety verdict, and one serializer.Management
GET /api/catalogand data-planeGET/HEAD /v1/catalogboth go through it.Safety check: what it is, and what it is not
It is a heuristic denylist, not a guarantee. It rejects a document (content-free
500 catalog_unsafe, both planes) when a key spelling names management state or a stringvalue matches a recognizable credential / identity / home-path shape. Representation
equivalence is enforced — Authorization scheme matching is case-insensitive, and key
normalization drops every non-alphanumeric character, so
account.id,account_id,account-id,accountId, andaccount idare one key.Verified gaps, pinned as known behavior in the tests rather than papered over:
D:\ocx\config.jsonAdditionally, key normalization strips all non-ASCII, so a non-ASCII key spelling (for
example a Cyrillic or fullwidth name) normalizes to the empty string and is accepted.
Widening the denylist is not the fix: it converts real catalogs into permanent 500s on
both routes. Closing these gaps needs a different strategy, which is a policy call.
The no-leakage invariant is therefore not fully enforced today — it is approximated by
this heuristic, and that is exactly what the open decision below is about.
Compatibility is therefore tested, not assumed. A rejected document takes both routes
down together, so the suite asserts that the pinned upstream snapshot
(
src/codex/data/upstream-models.json) and the OpenCodex-owned extension fields agenerated catalog carries are safe to distribute. A future Codex schema addition that trips
a rule fails there instead of in production.
Open maintainer decisions (recorded, not resolved)
non-guaranteeing); a versioned canonical distribution DTO / strict field projection
produced at the trusted writer boundary, which guarantees the field shape that
leaves the boundary but not that an allowed field is free of secrets, since permitted
strings such as
base_instructions,description,display_name, model ids andownership fields can still carry sensitive values; exact-value comparison against live
configured secrets, account identities, provider base URLs/headers and filesystem paths,
which is precise for known values but cannot decide unknown management-shaped
fields; or a hybrid of the last two, which is what is required if the no-leakage
invariant must actually hold for known live secrets. Provenance restriction is at best
an auxiliary input constraint, not a guarantee —
ocx syncdeliberately preserveson-disk and user-authored rows (
src/codex/catalog/sync.ts:1253-1257,:888-901) andreplaces only
catalog.modelsbefore serializing the whole document (:1392-1422), soan "OpenCodex-generated" file is not secret-free by construction.
shared materializer,
GET /api/catalognow also refuses an unsafe or oversized-sourcedocument; it previously served any parseable file. This is a consequence of the single
materialization step, not an approved policy, and the management-API reference says so.
catalog_unsafe,catalog_too_large,catalog_source_too_large, plus the 8 MiB / 32 MiB thresholds, auth-before-methodordering, and byte-identical (vs merely equivalent) cross-route bodies.
structure/05_gui-and-management-api.mdseparates these tiers explicitly. Only the planesplit, the single materialization authority, GET/HEAD-only access, the no-leakage
requirement itself, and "data-plane credentials cannot reach
/api/*" are presented asmaintainer-accepted; the enforcement strategy and detector rule set are marked
executor-selected. No option above is presented as chosen.
Bounded input and size errors
CATALOG_SOURCE_MAX_BYTES(32 MiB): size is checked on the open descriptor before anybytes are read, so an oversized file never reaches memory or
JSON.parse.DATA_PLANE_CATALOG_MAX_BYTES(8 MiB) measures serialized UTF-8 response bytes andrefuses rather than truncating. Pinned at limit−1 / exact / limit+1, plus a multi-byte
UTF-8 case proving bytes (not characters) are counted.
catalog_source_too_largeand says only that the source exceeded the safe read limit;reusing
catalog_too_largethere asserted a serialized size that was never computed (a33 MiB pretty-printed file can compact below 8 MiB). Route-level GET and HEAD tests cover
it, including that no partial catalog or source content appears in the observable
response and that
no-store/nosniffstill hold.(
{ "error": "<message>" }), not the data-planetype/codeenvelope, and that routedoes not apply the 8 MiB serialized ceiling. Both are documented in the management-API
reference in all six locales.
Method, header, and CORS semantics
GET/HEAD;POST/PUT/PATCH/DELETEanswer405withAllow: GET, HEADCache-Control: no-storeandX-Content-Type-Options: nosniff, so a cached404 catalog_not_foundcannot hide a catalog generated later. The global bodylessOPTIONSpreflight is answered before the route runs and does not carry these two route headers; the docs state that exclusion explicitlyHEADGETplus exactContent-Length, no bodyAccess-Control-Allow-MethodsincludesHEAD; a real OPTIONS preflight test carriesOrigin+Access-Control-Request-Method: HEAD+Access-Control-Request-Headers: x-opencodex-api-key, then proves the promised HEAD succeedsx-opencodex-codex-versionwhen authoritative; omitted, never fabricatedGET/HEAD /v1/catalogpinned as 404 on the optional unauthenticated loopback listener, with the public remote bind still answering 401Auth matrix
/v1/catalogis listed in the shippedAUTH_MATRIX(bearer / dedicated /x-api-keyallaccepted, same admission as
/v1/models) and the real-request matrix test drives all threeheader forms against it as a GET route. This addresses @Wibias's requested change — that
review is still
CHANGES_REQUESTEDand needs re-review, since a review cannot besatisfied by the author asserting it was. The branch has also been rebased onto current
devas that review asked; history is linear with no merge commits.Documentation
structure/05_gui-and-management-api.mdplus 18docs-sitefiles (English +ja/ko/ru/zh-cn/zh-tw), covering the data-plane reference, the management-plane reference,
and the Codex-integration guide.
catalog download no longer instructs operators to send
OPENCODEX_ADMIN_AUTH_TOKENtoclient machines and fetch
GET /api/catalog— the exact management-credentialdistribution this issue exists to remove. Remote clients are now directed to
GET /v1/catalogwithx-opencodex-api-key: $DATA_PLANE_KEY, linked to the locale'sreference/proxy-formats/page for the canonical atomic download workflow rather thanduplicating the shell snippet, followed by
ocx sync-cache.GET /api/catalogisdescribed only as the management-plane route for the dashboard and operator tooling on
the trusted machine.
guides previously claimed the response contains "no provider credentials". That is
stronger than the implementation can prove, given the verified false negatives listed
above. They now state only observable behavior: the response is the generated
opencodex-catalog.jsondocument; the data-plane route applies the currentcatalog-distribution safety checks and refuses content it recognizes as credential-,
identity-, or configuration-shaped; and the enforcement strategy remains subject to
maintainer review. No absolute guarantee replaces the removed one. The
x-opencodex-codex-versionskew explanation is unchanged./v1/catalogcontract, its error table, the authenticationmatrix row, the credential-class table, and the multi-machine workflow, whose snippet is
interruption-safe —
mkdir -pand a baremktempwith an explicit same-directorytemplate compatible with GNU and macOS/BSD each fail fast,
tmpis initialized beforeany trap, cleanup is bound to
EXITalone,HUP/INT/TERMhandlersexit129/130/143 rather than only cleaning up, the previous catalog survives until both
curland the same-directorymvsucceed, and every handler is cleared after asuccessful rename.
Verification
Runs on Windows with Bun 1.3.14 in this dedicated Issue #809 worktree. Commands 1–6 were run
against head
86c0636f2(rebased ontoupstream/dev=570347304). The most recent commitis documentation-only and touches no TypeScript; for it, only the two documentation checks
were re-run, per repository guidance not to rerun passing checks merely for confidence.
bun test tests/v1-catalog-route.test.tsbun test tests/api-catalog-route.test.ts tests/api-key-attribution.test.tsbun run typecheckbun run privacy:scangit diff --check upstream/dev...HEADcd docs-site && bun install --frozen-lockfile && bun run build[build] Complete!(re-run on the final head)Rebase note. The branch was rebased onto current
dev(previously it was broughtcurrent with merge commits). The rebase was clean with no conflicts, and the PR diff was
byte-identical before and after (146,273 bytes both), so no content was lost or altered.
History is linear.
Upstream delta review.
upstream/devchangedsrc/codex/catalog/parsing.tsandsync.ts, which this PR's materializer imports from. Reviewed: the change only flips theexisting
supports_search_toolboolean to!isCursorEntryand conditionsweb_search_tool_type; it introduces no new catalog key names, andparseCatalogJson/readCodexCatalogPathsignatures are unchanged.supports_search_toolnormalizes to
supportssearchtool, which matches no safety rule, so the safety verdict isunaffected.
Disclosure 1 — the catalog test ran more than once across this PR's history. An earlier
batch's first attempt at command 1 hung for ~968s and was killed: that draft's email
detector used an unanchored regex that is quadratic under backtracking on the new 8 MiB
fixtures. It was rewritten as a linear
@-anchored scan.Disclosure 2 —
tests/loopback-listener-integration.test.tsis not in the table. Itsone failure (
Codex injection targets the loopback listener, failing withCodexUserIdentityRefusal: Windows effective-account lookup returned an empty value) wasclassified earlier by running the same command on clean
upstream/devin the sameenvironment, which failed identically. That comparison was performed by temporarily
checking out
upstream/devinside this dedicated Issue #809 worktree and returning to thebranch; no other checkout was used or modified.
Disclosure 3 — command 2 failed with three timeouts, and they remain UNRESOLVED. The
failures are
attribution reaches usage.jsonl > the environment token records its own kind,… > search and realtime call-create each add an attributed row, andAUTH_MATRIX is true of the running server > every cell matches a real request. All three reporta beforeEach/afterEach hook timed outorthis test timed out after 5000ms, with akilled 1 dangling processnotice, immediately after command 1 had spawned dozens ofservers in 42s. No assertion mismatch was reported, which is what an upstream
supports_search_toolregression would produce, and the same command passed 23/23 beforethe rebase. Best available classification is local resource/port contention on this Windows
machine, but this was not re-run and no clean-upstream baseline was taken for it, so
it is not cleared. Repository CI must settle it.
Not run locally: the full suite. Cross-platform verification is repository CI's job,
and CI has not run — see blockers.
Changes
18 commits, linear (no merge commits), 0 behind / 18 ahead of
upstream/dev(
570347304). 28 files, +2545 / −69.src/codex/catalog/distribution.ts,src/codex/catalog.tssrc/server/data-plane-catalog.ts,src/server/index.tssrc/server/management/model-routes.tssrc/server/auth-cors.tstests/v1-catalog-route.test.ts,tests/api-key-attribution.test.ts,tests/loopback-listener-integration.test.tsstructure/05_gui-and-management-api.md, 18docs-sitefilesCurrent blockers
maintainer-sponsoredis not on this PR (Issue [Feature]: add least-privilege GET /v1/catalog for remote Codex clients #809 has it; thelabel does not transfer).
hygieneandenforce-targethave been failing onunsponsored_surfaceforsrc/server/auth-cors.ts, and the PR carriesintake: hygiene-blocked. Requires maintainer security review.CHANGES_REQUESTEDreview from @Wibias. The requested AUTH_MATRIX row isimplemented and the branch is rebased onto current
dev, but the review still stands andneeds a maintainer to re-review the current head.
tests/api-key-attribution.test.ts(disclosure 3). Not reproduced against a clean baseline and not re-run; CI is what would
settle them.
action_requiredand have never run on thisbranch; a maintainer must approve workflow runs for a fork PR.
has produced findings yet, so "all findings resolved" cannot be asserted from evidence.
Checklist
Admission helpers are unchanged; the auth-surface diff is one matrix row plus
HEADin
Access-Control-Allow-Methods. The safety check's limits are documented aboverather than overstated, and the user-facing guides no longer claim the response
contains no provider credentials.
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.