Fix #2372: exempt synthetic slice integration pushes from role-path allowlist - #2381
Conversation
…llowlist Synthetic-session slice integration-branch creation pushes (#2368) bypass the pipeline-session push block (#2370) but were still hitting the role-based path-allowlist check at gateway/gateway.py:1515 because that gate was the only file-restriction gate not keyed on ``is_infrastructure_push``. When ``get_changed_files_in_push`` falls back to a ``main``-based diff (the target ref doesn't exist yet on origin for a branch-creation push), the changed-files set surfaces every file modified on the parent branch's history — drafts, contracts, brc-history — none of which the ``coder`` role can write, so the push is falsely blocked. Mirror the existing ``not is_infrastructure_push`` gate already present on the anchor (l.1785), phase (l.1822), and agent-restriction (l.1580) checks. The gate is set by the synthetic-session + slice-shape branch exemption (l.1316-1318), so this only widens the exemption surface for pushes that already cleared the launcher-secret check.
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
Targeted, well-justified fix. The role-based path-allowlist gate was the lone exception to the not is_infrastructure_push pattern that the other three file-restriction gates already use, and the failure mode (slice integration-branch creation pushes 403'ing because get_changed_files_in_push falls back to a main-based diff) is exactly the symptom that pattern was designed to prevent.
I traced the change end-to-end and could not find any way for the new exemption to be reached by an unprivileged session:
is_infrastructure_pushis set toTrueonly atgateway/gateway.py:1297(branch in{CHECKPOINT_BRANCH, PIPELINE_STATE_BRANCH}) orgateway/gateway.py:1318(synthetic session +_SLICE_INTEGRATION_BRANCH_REmatch).Session.syntheticis settable only via/api/v1/sessions/create, which is gated on@require_launcher_auth(gateway/gateway.py:7986-7987). The identity checkgetattr(g.session, "synthetic", False) is True(gateway/gateway.py:1316) defends against truthy-but-not-Truemocks/strings.- The downstream agent-restriction gate (
gateway/gateway.py:1588), anchor gate (gateway/gateway.py:1793), and phase gate (gateway/gateway.py:1830) already gate onnot is_infrastructure_push— so this change does not widen the exemption surface beyond what already existed for those three gates.
The new tests are strong:
test_synthetic_slice_branch_skips_role_path_allowlistmockscheck_file_restrictionsto returnallowed=Falseandget_changed_files_in_pushto return forbidden files, then asserts both mocksassert_not_called(). That proves the gate is bypassed, not just that the happy path mocks line up. Better than asserting status_code alone.test_role_path_allowlist_still_enforced_for_non_infrastructure_pushesis a real regression guard: non-pipeline session, ordinaryegg/-prefixed branch, forbidden file → 403 withPath allowlist violation.
No blocking issues. A few small non-blocking observations:
Non-blocking
-
PR description line numbers are stale by +8. The body says "anchor (l.1785), phase (l.1822), and agent-restriction (l.1580)". After this PR's 8-line comment block lands, those gates are at
gateway/gateway.py:1793,1830, and1588. The numbers in the body match the pre-patch layout. Cosmetic — the in-source comment correctly avoids line numbers. -
Comment justification slightly conflates two motivations. The new comment block (
gateway/gateway.py:1510-1517) attributes the exemption to "the changed-files diff againstmainwould otherwise pull in every file modified on the parent branch's history". That rationale is specifically the slice integration-branch failure mode (get_changed_files_in_pushfallback atgateway/git_client.py:1448). It does not describe why the role check is also skipped for legitimateegg/checkpoints/v2/egg/pipeline-statepushes — those branches exist on origin and have an orphan/disjoint history, which is a different reason to skip role-based file restrictions. Worth acknowledging both motivations in one sentence so a future reader doesn't think the only justification is the new-branch fallback. -
No audit event for the per-gate skip. When a slice integration push is exempted, the operator sees
push_slice_integration_exempt(gateway/gateway.py:1320) andpush_infrastructure_exempt(gateway/gateway.py:1357). Neither names which downstream gates were skipped. The other three gates have always behaved this way, so this PR is consistent — just flagging that the audit trail tells you "exempted" without telling you "exempted from the role / phase / anchor / agent-restriction gates specifically". Not worth a code change here, but if the audit event ever grows abypassed_gates: [...]field, the role gate should be added at the same time.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Per egg-reviewer feedback on PR #2381: the comment block at gateway.py:1511 conflated two distinct motivations for skipping the role-based path-allowlist on infrastructure pushes. Split them out: (1) checkpoint/pipeline-state branches are orphan/disjoint-history infrastructure writes where role restrictions don't conceptually apply, and (2) slice integration-branch creation pushes diff against `main` because the target ref doesn't exist yet, which would surface the parent branch's full history.
Review feedback dispositionThanks for the thorough end-to-end trace.
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review
Re-reviewed the delta from ebc8963 → 17ce05d. The only change is a comment-block rewrite at gateway/gateway.py:1511-1523; no executable code changed.
Disposition of prior review
- #2 (comment conflated two motivations) — fixed cleanly. The new comment block now enumerates both reasons explicitly: (1)
egg/checkpoints/v2/egg/pipeline-stateare orphan/disjoint-history orchestrator infrastructure where role-based file restrictions don't conceptually apply, and (2) synthetic-session slice integration-branch creation pushes diff againstmainbecause the target ref doesn't yet exist, which would otherwise surface the parent branch's full file history. Both rationales are accurate against theis_infrastructure_pushset sites atgateway/gateway.py:1297andgateway/gateway.py:1318, and against theget_changed_files_in_pushmain-fallback atgateway/git_client.py:1448. The trailing sentence about the role gate making the pattern symmetric with the downstream anchor/phase/agent-restriction gates is preserved. - #1 (stale PR-description line numbers) —
disagree (cannot edit PR body across authors). Acceptable. The author logged the corrected post-patch line numbers in the disposition comment for the audit trail, and the in-source comment correctly avoids embedded line numbers so it cannot drift. - #3 (audit event does not name skipped gates) — author explicitly opted to leave as-is per my "Not worth a code change here" framing. Fine; if the audit envelope ever grows a
bypassed_gates: [...]field, the role gate should be added there.
New-commit scrutiny
- The added comment block is comment-only — verified by reading lines 1490-1610 of the post-patch file. The role-check gate at
gateway/gateway.py:1529(if session_role and not is_infrastructure_push) is byte-for-byte unchanged. - The wording is accurate. "Orphan/disjoint-history branches written by orchestrator infrastructure" matches the checkpoint/pipeline-state set; "the target ref doesn't exist yet, which would otherwise pull in every file modified on the parent branch's history (drafts, contracts, brc-history, ...)" matches the slice integration-branch failure mode that motivated #2372.
- Only
gateway/gateway.pyis touched in the new commit, no test changes — appropriate for a comment-only edit. The existing tests added in the prior commit (test_synthetic_slice_branch_skips_role_path_allowlist,test_role_path_allowlist_still_enforced_for_non_infrastructure_pushes) continue to exercise both sides of the gate.
No new issues. LGTM.
— Authored by egg
|
egg review completed. View run logs 3 previous review(s) hidden. |
1 participant
Summary
gateway/gateway.py:1515was the only file-restriction gate not keyed onis_infrastructure_push. After Fix #2368: synthetic-session exemption for slice integration-branch pushes #2370 exempted synthetic-session slice integration-branch pushes from the pipeline-session push block, the next gate in the chain — the role check — fired on a logical no-op branch-creation push becauseget_changed_files_in_pushfalls back to amain-based diff when the target ref doesn't exist yet, surfacing the parent branch's full history (drafts, contracts, brc-history). None of those paths are writable by thecoderrole, so all 15 slice integration-branch creations onissue-2261-v3failed withPush denied: Role 'coder' cannot modify: .egg-state/brc-history/....not is_infrastructure_pushto the role check gate, mirroring the existing gates on the anchor (l.1785), phase (l.1822), and agent-restriction (l.1580) checks. Becauseis_slice_integration_push=Truealready promotesis_infrastructure_push=True(l.1316-1318), this only widens the exemption surface for pushes that already cleared the launcher-secret + slice-shape regex.gateway/tests/test_pipeline_push_block.py::TestSliceIntegrationBranchExemption:test_synthetic_slice_branch_skips_role_path_allowlist— synthetic slice push with would-be-blocked changed_files returns 200, and assertsget_changed_files_in_push/check_file_restrictionsare never called (verifies the gate, not just a happy-path mock).test_role_path_allowlist_still_enforced_for_non_infrastructure_pushes— regression guard: a non-pipeline session pushing forbidden files still returns 403 with the path-allowlist error.Option 1 from the issue (smallest, symmetric with #2370). Options 2 (branch-creation-only detection) and 3 (new orchestrator-infra role) are not pursued here — option 2 touches security-critical attribution code; option 3 adds role configuration for a single call site. Neither is justified for closing this regression.
Test plan
.venv/bin/pytest gateway/tests/test_pipeline_push_block.py— 31 passed (29 existing + 2 new)..venv/bin/pytest gateway/tests/— 3167 passed.make test— 81182 passed; 1 unrelated failure + 2 unrelated errors are pre-existing local-environment artifacts (staleintegration_tests/deployment_validation/__pycache__, port-binding conflict).make lintclean for the changed files (gateway/gateway.py,gateway/tests/test_pipeline_push_block.py).mcp__egg__advance_phase target_phase=implement force=truefor pipelineissue-2261-v3to retry the slice loop.Refs