Skip to content

fix(combos): enforce API-key scope on decision providers - #6450

Closed
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:fix/jev-decision-admission-scope
Closed

luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:fix/jev-decision-admission-scope

Conversation

@luvs01

@luvs01 luvs01 commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Enforce the parent API key's resolved provider/model scope before TypeSafe or a configured System One decision provider reads credentials, extracts request state, or sends a request.
  • A refused optional decision keeps the existing first-eligible inference fallback; normal inference remains independently scoped. Unrestricted management probes and the admission-preserving decision-model path retain their behavior.
  • Add offline regression coverage for denied providers and models with zero credential reads/sends, normalized allowed destinations, and omitted/explicit canonical TypeSafe selectors. Related feature context: feat(combos): JEV decision methods — TypeSafe, System One server, or any opencodex model #6364 and [Feature]: Support self-hosted JEV-compatible decision endpoints #6268; no duplicate follow-up fix was found in targeted issue/PR searches.

Verification

  • Base: 10428d0120cbceeff997508a28a7a50c4007f7dc, confirmed current dev before publication.
  • New scope suite was red first: 2 pass / 5 fail; after the fix: 7 pass / 0 fail.
  • bun test tests/server/jev-decision-scope.test.ts tests/routing/jev-decision-destination.test.ts tests/routing/jev-decision.test.ts tests/routing/jev-model-backend.test.ts tests/server/server-jev-model-decision-e2e.test.ts: 68 pass / 0 fail, 361 assertions. All decision/provider work uses explicit synthetic executors; no live provider requests.
  • bun run typecheck, bun run structure:check, bun run privacy:scan, and git diff --cached --check: passed.
  • bun test tests/test-layout.test.ts tests/test-layout-tooling.test.ts tests/ci-workflows/file-size-ratchet.test.ts: 27 pass / 0 fail, 638 assertions.
  • Independent read-only boundary investigation and one candidate bypass/regression review completed; no concrete remaining issue identified in this scope. Explicit maintainer security review is still requested.
  • The full/changed suite was not run: concurrent worktrees and the no-live-provider constraint require the repository's focused-validation exception. Exact-head Cross-platform CI has passed; maintainer security review remains required before merge.

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.

Exact-head review evidence

Cross-platform CI completed successfully for ed1b70b55eb94b521c00b78a8a3c30659a460ef6. The branch is one commit behind current dev, within the repository's readiness tolerance; that upstream change affects Windows service recovery, outside this diff. The focused local validation and documented full-suite exception above remain accurate. No open Codex/CodeRabbit review threads were present at attestation. CodeRabbit's previous green Draft status was a review skip; substantive review is now requested, and maintainer security review remains required before merge.

Summary by CodeRabbit

  • Security
    • Decision-provider destinations are checked against the API key’s allowed provider and model scope before credentials are read or requests are sent. Denied optional decisions retain the existing inference fallback.
  • Tests
    • Added coverage for allowed and denied destinations, including case-normalized model IDs and credential-read behavior.

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (2)
src/AGENTS.md — auto-discovered
structure/AGENTS.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 71134cbb-e343-4221-b1c1-56cb157ffec8

📥 Commits

Reviewing files that changed from the base of the PR and between 10428d0 and ed1b70b.

📒 Files selected for processing (6)
  • scripts/test-layout/layout.json
  • src/combos/jev.ts
  • src/server/responses/core-combo.ts
  • structure/providers/jev-decision.md
  • tests/fixtures/test-layout-expected.json
  • tests/server/jev-decision-scope.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

JEV decision destinations are now checked against the current admission’s provider/model scope before credentials are resolved. Denied destinations use the existing inference fallback. Tests cover denied and allowed scopes, destination checks, and normalized model IDs.

Changes

JEV destination scope checks

Layer / File(s) Summary
Authorize JEV decision destinations
src/combos/jev.ts
ResolveJevDecisionOptions accepts an authorization callback. JEV checks canonical and configured destinations before resolving credentials, and maps a denied destination to the invalid fallback gate.
Wire and verify admission scope checks
src/server/responses/core-combo.ts, tests/server/jev-decision-scope.test.ts, tests/fixtures/test-layout-expected.json, scripts/test-layout/layout.json, structure/providers/jev-decision.md
The response path checks destinations against the current admission scope. Tests cover denied and allowed destinations, credential reads, fetch sends, and normalized model IDs. The test is registered in the server group, and the provider documentation describes the scope check and fallback behavior.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant executeComboResponses
  participant routeAllowedByScope
  participant jevDecisionEndpoint
  participant credentialResolver
  participant resolveJevDecision
  executeComboResponses->>jevDecisionEndpoint: Pass admission-scope authorization callback
  jevDecisionEndpoint->>routeAllowedByScope: Check proposed provider and model
  routeAllowedByScope-->>jevDecisionEndpoint: Return allowed or denied
  alt Destination allowed
    jevDecisionEndpoint->>credentialResolver: Resolve decision credentials
  else Destination denied
    jevDecisionEndpoint-->>resolveJevDecision: Return null
    resolveJevDecision-->>executeComboResponses: Use invalid fallback gate
  end
Loading

Suggested reviewers: lidge-jun

Merge Risk: ⚪ Minimal · up to ed1b7

The reviewed scope checks preserve the intended decision paths and fallback behavior. No actionable merge-blocking issue remains after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to ed1b7

This narrowly scoped change strengthens outbound authorization while retaining independent checks on inference requests. No new security issue was identified in the reviewed paths, but broader access-control and deployment coverage remains incomplete.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed exposure is an admitted request selecting a combo that uses a canonical or configured decision provider. The new control limits decision-provider credential use and request-data disclosure; it does not grant management authority or authorize the eventual inference destination.

Security Findings and Attack Paths

  • observed — The previously unchecked optional decision-provider path is narrowed by this PR. In the traced flows, denial cannot send to the refused decision destination, and a later inference fallback remains subject to concrete-route authorization. No introduced or worsened bypass was established in those flows.

Trust Boundaries and Controls

  • observed — The provider callback and the inference-backed decisionModel path enforce authorization through different mechanisms. Provider resolution uses the new predicate; decisionModel invocation forwards the original admission into normal response handling and suppresses caller credential channels.
  • observed — Unconfigured environment or loopback admissions retain their existing unrestricted scope behavior. Management decision probes also omit the data-plane predicate; this callback-free behavior predates the PR, and its outer authentication guard was not directly hydrated.

Resilience and Maintainability Implications

  • observed — Denial creates no decision transport to clean up. Initial and later inference children retain the parent admission and re-enter route authorization. Nested decision-model invocation retains its existing cancellation propagation and finally-based resource release.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. (3 skipped: 3… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: enforcing the parent API key's provider/model scope on JEV decision providers.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. (3 skipped: 3 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 2, 2026
@luvs01
luvs01 marked this pull request as ready for review October 2, 2026 13:08
robin-bially pushed a commit to robin-bially/opencodex that referenced this pull request Oct 3, 2026
…-jun#6450)

Check concrete decision destinations before resolving credentials or extracting request state.
Preserve scoped inference fallback and unrestricted management probes.

Carries lidge-jun#6450 by @luvs01.
Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
@lidge-jun

Copy link
Copy Markdown
Owner

Superseded by the integration in #6487, with reviewed follow-up fixes in #6490 and Windows validation repairs in #6494/#6495, all merged into dev.

Decision-provider API-key scope enforcement and negative regression coverage were carried.

Original carry commit: ce87c68077216ad0275036315410d1a1eab9e0c0. Attribution to @luvs01 is preserved in the integration history and merge trailers. The final integrated candidate passed the complete cross-platform CI run.

Closing this PR as superseded, not claiming that its original head was merged. Thank you for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working superseded

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants