Skip to content

feat(server): add least-privilege data-plane catalog endpoint - #1557

Closed
LeoWang331 wants to merge 18 commits into
lidge-jun:devfrom
LeoWang331:feat/809-v1-catalog
Closed

feat(server): add least-privilege data-plane catalog endpoint#1557
LeoWang331 wants to merge 18 commits into
lidge-jun:devfrom
LeoWang331:feat/809-v1-catalog

Conversation

@LeoWang331

@LeoWang331 LeoWang331 commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Closes #809

Draft, and not claiming completeness. The mechanical findings from review are fixed.
The distribution-safety policy is unresolved: the safety check in the tree is a
heuristic denylist with verified gaps, and choosing its replacement is a maintainer
decision (see Open maintainer decisions). This PR also touches
src/server/auth-cors.ts, which is on the sponsored authentication surface, so a
maintainer must complete security review and apply maintainer-sponsored. The author
cannot and will not self-apply it.

Summary

  • Adds GET/HEAD /v1/catalog, a read-only data-plane projection of the generated Codex
    catalog, 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-plane
    credential is still denied on every management route, including GET /api/catalog.
  • Puts one catalog materialization authority in src/codex/catalog/distribution.ts: a
    hard-bounded source read before JSON.parse, a safety verdict, and one serializer.
    Management GET /api/catalog and data-plane GET/HEAD /v1/catalog both 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 string
value 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, and account id are one key.

Verified gaps, pinned as known behavior in the tests rather than papered over:

Not detected Why not widened
Raw provider base URL in an ordinary string field A blanket URL rule rejects every real catalog — instruction text contains URLs
Arbitrary-format token (e.g. bare hex) in an ordinary field No shape to match without also matching legitimate values
Arbitrary account identifier under an innocuous key Same
Non-home absolute path, e.g. D:\ocx\config.json A blanket absolute-path rule rejects legitimate instruction text

Additionally, 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 a
generated 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)

  1. Enforcement strategy. Candidates: heuristic rejection (in the tree today, and
    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 and
    ownership 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 sync deliberately preserves
    on-disk and user-authored rows (src/codex/catalog/sync.ts:1253-1257, :888-901) and
    replaces only catalog.models before serializing the whole document (:1392-1422), so
    an "OpenCodex-generated" file is not secret-free by construction.
  2. Shared rejection on the management plane. Because the verdict lives in the one
    shared materializer, GET /api/catalog now also refuses an unsafe or oversized-source
    document; 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.
  3. Statuses and public error codes for catalog_unsafe, catalog_too_large,
    catalog_source_too_large, plus the 8 MiB / 32 MiB thresholds, auth-before-method
    ordering, and byte-identical (vs merely equivalent) cross-route bodies.

structure/05_gui-and-management-api.md separates these tiers explicitly. Only the plane
split, the single materialization authority, GET/HEAD-only access, the no-leakage
requirement itself, and "data-plane credentials cannot reach /api/*" are presented as
maintainer-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 any
    bytes 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 and
    refuses rather than truncating. Pinned at limit−1 / exact / limit+1, plus a multi-byte
    UTF-8 case proving bytes (not characters) are counted.
  • The two limits answer with distinct codes. Source refusal is
    catalog_source_too_large and says only that the source exceeded the safe read limit;
    reusing catalog_too_large there asserted a serialized size that was never computed (a
    33 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/nosniff still hold.
  • On the management plane the same two refusals surface in the ordinary management envelope
    ({ "error": "<message>" }), not the data-plane type/code envelope, and that route
    does not apply the 8 MiB serialized ceiling. Both are documented in the management-API
    reference in all six locales.

Method, header, and CORS semantics

Aspect Behavior
Methods GET/HEAD; POST/PUT/PATCH/DELETE answer 405 with Allow: GET, HEAD
Headers Every response the route itself generates — 200, HEAD, 401, 403, 404, 405, 500 — carries Cache-Control: no-store and X-Content-Type-Options: nosniff, so a cached 404 catalog_not_found cannot hide a catalog generated later. The global bodyless OPTIONS preflight is answered before the route runs and does not carry these two route headers; the docs state that exclusion explicitly
HEAD Same status/headers as GET plus exact Content-Length, no body
CORS Access-Control-Allow-Methods includes HEAD; a real OPTIONS preflight test carries Origin + Access-Control-Request-Method: HEAD + Access-Control-Request-Headers: x-opencodex-api-key, then proves the promised HEAD succeeds
OPTIONS Global CORS preflight answers a bodyless 204 before route authentication, here as everywhere. The docs no longer claim every anonymous non-read method returns 401
Version x-opencodex-codex-version when authoritative; omitted, never fabricated
Loopback GET/HEAD /v1/catalog pinned as 404 on the optional unauthenticated loopback listener, with the public remote bind still answering 401

Auth matrix

/v1/catalog is listed in the shipped AUTH_MATRIX (bearer / dedicated / x-api-key all
accepted, same admission as /v1/models) and the real-request matrix test drives all three
header forms against it as a GET route. This addresses @Wibias's requested change — that
review is still CHANGES_REQUESTED and needs re-review
, since a review cannot be
satisfied by the author asserting it was. The branch has also been rebased onto current
dev
as that review asked; history is linear with no merge commits.

Documentation

structure/05_gui-and-management-api.md plus 18 docs-site files (English +
ja/ko/ru/zh-cn/zh-tw), covering the data-plane reference, the management-plane reference,
and the Codex-integration guide.

  • Codex-integration guide, all six locales — least-privilege workflow. The remote-client
    catalog download no longer instructs operators to send OPENCODEX_ADMIN_AUTH_TOKEN to
    client machines and fetch GET /api/catalog — the exact management-credential
    distribution this issue exists to remove. Remote clients are now directed to
    GET /v1/catalog with x-opencodex-api-key: $DATA_PLANE_KEY, linked to the locale's
    reference/proxy-formats/ page for the canonical atomic download workflow rather than
    duplicating the shell snippet, followed by ocx sync-cache. GET /api/catalog is
    described only as the management-plane route for the dashboard and operator tooling on
    the trusted machine.
  • Codex-integration guide, all six locales — accuracy of the response description. The
    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.json document; the data-plane route applies the current
    catalog-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-version skew explanation is unchanged.
  • Reference pages: the /v1/catalog contract, its error table, the authentication
    matrix row, the credential-class table, and the multi-machine workflow, whose snippet is
    interruption-safe — mkdir -p and a bare mktemp with an explicit same-directory
    template compatible with GNU and macOS/BSD each fail fast, tmp is initialized before
    any trap, cleanup is bound to EXIT alone, HUP/INT/TERM handlers exit
    129/130/143 rather than only cleaning up, the previous catalog survives until both
    curl and the same-directory mv succeed, and every handler is cleared after a
    successful 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 onto upstream/dev = 570347304). The most recent commit
is 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.

# Command Exit Duration Result
1 bun test tests/v1-catalog-route.test.ts 0 42.1s 65 pass, 0 fail
2 bun test tests/api-catalog-route.test.ts tests/api-key-attribution.test.ts 1 52.1s 20 pass, 3 fail — all three are timeouts; unresolved, see disclosure 3
3 bun run typecheck 0 5.3s clean
4 bun run privacy:scan 0 16.8s passed
5 git diff --check upstream/dev...HEAD 0 0.1s clean (re-run on the final head)
6 cd docs-site && bun install --frozen-lockfile && bun run build 0 12.2s + 38.7s no lockfile changes; 265 pages, [build] Complete! (re-run on the final head)

Rebase note. The branch was rebased onto current dev (previously it was brought
current 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/dev changed src/codex/catalog/parsing.ts and
sync.ts, which this PR's materializer imports from. Reviewed: the change only flips the
existing supports_search_tool boolean to !isCursorEntry and conditions
web_search_tool_type; it introduces no new catalog key names, and
parseCatalogJson/readCodexCatalogPath signatures are unchanged. supports_search_tool
normalizes to supportssearchtool, which matches no safety rule, so the safety verdict is
unaffected.

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.ts is not in the table. Its
one failure (Codex injection targets the loopback listener, failing with
CodexUserIdentityRefusal: Windows effective-account lookup returned an empty value) was
classified earlier by running the same command on clean upstream/dev in the same
environment, which failed identically. That comparison was performed by temporarily
checking out upstream/dev inside this dedicated Issue #809 worktree and returning to the
branch; 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, and AUTH_MATRIX is true of the running server > every cell matches a real request. All three report
a beforeEach/afterEach hook timed out or this test timed out after 5000ms, with a
killed 1 dangling process notice, immediately after command 1 had spawned dozens of
servers in 42s. No assertion mismatch was reported, which is what an upstream
supports_search_tool regression would produce, and the same command passed 23/23 before
the 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.

Area Files
Shared authority src/codex/catalog/distribution.ts, src/codex/catalog.ts
Data-plane route src/server/data-plane-catalog.ts, src/server/index.ts
Management route src/server/management/model-routes.ts
Auth matrix / CORS src/server/auth-cors.ts
Tests tests/v1-catalog-route.test.ts, tests/api-key-attribution.test.ts, tests/loopback-listener-integration.test.ts
Docs structure/05_gui-and-management-api.md, 18 docs-site files

Current blockers

  • Missing sponsorship. maintainer-sponsored is not on this PR (Issue [Feature]: add least-privilege GET /v1/catalog for remote Codex clients #809 has it; the
    label does not transfer). hygiene and enforce-target have been failing on
    unsponsored_surface for src/server/auth-cors.ts, and the PR carries
    intake: hygiene-blocked. Requires maintainer security review.
  • Active CHANGES_REQUESTED review from @Wibias. The requested AUTH_MATRIX row is
    implemented and the branch is rebased onto current dev, but the review still stands and
    needs a maintainer to re-review the current head.
  • Three unresolved local timeout failures in tests/api-key-attribution.test.ts
    (disclosure 3). Not reproduced against a clean baseline and not re-run; CI is what would
    settle them.
  • Cross-platform CI and React Doctor are action_required and have never run on this
    branch; a maintainer must approve workflow runs for a fork PR.
  • CodeRabbit skipped the PR because it is a draft; no Codex review exists. Neither
    has produced findings yet, so "all findings resolved" cannot be asserted from evidence.
  • The enforcement-strategy decision above is unresolved.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.
    Admission helpers are unchanged; the auth-surface diff is one matrix row plus HEAD
    in Access-Control-Allow-Methods. The safety check's limits are documented above
    rather 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.

@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a58ab4ca-0747-4629-9b72-da1012345fd8

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the enhancement New feature or request label Aug 12, 2026
@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • hygiene: unsponsored_surface.

What to do

  • Fix unsponsored_surface — This changes an authentication, workflow, release-automation, or dependency surface. MAINTAINERS.md requires security review for these; ask a maintainer to apply maintainer-sponsored once they have reviewed it. Paths: src/server/auth-cors.ts.
  • Tick all four boxes in the PR description once you're done (currently 1/4).

Review readiness checklist

  • ⬜ 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.

1/4 boxes ticked.

This pull request was already a draft. Its draft status will be preserved after every issue above is resolved.
@LeoWang331 Tick the boxes once your local CI is green, your branch is on the latest dev commit, and every correct Codex and CodeRabbit finding is resolved.

@github-actions github-actions Bot added intake: hygiene-blocked Deterministic PR hygiene checks failed and removed intake: hygiene-blocked Deterministic PR hygiene checks failed labels Aug 12, 2026
Wibias
Wibias previously requested changes Aug 13, 2026

@Wibias Wibias left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions github-actions Bot added the intake: hygiene-blocked Deterministic PR hygiene checks failed label Aug 13, 2026
LeoWang331 and others added 17 commits August 13, 2026 05:23
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>
…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>
@LeoWang331
LeoWang331 force-pushed the feat/809-v1-catalog branch from 93d83e3 to 86c0636 Compare August 13, 2026 09:39
Co-authored-by: Cursor <cursoragent@cursor.com>
@LeoWang331

Copy link
Copy Markdown
Contributor Author

@Wibias @Ingwannu — Issue #809 / PR #1557 now needs maintainer policy and security decisions before it can leave Draft.

Current head is 5586e4e01b63e5076bee9fedc3eb980b499c925a; the branch is 18 ahead and 4 behind dev (2cdbf66a23f9fd8f2f38dcc702ccd3f2e60ac535) as of this comment.

The mechanical requests from the existing review are implemented:

  • /v1/catalog is restored to the shipped AUTH_MATRIX with bearer, x-opencodex-api-key, and x-api-key accepted, and the real-request matrix test covers it.
  • The branch was rebased onto dev with linear history as requested.
  • All six Codex-integration guides now direct remote clients to GET /v1/catalog with a data-plane key instead of distributing OPENCODEX_ADMIN_AUTH_TOKEN to call /api/catalog.
  • The guides no longer claim the response is guaranteed to contain no provider credentials.
  • The PR remains Draft and does not claim the no-leakage invariant is fully enforced.

The remaining blocker is the catalog-distribution safety policy.

The current implementation is a name/shape-based heuristic denylist. It is explicitly non-guaranteeing:

  • it can miss arbitrary-format tokens, raw base URLs embedded in permitted strings, arbitrary account IDs, non-home paths, and non-ASCII key spellings;
  • it can also reject future legitimate catalog fields whose names look sensitive, returning 500 on both /api/catalog and /v1/catalog.

Our recommendation is:

  1. A versioned canonical distribution DTO / strict projection for the field-shape boundary.

    • This guarantees which fields leave the boundary, but not that free-form values inside permitted fields are secret-free.
    • The complete field set must be derived from the actual Codex schema, pinned snapshot, parser requirements, OpenCodex generation, and golden compatibility fixtures.
    • No schema-version field should be added to the payload without Codex compatibility evidence.
  2. Exact-value comparison as a required companion if protection against known configured secrets is required.

    • It must detect known sensitive values both as complete structured values and when embedded inside permitted free-form strings.
    • It should cover configured data/admin/provider credentials, OAuth values, account identity, provider base URLs/headers, and resolved local paths.
    • Refusals must remain content-free and must never log or identify the matched value.
  3. Provenance may be an auxiliary constraint only.

    • ocx sync preserves on-disk and user-authored rows and unknown top-level content, so an "OpenCodex-generated" file is not secret-free by construction.

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 base_url or *_token, and we would record that no-leakage remains an approximation.

Please decide:

  1. Safety strategy:

    • A. versioned DTO/projection + exact-value comparison — recommended;
    • B. versioned DTO/projection only;
    • C. temporary heuristic with schema/type-aware exceptions;
    • D. another approach.
  2. Should unsafe/source-too-large refusal also apply to management GET /api/catalog, or should management remain file-faithful?

  3. Keep or change:

    • 8 MiB serialized / 32 MiB source limits;
    • status 500;
    • catalog_unsafe, catalog_too_large, catalog_source_too_large;
    • authentication-before-method ordering;
    • byte-identical versus equivalent cross-route content.
  4. Are the three inbound credential forms and the secondary-loopback 404 behavior accepted as implemented?

Process actions also needed:

  • Wibias's existing CHANGES_REQUESTED review is still active on the old head (a5737ffaa); please re-review the current head (5586e4e01).
  • src/server/auth-cors.ts is on the sponsored surface. After security review, please apply maintainer-sponsored to this PR if approved; the issue's label does not transfer.
  • Cross-platform CI and React Doctor are still awaiting fork-run approval and have not executed.
  • One targeted local command remains unresolved after three timeout-only failures with no assertion mismatch; repository CI must settle it.
  • CodeRabbit skipped the Draft and no Codex review exists yet.

Nothing further will be implemented until a maintainer chooses the policy. The PR will remain Draft.

@Wibias
Wibias dismissed their stale review August 14, 2026 20:46

dismiss

@Wibias Wibias left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lidge-jun

Copy link
Copy Markdown
Owner

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.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 37 / 80

Draft이고 intake: hygiene-blocked가 붙어 있으며 mergeable이 UNKNOWN이다. #809의 방향 자체는 맞다. 원격 Codex 클라이언트가 이미 들고 있는 data-plane 자격 증명으로 카탈로그를 읽고, /api/*에는 예외를 만들지 않는다는 분리와, GET /api/catalogGET/HEAD /v1/catalog가 같은 src/codex/catalog/distribution.ts를 거치게 한 경계는 이 PR의 가장 단단한 부분이다. materializeCatalogDistribution이 디스크 읽기 상한, 안전 판정, 직렬화, x-opencodex-codex-version을 한곳에서 소유하고 두 라우트는 전송만 담당한다. src/server/index.ts도 인증을 메서드보다 먼저 보고 익명 POST는 401, 통과한 호출자의 비읽기 메서드는 405+Allow로 나눈다. withDataPlaneCatalogResponseHeaders가 성공·HEAD·오류 모두에 Cache-Control: no-storeX-Content-Type-Options: nosniff를 붙이는 것도, 캐시된 404 catalog_not_found가 나중에 생성된 카탈로그를 가리는 일을 막는다. loopback allowlist에 넣지 않은 것도 의도대로다.

다만 점수 한가운데에 두지 못하는 이유는 안전 판정이 저자가 스스로 적은 그대로 휴리스틱 denylist이기 때문이다. isCatalogDocumentSafeToDistribute는 키를 소문자화한 뒤 비알파뉴메릭을 전부 버리고 SENSITIVE_KEY_EXACT/SENSITIVE_KEY_SUFFIXES와 맞추며, 문자열 값은 이메일 모양·ocx_/sk-/ghp_/eyJ·bearer …·홈 디렉터리 경로만 본다. 주석과 structure/05_gui-and-management-api.md가 이미 적시한 대로, 평범한 필드에 들어간 raw provider URL, 인식 불가 형식의 hex 토큰, 계정으로 이름 붙지 않은 임의 식별자, D:\ocx\config.json 같은 홈 밖 절대 경로는 통과한다. 키 정규화도 ASCII만 남기므로 키에 비ASCII/동형 문자가 섞이면 apikey 접미사가 깨져 그대로 배포될 수 있다. 오류가 매칭된 키·값을 메아리내지 않는 것은 맞지만, 그건 판정이 맞았을 때의 이야기다. data-plane 자격 증명을 가진 모든 원격 호출자가 이 문서를 받게 되므로, denylist가 놓친 값은 /api/catalog보다 노출면이 넓다.

src/server/auth-cors.ts는 후원 인증 표면이다. AUTH_MATRIX/v1/catalog/v1/models와 같은 bearer/dedicated/x-api-key 허용으로 넣은 것과, 전역 Access-Control-Allow-Methods에 HEAD를 추가한 것은 이 라우트만의 국소 변경이 아니다. HEAD 추가는 preflight된 HEAD /v1/catalog를 브라우저가 막지 않게 하려는 것이 분명하지만, CORS 허용 메서드 목록은 서버 전 경로에 적용된다. 이 파일 변경은 maintainer-sponsored 없이 들어오면 안 되는 축이고, 지금 라벨 상태와도 맞물린다. 관리 라우트 src/server/management/model-routes.ts가 이제 같은 materializer를 쓰면서, 예전처럼 파싱만 되면 무엇이든 주던 /api/catalog가 unsafe/source-too-large에서 500을 내게 된 것도 동작 변경이다. 공통 경로를 유지하려면 필요하지만, 관리 평면에서 예전 문서를 보던 운영자에게는 회귀로 보일 수 있다.

크기 거절은 의도가 분명하다. 디스크 원본은 CATALOG_SOURCE_MAX_BYTES(32MiB)로 JSON.parse 전에 끊고, data-plane 응답은 DATA_PLANE_CATALOG_MAX_BYTES(8MiB)로 자른다. pretty-print된 33MiB 파일이 compact 후에는 8MiB 아래일 수 있으니 catalog_source_too_largecatalog_too_large를 가른 것은 맞다. readCatalogSourceBounded가 이미 연 descriptor에서 fstat하고 그 크기만큼만 readSync하는 것도, 그 사이 파일이 커져도 무한 읽기가 되지 않게 한다. 짧은 읽기는 subarray(0, offset)만 쓰므로 allocUnsafe도 그 범위 안에서는 새지 않는다. 다만 두 거절과 catalog_unsafe가 모두 500인 점은 저자도 열어 둔 정책이다. 클라이언트가 보기엔 서버 버그이고, 413이나 명시적 거절 코드가 더 정확할 수 있다. HEADContent-Length를 GET과 맞추는 계약은 유지해야 한다.

tests/v1-catalog-route.test.ts가 업스트림 스냅샷 호환, 자격 증명 미포함, 알려진 허점까지 핀한 것은 이 설계를 정직하게 고정한다. 반대로 tests/api-key-attribution.test.ts의 미해결 로컬 타임아웃 3건과 미완 체크리스트, 6개 로케일 docs-site 동시 수정, hygiene-blocked는 아직 머지선이 아니다. Wibias의 CHANGES_REQUESTED도 본문에 남아 있다.

해결방안: 배포 문서는 denylist가 아니라 필드 allowlist 투영으로 직렬화하거나, 후보 문자열을 살아 있는 config/자격 증명 저장소와 정확히 대조하는 쪽으로 정책을 먼저 고정하라. 전자는 알 수 없는 Codex 필드를 떨어뜨릴 수 있고 후자는 materializer가 config에 의존하게 되므로, 그 트레이드오프를 structure/05_gui-and-management-api.md의 열린 결정으로 남기지 말고 머지 전에 받아라. 휴리스틱을 유지한다면 비ASCII 키 정규화와 홈 밖 경로를 “알려진 허점”으로만 두지 말고, 배포 불변조건이 아직 증명되지 않았다는 점을 공개 계약에 명시하라. auth-cors.ts는 스폰서 승인 후 넣고, 전역 HEAD CORS가 다른 경로에 주는 영향을 한 줄로 기록하라. catalog_unsafe/catalog_too_large/catalog_source_too_large의 HTTP 상태를 500에서 분리할지 유지자 결정을 받고, attribution 테스트 타임아웃과 hygiene를 해제한 뒤에만 Ready로 올려라.

이 댓글은 grok-bot이 작성했습니다

@Ingwannu Ingwannu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cursor

cursor Bot commented Aug 28, 2026

Copy link
Copy Markdown

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 GET /v1/catalog on the existing data-plane admission matrix (accepts x-opencodex-api-key or a dedicated data key, rejects the admin token) and shares the serializer with /api/catalog. That covers the least-privilege split this PR targeted. The heuristic denylist in src/codex/catalog/distribution.ts is intentionally not carried forward, per the earlier review on this branch.

Closing in favour of #2772 rather than asking you to rebase a branch that is ~2190 commits behind dev and conflicts in src/server/index.ts, src/server/auth-cors.ts, and model-routes.ts. Issue #809 stays open and still tracks the requirement, so nothing is lost if the stack changes shape.

@lidge-jun lidge-jun closed this Aug 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request intake: hygiene-blocked Deterministic PR hygiene checks failed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants