Repository navigation
fix(security): keep the Kiro auto-import route out of the manage-scope bypass - #16691
Merged
diegosouzapw merged 1 commit intoOct 15, 2026
Merged
Conversation
…e bypass /api/oauth/kiro/auto-import reads host-local Kiro credential files (kiro-cli data.sqlite3, ~/.aws/sso/cache, the Kiro profile.json). It is LOCAL_ONLY, but it was reviewed as bypass-eligible, so an operator-set LOCAL_ONLY manage-scope bypass list could expose it to a remote manage-scoped API key. Add it to the never-bypassable set in spawnCapablePrefixes.ts, next to the other non-spawn entries (/api/discovery/, /api/issue-agent/), so the shared isSpawnCapableBypassPrefix() predicate refuses it at all three layers: the settings PATCH refine (BYPASS_PREFIX_NOT_ALLOWED), the runtime snapshot normalization (also dropping covering parents such as /api/oauth/kiro/), and the request-time isLocalOnlyBypassableByManageScope check. The authz inventory and the dashboard pick it up as "cannot be made bypassable" through the same list. Tests cover the PATCH rejection (exact path and /api/oauth/kiro/), the stored-list normalization, and a manage-scoped key from a non-loopback address getting 403 LOCAL_ONLY both through the settings pipeline and with a raw snapshot that skips normalization. The drift inventory moves the route from reviewed-eligible to the deny-list.
Contributor
CI Coverage Report
Coverage artifact was not available for this run. |
diegosouzapw
added a commit
that referenced
this pull request
Oct 18, 2026
…port, route-guard locale proof, parseModel alias, family-fallback consent (#17007) * test(sse): align the parseModel cache-key expectation with the #16811 Opus 4.5 contract #16811 made the provider-qualified `anthropic/claude-opus-4.5` alias keep its `anthropic` prefix and resolve to the dashed wire ID (`claude-opus-4-5`), leaving only the bare alias on the dated snapshot. It updated its own regression test but not this sibling from #16341, which still expected `provider: null`, `claude-opus-4-5-20251101`, `isAlias: true`. The compat on/off results still differ, which is what the cache-key test is about. Refs #15306 * test(docs): bound the route-guard locale proof by the current paragraph order #16691 moved the deny-list test reference ahead of the regex companion constant in docs/security/ROUTE_GUARD_TIERS.md; the mirrors followed, but this test (from #16504) still searched for that reference AFTER `SPAWN_CAPABLE_PATTERN_ANCESTORS` and reported "missing localized policy proof" for all 66 locales. The window now runs from the Kiro auto-import deny-list entry to the regex companion and asserts the deny-list proof inside it; all 66 mirrors pass and the English-fallback and hash checks are unchanged. Refs #15306 * fix(mcp): keep apiKeyPolicy and next/headers out of the MCP server bundle's import time `git bisect` on tests/unit/build/mcp-bundle-startup.test.ts lands on #16810, which made src/shared/utils/modelFallbackAuthorization.ts import `resolveEnforcedPolicyKey` from apiKeyPolicy. That module is reached from open-sse (modelFamilyFallbackPolicy), so the MCP server bundle started carrying apiKeyPolicy → apiAuth → `next/headers` (esbuild hoists the external import; plain Node cannot resolve it) and, once that is out of the way, an async init cycle around sse/services/auth that leaves the import with an unsettled top-level await (exit 13). #16499 then opened a second path into apiKeyPolicy through keyConnectionScope → comboTargetKeyPolicy on the embeddings service. Load the three edges lazily, at the call sites that are already async: modelFallbackAuthorization and keyConnectionScope import apiKeyPolicy / comboTargetKeyPolicy inside the functions that use them, and apiAuth imports `next/headers` only when it actually falls back to the request cookie store. The bundle imports again on Node 24 and the request paths are unchanged. Refs #15306 * test(chat): opt the family-fallback effort cases into the #16810 cross-version consent #16810 gated the internal cross-version family hop behind `x-omniroute-allow-cross-version-fallback: true`; without it an empty-content answer from claude-sonnet-4.6 is returned as 502 instead of being recovered on claude-sonnet-4.5. The three "empty family fallback preserves only explicit effort" cases (from #13720) exercise exactly that hop and never sent the header, so they failed with `502 !== 200` after 24s. Send the opt-in; the effort propagation they assert is unchanged. Refs #15306
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/api/oauth/kiro/auto-importreads credential files from the host's home directory: the kiro-clidata.sqlite3,~/.aws/sso/cacheand the Kiroprofile.json. The route is LOCAL_ONLY, but the drift inventory had reviewed it as bypass-eligible. That meant an operator-configured LOCAL_ONLY manage-scope bypass list could open it to a remote manage-scoped API key.This PR adds the route to the never-bypassable set in
src/shared/constants/spawnCapablePrefixes.ts. It goes next to the other non-spawn entries already on that list (/api/discovery/,/api/issue-agent/), with a comment saying it is a host-credential read and not a spawn. Because the sharedisSpawnCapableBypassPrefix()predicate reads that list, all three layers now refuse the route:BYPASS_PREFIX_NOT_ALLOWEDfor the exact path and for any entry that covers it (e.g./api/oauth/kiro/)./api/oauth/was already refused, because it covers/api/oauth/cursor/auto-import.normalizeAuthzBypass): drops the route, and any parent of it, from a stored bypass list.isLocalOnlyBypassableByManageScope): refuses it even when the snapshot was written without going through normalization.The authz inventory and the dashboard read the same list, so they now show the route as "cannot be made bypassable" with no further changes.
Tests (TDD)
New cases in
tests/unit/settings/authz-bypass.test.ts:/api/oauth/kiro/auto-importgets 400BYPASS_PREFIX_NOT_ALLOWED, and so does a PATCH with/api/oauth/kiro/; in both cases the persisted list and the live snapshot stay unchanged;["/api/mcp/", "/api/oauth/kiro/auto-import", "/api/oauth/kiro/"]normalizes to["/api/mcp/"];LOCAL_ONLYon the route. This is checked through the settings pipeline and again with a raw snapshot that skips normalization. A control asserts that the same key is still allowed through the default/api/mcp/bypass.Drift inventory (
tests/unit/authz/local-only-bypass-spawn-denylist.test.ts): the route moves fromREVIEWED_BYPASS_ELIGIBLEto the deny-list cases, and/api/oauth/kiro/is added to the parent-prefix cases. The count pin inspawn-capable-prefixes-client-safe.test.tsgoes from 51 to 52.authz-bypass.test.ts(PATCH returned 200, the snapshot kept the entry, the remote manage key was allowed) and 4 failing in the drift test.Similar routes (follow-up, not changed here)
/api/oauth/cursor/auto-import: already on the deny-list (it spawnswhich cursor)./api/oauth/cliproxy-import: reads~/.cli-proxy-apiOAuth files. It is not LOCAL_ONLY: the broad/api/oauth/PUBLIC prefix classifies it PUBLIC and the handler only callsrequireManagementAuth, so a remote manage-scoped key can import those host credentials as connections. Tokens are not returned to the caller. The manage-scope bypass list does not reach it, so it needs its own tiering decision./api/providers/agy-auth/apply-local,/api/providers/{id}/(claude|codex)-auth/apply-local: these write into host CLI config and are on the ALWAYS_PROTECTED tier on purpose. They are a different class./api/sync/cloud: reads and rewrites~/.claude/settings.json. It holds config, not credentials.None of the other bypass-eligible LOCAL_ONLY prefixes (
/api/mcp/,/api/copilot/,/api/jobs*,/api/resilience/connections) read host credential files.Validation
npx prettier --checkon the touched files: cleannpx eslint --suppressions-location config/quality/eslint-suppressions.json <touched>: exit 0node scripts/check/check-complexity-ratchets.mjs --base-ref <merge-base>: OK (0 violations)npm run -s check:file-size: only the 4 known base overages, none in touched filesnpx tsc --noEmit -p tsconfig.json: 0 errors in touched files. The run reports 6545 errors, all in untouched files (bin/, open-sse/ streams, scripts/ad-hoc, etc.). A focused tsc over the 4 touched files also gave 0 errors in them; its one error is in the untouchedtests/helpers/managementSession.ts, which is identical to basenpm run check:docs-all: exit 0. An earlier run under heavy load hit the 180sreadCodeFactsspawn timeout, which is unrelated to this diff