Uh oh!
There was an error while loading. Please reload this page.
fix(aca): harden agent runner transport security - #426
Merged
Conversation
added 2 commits
August 13, 2026 09:07
Add opt-in transport-token auth gate on the ACA runner's /execute endpoint (X-Conductor-Runner-Token), narrow inner_provider_settings to an explicit allowlist, add an optional base_url allowlist, and default the runner to bind loopback only. Health endpoint reports auth status without requiring auth itself.
Blocking: - A non-string inner_provider_settings.base_url crashed check_inner_provider_settings with an unhandled AttributeError, returning a retryable 500 instead of a clean 400. Add an explicit type check (unconditional, not gated behind the allowlist) and correct the _unwrap docstring's false assumption that base_url always arrives as a str. - execute_endpoint's docstring falsely claimed the token gate runs "before any request parsing" — FastAPI validates the request body before the handler runs. Reworded to state the actual contract: the gate protects execution, not the parser, and a malformed body from an unauthenticated caller still gets FastAPI's 422. Recommendations applied: - R1: moved RUNNER_TOKEN_HEADER to providers/aca_protocol.py (a genuine leaf both sides already import), removing providers/aca.py's import of conductor.aca_runner.auth — that import pulled FastAPI, Starlette and the Copilot SDK onto every host process (822 -> 1054 modules) and created a latent import cycle. auth.py now re-exports the constant for backward compatibility. - R8 (partial): corrected "401/400" -> "400" for the allowlist rejection (it's always 400) in three docstrings, and fixed the /interrupt-endpoint description in providers/aca.py and the "/execute is chosen over Authorization" sentence in AGENTS.md (the intended subject was the header name, not the endpoint). Added regression tests for both blockers (non-string base_url with an allowlist configured -> 400 not 500; malformed body + no token -> 422 not 401). Skipped recommendations R2-R7, R9-R13: real and well-argued, but each grows the diff (new ProviderError raises changing startup behavior, new logging, restructured tests, a new shared type/dataclass) beyond what a review-response pass should carry without its own review.
Jason Robert (jrob5756)
marked this pull request as ready for review
August 13, 2026 13:30
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Hardens the ACA agent runner's transport surface: opt-in token auth on
/execute, an allowlist forinner_provider_settings, an optionalbase_urlallowlist, and loopback-only binding by default.Closes#396