fix(api): close scope-enforcer body-injection + header-target gaps (P1)#719
Conversation
Addresses three P1 findings from the #589 bot review: extractLockTargets resolved authorization targets from path params and the JSON body only, with the body taking precedence. This allowed: • Scope bypass — a caller could hit a path-scoped write (e.g. PATCH /instances/:realId) and inject body.instanceId pointing at an allowlisted instance, so allowlist + require_genie_signature checks evaluated against the wrong instance. • Missed enforcement / false 403 — header-scoped routes (POST /turns/close targets via x-omni-instance / x-omni-chat) were invisible to the extractor, so signature enforcement was skipped and allowlisted profile keys were wrongly denied. Fix: • extractLockTargets now also reads x-omni-instance / x-omni-chat (via the new readHeaderTargets helper) and applies precedence path > header > body. Route-derived targets (path, header) are trusted; the caller-controllable body can no longer override them and is used only as a last resort. • require-signed-instance + scope-enforcer middleware both pass header targets through. Tests: path/header beat conflicting body targets; headers populate targets for header-scoped routes; body still used when no path/header target.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
vasconceloscezar
deleted the
fix/scope-enforcer-header-targets-precedence
branch
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces header-derived targets (x-omni-instance and x-omni-chat) to the scope enforcer middleware, ensuring that route-derived targets (path parameters and headers) take precedence over the request body to prevent scope bypass. It also adds comprehensive unit tests to verify this precedence logic. The review feedback suggests simplifying the normalization and extraction of these header targets in scope-enforcer.ts by utilizing the logical OR (||) operator to reduce verbosity and improve readability.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| export function readHeaderTargets(c: Context<{ Variables: AppVariables }>): HeaderLockTargets { | ||
| const norm = (v: string | undefined): string | null => (v && v.length > 0 ? v : null); | ||
| return { | ||
| instance: norm(c.req.header('x-omni-instance')), | ||
| chat: norm(c.req.header('x-omni-chat')), | ||
| }; | ||
| } |
There was a problem hiding this comment.
The norm helper function is recreated on every call to readHeaderTargets and adds unnecessary complexity. Since c.req.header returns string | undefined, we can simplify this by using the logical OR (||) operator to default to null directly, which is cleaner and more idiomatic.
export function readHeaderTargets(c: Context<{ Variables: AppVariables }>): HeaderLockTargets {
return {
instance: c.req.header('x-omni-instance') || null,
chat: c.req.header('x-omni-chat') || null,
};
}| const headerInstance = headers?.instance && headers.instance.length > 0 ? headers.instance : null; | ||
| const headerChat = headers?.chat && headers.chat.length > 0 ? headers.chat : null; |
There was a problem hiding this comment.
Since headers.instance and headers.chat are already normalized to string | null | undefined, we can simplify the check for non-empty strings using the logical OR (||) operator. This avoids the verbose length check and is much more readable.
| const headerInstance = headers?.instance && headers.instance.length > 0 ? headers.instance : null; | |
| const headerChat = headers?.chat && headers.chat.length > 0 ? headers.chat : null; | |
| const headerInstance = headers?.instance || null; | |
| const headerChat = headers?.chat || null; |
1 participant
Resolves the three P1 authorization findings from the #589 (
dev → main) bot review, verified still-present on currentdev.Problem
extractLockTargets()— the helper feeding both the allowlist enforcer andrequire-signed-instance— resolved targets from path params + JSON body only, body-first. Two failure modes:scope-enforcer.ts) —PATCH /instances/:realIdwithbody.instanceId = <allowlisted-id>made allowlist +require_genie_signatureevaluate against the injected id, not the path target.require-signed-instance.ts) — header-scoped routes (POST /turns/closetargets viax-omni-instance/x-omni-chat) returned no instance target, so the signature gate was skipped onrequireGenieSignatureinstances.scope-enforcer.ts) — same root cause: header targets were invisible, so allowlisted profile keys were wrongly denied on header-scoped routes.Fix
extractLockTargetsnow also readsx-omni-instance/x-omni-chat(newreadHeaderTargets(c)helper) and applies precedence path > header > body. Route-derived targets (path, header) are trusted; the caller-controllable body can no longer override them — it's used only when neither path nor header supplies the target (outbound sends, explicit-body turn close).scope-enforcerandrequire-signed-instancemiddleware pass header targets through.Tests
body.instanceId/body.chatId(bypass guard)x-omni-*headers populate targets for header-scoped routes🤖 Generated with Claude Code