feat(ado-proxy): add credential-isolated Azure DevOps reads - #1824
Merged
Conversation
The Azure CLI extension's injected prompt told agents that `az devops`, `az pipelines`, `az repos`, and `az boards` are "authenticated automatically from \ when the pipeline declares `permissions: read:`" and that list operations "Just Work". None of that is true: `permissions.read` authenticates the first-party Azure DevOps MCP backend and never populates `AZURE_DEVOPS_EXT_PAT` in the agent sandbox. Agents therefore burned turns on `az devops` calls that could only ever fail, and the docs pointed operators at `az login` as the fix - which would put a real Azure credential inside the sandbox, the exact outcome the threat model forbids. State the boundary instead: the extension ships the binary, not a credential. Direct the agent to the `azure-devops` MCP tools for authenticated reads and to the `missing-tool` safe output otherwise, and tell it explicitly not to sign in. The prompt-anchor test asserted on `AZURE_DEVOPS_EXT_PAT`, so it locked in the false claim; it now anchors on the auth boundary wording. Refs #1652, #1717. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
An ARM service connection's Azure RBAC scope does not constrain what its identity may do in Azure DevOps, so handing that credential to a Stage 1 agent grants whatever ADO permissions the identity happens to hold - which the workflow author neither chose nor can see. Exposing `az` directly cannot be made safe by configuration, because it depends on every consumer having scoped their ADO instance correctly. Take gh-aw's approach instead: a policy proxy that holds the credential so the agent never sees it. AWF points the agent's HTTP(S)_PROXY at a managed sidecar; Squid denies the protected ADO hosts to the agent, making the sidecar the only route to them. Stock `az`, curl, and the SDKs keep working unmodified. Deny-by-default and narrow on purpose: reads only, current organization/project/repository, 34 catalogued operations. Writes stay in SafeOutputs, where they are already reviewed. Catalog authored once, in Rust -------------------------------- The compiler emits the policy document and the sidecar consumes it, so the two must not diverge. `src/ado_proxy/catalog.rs` is authoritative; everything else is generated from it by `npm run codegen` - the JSON Schema, the TypeScript types the bundle compiles against, and a committed `catalog.gen.json` snapshot. A drift test re-runs the exporter and fails on any difference, and the bundle refuses to start if the mounted policy's `catalog_version` does not match the one compiled into it, so a stale policy fails closed rather than under-enforcing. Runtime is TypeScript, not Rust -------------------------------- A Rust implementation needs a TLS stack plus certificate minting (rustls + rcgen -> ring), which would make a native C toolchain a hard build requirement for the whole compiler. ado-aw is otherwise pure-Rust and must stay buildable without one. `ado-script` already ships 20 bundles through the same supply-chain mirror, and Node's built-in tls/http/net need no new dependency - the bundle has zero runtime deps. Enforcement ----------- Two paths, and only two. Non-protected destinations are byte-tunnelled to Squid untouched, so package feeds behave exactly as before. Protected destinations are TLS-terminated, normalized, authorized against the catalog, stripped of every client-supplied credential, and only then - after a complete allow decision - given the bearer. Deny-by-default is structural: unknown route, non-read method, unlisted query parameter, disabled capability, out-of-window api-version, or out-of-scope organization/project/repository all deny before the upstream is contacted, so a rejected request never exercises the credential. Request normalization refuses ambiguous targets rather than rewriting them, and the api-version is read from *both* the query string and the Accept header because ADO honours either - declaring one in each would otherwise let a request be checked as one operation and served as another. Two ADO endpoints (`az repos pr show`, `az boards work-item show`) are addressable by id alone, so their scope is validated against the response body; list endpoints are filtered. Response headers are allow-listed, so upstream Set-Cookie, WWW-Authenticate, and redirect Location never reach the agent. Denials return a WrappedException-shaped 403 that clients can surface, and infrastructure failures return 502 rather than 401/429/503, which msrest would retry. `runtime_available` stays false: nothing emits the sidecar or policy document yet, so authors must not be told the capability exists. Also extends `permissions.read` to accept the object form for explicit policy configuration, rejected at compile time until the runtime is wired. Tested with 139 tests including an end-to-end suite that drives the assembled server against a fake Squid and a fake Azure DevOps with a canary bearer, asserting the credential is injected on allowed reads and that denials never reach the upstream. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
`compiler-smoke-e2e/index.test.ts` failed intermittently in full-suite runs
while passing in isolation, and the reported error pointed at the wrong test:
(happy path) Test timed out in 5000ms
(unexpected path) expected +0 to be 1
Both symptoms have one cause. These tests call `await import("../index.js")`
inside the test body, so Vite's on-demand transform of that module's whole
dependency graph is charged to the test's 5s budget. That is infrastructure
work, not test work, and it is wildly variable: measured between ~8s and ~158s
of transform across the suite on the same machine depending on cache state and
load. The happy-path test needs ~1.6s when warm, so a cold or contended run
tips it over.
The second failure is a cascade. Vitest fails a timed-out test but does not
cancel the promise, so the abandoned `main()` kept running and consumed the
`mockResolvedValueOnce` that the *next* test had queued for
`worktreeChangedFiles`. That test then took the clean-path branch and
returned 0 instead of 1 — an assertion failure with no visible connection to
the timeout that caused it.
Raise `testTimeout`/`hookTimeout` to 30s, which is what the Vitest error
message itself recommends. A genuine hang still fails, just later.
Reproduced by clearing `node_modules/.vite` and running the full suite under
CPU contention; verified with five consecutive clean runs under the same
conditions, including one with 147s of transform time.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
…y catalog `AdoReadCapability` (front matter) was a hand-written copy of `ado_proxy::catalog::Capability` (authoritative) with no mapping between them and nothing asserting they agree. That is precisely the drift the generate- don't-duplicate contract removes between Rust and TypeScript, left open inside Rust: adding a capability to the catalog would ship a proxy enforcing a policy authors have no way to request, and renaming one would silently change the accepted YAML. Give the catalog a `Capability::ALL` plus `is_always_on`, make `AdoReadCapability::to_catalog` the single mapping point, and add a test that fails if the catalog gains a selectable capability front matter cannot express. Verified non-vacuous: adding a `wiki` capability to the catalog fails with "catalog capability wiki is not reachable from front matter". `discovery` stays deliberately unselectable. `az` and the REST SDKs call `resourceareas`/`connectiondata` before anything else, so a policy without it yields a proxy no supported client can use; offering it as a toggle would imply an author could turn it off and still have something that works. The test asserts that too, so it cannot be added by accident. Also close a widening footgun in the same schema: an `allow` entry naming an organization with no `projects` would have granted every project in that organization as the result of *omitting* a key. Reject it. An empty `repositories` list is still fine - it grants project-scoped reads (builds, pipelines, work items) without any repository-scoped read, so it narrows. The object form remains rejected at compile time, but the structural rules now run on the live path ahead of that rejection, so a scope mistake surfaces on the fixture that contains it rather than lying dormant until the proxy is wired. The rejection message now echoes the requested capabilities. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
…ery route Driving the real Azure CLI against the implemented engine invalidated the container-wide interception model the design assumed. Measured: with the OS trust store updated and nothing else, `az` still fails CERTIFICATE_VERIFY_FAILED, because Python's requests uses its own bundled certifi/cacert.pem. Node ignores the OS store too, and the remedies (REQUESTS_CA_BUNDLE, SSL_CERT_FILE) replace rather than extend the bundle, so they must carry the public roots or every non-ADO HTTPS request breaks. Each further runtime needs its own handling, so the mechanism never converges - and the end state plants a CA trusted for every host by every process in the agent for the whole run, to police two hostnames. Two findings give a better shape: - `az` honours an arbitrary base URL. Pointed at https://localhost:<port>/<org> it issued OPTIONS /<org>/_apis then GET /<org>/_apis/projects to that endpoint, verifying TLS from REQUESTS_CA_BUNDLE alone with no trust store touched. So it can be *told* where to go rather than deceived about a public hostname - the same trick AWF's existing cli-proxy uses for `gh` via GH_HOST. - The MCP cannot be told: src/index.ts hardcodes "https://dev.azure.com/" + orgName with no override, and 8 raw fetch() call sites ignore proxy env vars regardless (undici needs NODE_USE_ENV_PROXY, which needs Node >=24.5 against a pinned node:20-slim). A DNS alias redirects it at resolution time, which defeats both problems at once. So ingress is per client and trust is scoped to match - one process for `az`, one container for the MCP - while both terminate at the same catalog, keeping a single place where "what may be read" is decided. Enforcement comes from topology rather than client cooperation: Squid denies the protected hosts, so a client that ignores its configuration fails rather than escaping. Certificate trust therefore becomes an availability control, not a security one. Also adds the SPS discovery route the probe proved is required: `az repos show` calls OPTIONS https://app.vssps.visualstudio.com/_apis before its first data call and fails outright without it. It returns service topology only, so it does not widen data access. scripts/az-probe.mjs is the harness behind these findings - it runs the real az through the real bundle against a fake Squid and a fake Azure DevOps with a canary bearer. It should become a conformance test rather than staying a one-off. Open questions are recorded in the doc rather than resolved: whether the SPS call is avoidable with a faithful discovery document, whether Docker's embedded DNS reliably wins for a public FQDN alias, how the engine obtains egress given AWF's DOCKER-USER rules, and whether the MCP can start without npm registry access. No runtime behaviour changes; runtime_available stays false. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Wave 0 de-risking for the per-client ingress design. Both spikes ran against real Docker (engine 29.6.2, linux/arm64) and the real Azure CLI, so the design now rests on measurement rather than inference. --add-host redirection (scripts/add-host-probe.mjs) --------------------------------------------------- The ADO MCP path depends on redirecting a container that cannot be told where to go: @azure-devops/mcp hardcodes its base URL, and 8 of its call sites use raw fetch(), which ignores proxy env vars. A node:20-slim container given --add-host dev.azure.com:<ip> plus NODE_EXTRA_CA_CERTS reached the stand-in proxy over BOTH node:https AND global fetch, with rejectUnauthorized left on, and the server observed Host: dev.azure.com - so the client genuinely believed it was talking to Azure DevOps. A negative control (unrelated host) failed ENOTFOUND, confirming the redirect is narrow. This retires the Docker DNS-alias approach: --add-host needs no DNS at all, which matters because AWF itself falls back to /etc/hosts where embedded DNS is unreachable (gVisor, ARC/DinD). It also retires the Node 20 blocker, since no proxy env var is involved. SPS avoidance (scripts/sps-probe.mjs) -------------------------------------- An earlier probe saw az contact app.vssps.visualstudio.com despite a custom --organization, which would have meant reaching a deployment-level host outside the policy scope. Three scenarios show it is an artifact of the discovery document, not fixed behaviour: minimal doc -> az fails: location area not registered faithful doc + sparse areas -> az falls back to SPS faithful doc + complete areas -> az exit 0, never contacts SPS The third case is the first time az has completed end to end in any probe. This carries a concrete implementation consequence, recorded in the design doc and tracked as proxy-rewrite-areas: the engine must REWRITE every locationUrl in /_apis/resourceAreas to point at itself. The filter-resource-areas policy currently implemented drops entries failing a protected-host check, which would empty the list and send az straight back to the SPS fallback - the opposite of the intent. No runtime behaviour changes; runtime_available stays false. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
…g it The /_apis/resourceAreas response is what tells az where each Azure DevOps service lives, so it decides whether az stays on the policy endpoint. The filter-resource-areas policy dropped entries whose locationUrl was not already a protected host - which empties the list in the normal case and sends az straight to deployment-level SPS, the opposite of the intent. Rewrite instead: replace each URL's scheme and host with the origin the client is already talking to, preserve the path, and drop only entries that cannot be rewritten at all. The origin differs between the intercepted MCP path and the az broker path, so it is passed in rather than assumed. Evidence, through the real bundle rather than a unit test: the fake upstream now deliberately advertises vsrm.dev.azure.com, so a working rewrite is the only thing that can keep az on the policed origin. Both �z devops project list and �z repos show return exit 0 with correct JSON, every request matches a catalogued operation, SPS is never contacted, the sentinel PAT never reaches the upstream, and the injected bearer does. That is the first time stock az has completed end to end against the proxy with no real credential - one of the production gates in the design doc. 892 TS tests green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
The engine minted its own CA with openssl, which forced it onto the full node:20 image (node:20-slim has no openssl) and put private keys on a filesystem. Neither was necessary. The real constraint is only that the private key must never be agent-readable. The engine starts before AWF, so at generation time no agent exists at all - and passing the material on stdin means it touches no filesystem, so there is no window to get wrong and nothing to delete afterwards. Generation moves to a host pipeline step. That adds no dependency: every compiled pipeline already requires host openssl, since prepare_mcpg_config_step mints the MCPG API key with openssl rand on every run. A helper container would have been a second image to pull and mirror for air-gapped customers, for no gain. Because the protected host set is compiler-known, the leaves are generated alongside the CA and arrive in the same stream, so the engine never needs to issue a certificate - which suits Node, as it can parse X.509 but not issue it. The engine therefore needs no openssl and runs on node:20-slim, already the Azure DevOps MCP image, so nothing new enters the supply chain. ca.ts inverts from minting to parsing, keeping the CaMaterials shape so server.ts and the SNI callback are untouched. The stream is section-marked rather than bare concatenated PEM so each leaf stays bound to its hostname; relying on order would be a silent correctness trap if the generator changed. Fail-closed throughout, verified in a container against the real bundle: an empty stream exits 1 with "no certificate material on stdin"; a CA with no leaves exits 1 with "certificate stream carried no host leaves". A half-formed leaf is rejected rather than served, since TLS would otherwise fail at handshake time with nothing pointing at the material as the cause. publishCaCertificate now refuses anything containing a private key, because that path is mounted into the MCP container. Also proven: host openssl generates -> real bundle on node:20-slim -> engine listening. 905 TS tests green, 13 of them new. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
The engine only spoke proxy protocol: a CONNECT, or an absolute-form request. But neither production client uses a proxy. The Azure DevOps MCP is redirected by --add-host, and the az wrapper is pointed at the engine hostname; both open a TLS connection directly and would have handed raw handshake bytes to an HTTP parser. Add a second listener that terminates TLS straight off the socket, choosing the host by SNI rather than a CONNECT target. Everything after the handshake - normalization, catalog enforcement, credential injection, response filtering - is the identical code path, so there is one policy implementation with two ingresses rather than two implementations to keep in step. The CONNECT listener stays for proxy-configured clients; the design doc already marks the byte-tunnel as a compatibility affordance. Hardening: a TLS handshake that fails (unknown SNI, a client that does not trust the CA) arrives as a tlsClientError, and pre-handshake socket errors have no other handler. Both are now caught, since a client resetting mid-handshake must not take down a process serving every other client. New --tls-port option, defaulting to 443 because a redirected client uses the ordinary HTTPS port; configurable only so tests can bind unprivileged. Three e2e tests, all against the assembled server: an allowed read over direct TLS carries the injected bearer and returns 200; a denied one returns 403 without the upstream being contacted; a handshake for a host the catalog does not police is refused outright rather than served with some other leaf. 908 TS tests green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
…y constraint
Two findings from probing the MCP path, plus a gap they exposed.
MCP packaging: mount, do not pre-bake
--------------------------------------
@azure-devops/mcp@2.8.1 installed on the host and mounted read-only into an
unchanged node:20-slim completed an MCP initialize handshake under
--network none, returning full tool capabilities. So no pre-baked image is
needed, and nothing new enters the supply chain for air-gapped customers -
the package is handled exactly as ado-script.zip already is.
Two implementation details that are easy to get wrong:
- the mount must be at /app/node_modules, not an arbitrary path. Node
resolves dependencies by walking upward from the importing file, so
mounting elsewhere leaves the MCP own dependencies unresolvable
(ERR_MODULE_NOT_FOUND for @modelcontextprotocol/sdk).
- the startup tenant lookup in org-tenants.js targets vssps.dev.azure.com,
which is not in the protected set. It fails under isolation, the server
logs it and continues - so it neither blocks startup nor needs a catalog
entry.
Credential delivery is not yet wired, and the obvious path is unsafe
--------------------------------------------------------------------
Acquisition exists (generate_acquire_ado_token -> SC_READ_TOKEN), but nothing
produces the --token-file the engine reads.
The obvious choice - a file under the runner /tmp - would be a security bug,
for the same reason the CA private key was: AWF mounts /tmp into the agent at
both /tmp and /host/tmp (agent-service.ts), which is exactly how AWF installs
its own gh wrapper. A token written there is agent-readable and the boundary
is gone. Recorded with the two mechanisms that avoid a shared path, tracked as
proxy-token-delivery.
Separately, the compiler currently passes -e ADO_MCP_AUTH_TOKEN="\"
straight into the MCP container. Under interception the engine holds the
credential and injects it after an allow decision, so the MCP must receive a
non-secret sentinel instead; leaving the real token there would make the proxy
decorative on that path. Tracked as mcp-token-sentinel.
Docs only; no runtime behaviour changes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
… lifetime The engine read its bearer from --token-file, and nothing produced that file. The obvious implementation would have been a security bug: AWF mounts the runner /tmp into the agent at both /tmp and /host/tmp (agent-service.ts), which is exactly how AWF installs its own gh wrapper, so a token written to a runner path is readable by the very agent the credential is being hidden from. The token now travels in the same stdin stream as the interception certificates, under a ### TOKEN section. Same custody property as the CA private key: it touches no filesystem, and the engine starts before AWF so no agent exists while it is being delivered. --token-file and ADO_PROXY_TOKEN_FILE are removed rather than left as a trap. TokenSource becomes an in-memory holder that rejects an empty bearer at construction, so no request path can forward unauthenticated - Azure DevOps answers those with a sign-in page a client can mistake for data. The cost is that a stdin-delivered token cannot rotate, so a run must not outlive it. Rather than let that surface mid-run as opaque 502s - worse than today, since the agent cannot tell an expired credential from a policy denial - it is enforced at compile time: validate_proxied_timeout rejects timeout-minutes above 50 (ADO tokens are ~1h; the margin covers minting before the Agent job starts, plus clock skew). The error names both the limit and the reason. The bound applies only to workflows that opt into the proxy. An unproxied agent holds no Azure DevOps credential, so there is nothing to expire and no reason to constrain it. Rotation needs a different delivery mechanism - a private volume, or docker cp into the running container - and remains required by the WIF-renewal production gate in the design doc. This lands the security fix without pretending that is solved. Clippy clean, 19 Rust suites, 910 TS tests green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
The stdin material used an ad-hoc `### MARKER` format whose parser matched markers anywhere in a line rather than anchored to line starts, so a PEM or token value containing a marker could fabricate a section. Duplicate sections resolved silently to the last occurrence, truncation was only caught when it happened to break PEM shape, and there was no version to reject a future format against. Replace it with a versioned JSON document carrying base64 blobs. Truncation now fails at the JSON parse, values cannot influence framing, unknown schema versions are rejected outright, and every blob is validated for base64 round-trip and PEM shape before use. Each failure names what was wrong. Verified with a host-generated document piped into the real bundle on node:20-slim: the engine starts with no material on any filesystem, and truncated, wrong-schema, missing-token, corrupt-base64 and empty inputs each fail closed with a distinct message. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Adds the Agent-job steps that start and stop `ado-proxy`, plus the policy document the engine reads at startup. Nothing emits them yet — the Agent job gains them in the topology-attach change — so this is inert on compiled output. The engine needs no image of its own. It ships as an ado-script bundle that is already downloaded onto the runner, so it is mounted into the same stock Node image the ADO MCP uses; the supply chain is unchanged. Scope is substituted at step time from System.CollectionUri and System.TeamProject rather than baked in at compile time, because a compiled pipeline is routinely queued against a different project than it was compiled in, and a stale scope would silently widen access. The credential and the CA signing key are generated into the agent work directory, never /tmp: AWF mounts /tmp into the agent chroot, so anything written there is readable by the very agent this design withholds the credential from. The material is streamed to the container on stdin — not via -e, which would expose it to `docker inspect`, nor via argv, which would expose it in the process table — and every private key is shredded once handed over. Verified by running the emitted bash against a stubbed docker, then feeding its real output to a live container: the engine came up on both ingresses, published its interception CA, and policed traffic from a container redirected at it with --add-host — allowed discovery reached the Squid egress path, a route outside the granted capabilities got 403 unknown-route, and /_apis/distributedtask/variablegroups got 403 as an always-denied family. No private key survived the step. Two corrections fell out of that run and are recorded in the design doc: --public-ca-file is an output path, not a trust store, and node:20-slim ships no OS trust store at all (Node's own bundled roots verify the upstream). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Gives run_agent_step a second --topology-attach for the policy engine, so the
agent can reach it, and adds the engine to the NO_PROXY peer list — its name is
not public DNS, and routing it through Squid would break the very connection
that reaches the policy.
Still passed false at the only call site, so compiled output is byte-identical;
no lock file changes.
Verified against the pinned AWF v0.27.32 binary rather than the local clone,
which is stale at v0.23.1 and predates the flag entirely:
- --help documents --topology-attach as "Repeatable", with a two-peer example;
- config.topologyAttach is an array, and both connectTopologyContainers and
getTopologyContainerIps take the whole list;
- patchComposeWithTopologyHosts writes extra_hosts for every peer, which is
what will let the az wrapper resolve the engine by name. AWF does this
precisely because Docker's embedded DNS is unreliable under gVisor and
ARC/DinD.
The first attempt emitted ragged 11- and 13-space indents because the preceding
--network-isolation continuation already supplies the indent for the first
line. Both variants are now shellcheck-clean, and a test asserts that enabling
the engine changes nothing in the invocation but the attachment and NO_PROXY
lines.
Runner verification of two live peers remains outstanding.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
The MCP container was handed the real Azure DevOps bearer via
-e ADO_MCP_AUTH_TOKEN="$SC_READ_TOKEN" and put on --network host, so it
reached Azure DevOps directly with a live credential the agent could steer
through tool calls. This removes that path.
The MCP now runs on a dedicated network shared only with the policy engine,
with dev.azure.com redirected at the engine via --add-host, trusting the
published interception CA, and holding a non-secret sentinel in place of a
credential. AWF's DOCKER-USER rules are scoped to its own bridge, so they do
not filter this network — the MCP can reach the engine and nothing else.
It is also launched directly rather than through npx: the package is installed
on the runner and mounted read-only at /app/node_modules, so the container
needs no registry access. The mount point is load-bearing, since Node resolves
dependencies by walking upward from the importing file.
The pinned version is now enforced rather than merely requested: it is
surfaced in the version catalog alongside AWF and MCPG, installed with
--save-exact, and the resolved tree is checked against the pin. npm resolves
ranges transitively, so a matching request does not by itself guarantee a
matching tree — and the agent's tool surface is whatever ends up on disk.
Capabilities default to the whole catalog. That is broad within a narrow
boundary: every catalogued operation is a GET or OPTIONS, and secret-bearing
route families (ACLs, tokens, service endpoints, variable groups, secure
files) are denied outright. Starting narrower would leave the MCP unable to
answer most questions, which pushes authors back towards handing agents raw
credentials.
Proven end to end before the compiler change was written, using the real
@azure-devops/mcp 2.8.1 against a live engine:
- core_list_projects returned the engine's own error text, so the call
reached the policy engine with TLS trust intact and only a sentinel;
- on the redirected host, writes are refused 403 "POST is not a read
method", and denied families are refused 403 by name;
- the MCP's startup tenant lookup targets a non-protected host and remains
non-fatal under redirection.
Six tests asserted the old behaviour, including the credential mapping itself.
They are inverted into regression guards rather than deleted, so the hole
cannot silently reopen. Compiled output confirms SC_READ_TOKEN now appears
only in its acquisition step.
Runner verification is still outstanding, as is the case where the engine
address is unresolved — that now fails the build loudly rather than letting a
client resolve the real Azure DevOps.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
…ngine
The MCP shared a normal user-defined bridge with the policy engine. Those have
outbound NAT, so the MCP kept a direct route to the internet — including every
Azure DevOps host the --add-host redirect does not override — and the engine
policed one hostname rather than a boundary. Create the network --internal
instead.
Measured rather than reasoned about: a container on a normal bridge reached
example.com with status 200; on an --internal bridge the same request failed
while the container still routed to its peers; and a dual-homed container kept
full egress via its second network. That last point is what makes the fix safe
— the engine still reaches Squid over awf-net, which AWF attaches it to.
This corrects a claim in the design doc that AWF's DOCKER-USER scoping alone
left the MCP with no unpoliced route out.
Also records the Wave 2 acceptance evidence, gathered against a live chain of
the real MCP, the real engine, a fake Squid and a fake upstream:
- a client reached the upstream and got 200 with real JSON;
- every request arriving upstream carried the INJECTED canary bearer, and
the sentinel the client held never appeared there;
- variablegroups, serviceendpoint, a POST write and an unknown route were
each refused 403 with distinct reasons, and the upstream request count was
unchanged across all four;
- scanning the MCP container's environment, mounts, /tmp and process table
for the canary found zero occurrences;
- npm view failed EAI_AGAIN inside the MCP container, yet the MCP still
completed an initialize handshake from the mounted package.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Adds the wrapper the agent gets in place of stock az. It sets three environment variables and execs the real binary; it does not rewrite arguments. Not wired into the sandbox yet — that is the next change. Earlier drafts rewrote --organization to a broker hostname. Dropping that is a simplification, not a shortcut. The organization can also arrive via --org, AZURE_DEVOPS_ORG, or a stored �z devops configure --defaults value, so rewriting means enumerating every form and staying correct as the CLI evolves, and a missed form silently escapes the policy. A non-canonical hostname is also unusable: interception leaves and catalogued routes are both keyed to dev.azure.com, so it would fail SNI selection and match no operation. Pointing HTTPS_PROXY at the engine puts the redirect below the CLI's own configuration, so every form works without the wrapper interpreting any of them. Verified with real az 2.86 against a live engine: �z devops project list --organization https://dev.azure.com/contoso completed the CONNECT, verified the intercepted certificate from REQUESTS_CA_BUNDLE alone, and the request arriving upstream carried the INJECTED bearer while the sentinel the CLI held never appeared there. The generated wrapper was then run on Linux: allowed groups pass through with arguments untouched, az storage is refused with an actionable message, --version still works, and the PATH scan skips the wrapper's own directory so it cannot re-enter itself. That first az run also exposed a real defect in the certificate the engine publishes. The CA declared basicConstraints pathlen:0 without keyUsage keyCertSign, which Node accepts and OpenSSL 3 rejects outright ("Path length given without key usage keyCertSign"). Every client exercised so far had been Node, so nothing caught it; az failed CERTIFICATE_VERIFY_FAILED. The CA now declares keyUsage=critical,keyCertSign,cRLSign, with a regression test, after which az completed TLS and reached policy. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Wires the wrapper in: an agent-prepare step writes it to /tmp/ado-aw-lib/az and the Azure CLI extension prepends that directory to the sandbox PATH, so the agent's �z resolves to the wrapper and the wrapper execs the real binary. No mount is needed. AWF already bind-mounts the runner's /tmp into the agent chroot — the same mechanism that delivers the agent prompt and the Copilot binary — so writing the file makes it visible at the same path inside. The PATH prepend is still required, because the chroot's /usr/local/bin is not the container's and only PATH order decides which binary the agent invokes; this mirrors how AWF installs its own gh wrapper. The engine now publishes its interception certificate directly into that directory rather than a separate one, so the wrapper reads and the MCP mounts the same file and no client can trust a stale copy. Only the certificate is published; the private key is still destroyed by the step that starts the engine. Installation is gated on the existing AW_AZ_MOUNTS detection signal: with no az on the runner there is nothing for the wrapper to exec, and shadowing a missing binary would turn a clear "command not found" into a confusing wrapper error. A shared ado_proxy_enabled() predicate now backs both the pipeline builder and this extension. Two independent checks could drift into installing a wrapper that points at an engine which was never started, or starting an engine that nothing routes through. Verified against real compiled output rather than the generator: the install step was extracted from the emitted lock file and run on Linux, producing a mode-755 file whose shebang sits at column 0. Invoking it, an allowed group passed through with arguments untouched and all three environment variables set, and �z vm was refused with the actionable message. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
… rest The Azure CLI advisory still said �z devops was not pre-authenticated. Under the policy engine that is wrong in the opposite direction, and a wrong prompt is not cosmetic: an agent told a command is unavailable will not try it, while one told it has access it lacks will retry a failing call or invent a workaround. The proxied text now states what genuinely works — read-only, current organization and project — and, just as importantly, that refusals are deliberate rather than a misconfiguration, so retrying or authenticating will not help. The unproxied text is unchanged and still claims nothing beyond "not pre-authenticated". Two defects surfaced while writing it. The wrapper permitted �z artifacts, but no catalogued operation backs it, so the call passed the wrapper and was refused by the engine. The allow-list is now derived from the capabilities the policy actually grants, via a new Capability::az_command_group(), so the two cannot drift again; narrowing the policy narrows the wrapper with it. �z rest was refused, which added no security and contradicted �z devops invoke being allowed — both express arbitrary Azure DevOps REST. Measured against a live engine, �z rest is fully contained by the catalog: a catalogued read returned real data with no login and no PAT, because the engine injects the credential; a denied route family returned 403 denied-route-family; and a POST returned 403 method-not-read. It is now permitted regardless of capabilities, since the catalog is what contains it. ado_proxy_capabilities moved to common.rs so the pipeline builder and the Azure CLI extension resolve capabilities from one place. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
… reads
PolicyDocument::new took a capability slice and never saw the front matter, so
every field it could not derive was invented: project_id, repository and
repository_id were hard-coded None, and skip_serializing_if dropped them from
the JSON entirely. The bundle's sameIdentifier() returns false for undefined,
so isCurrentRepository was ALWAYS false and all twelve catalogued
current-repository-path operations denied unconditionally - the repos
capability was dead. isCurrentProject fell back to the name alone, so the
current project addressed by GUID was also denied, which matters because az
substitutes whichever form it cached.
None of this surfaced because absent reads as "match nothing": every gap was a
silent denial rather than an error, and the live tests so far exercised
discovery, the project-validation probe and always-denied families - none of
which take the repository path.
The constructor now takes FrontMatter, so adding a policy field forces a
decision about which configuration populates it. Capability resolution moves
next to it, and common.rs re-exports it, so the emitted policy and the az
wrapper's allow-list cannot disagree.
Also folds in the scope-identifier work:
- project_id, repository and repository_id are substituted at step time from
System.TeamProjectId, Build.Repository.Name and Build.Repository.ID, so a
compiled pipeline stays portable;
- System.TeamProjectId is added to ALLOWED_ADO_MACROS with its rationale;
- the step now fails if any placeholder survives substitution,
since a literal placeholder would be read as an organization name matching
nothing - a total denial that reads as a policy decision.
The two organization derivations are collapsed into one shared helper, and
both were wrong for a form the other handled. engine.rs stripped a literal
https://dev.azure.com/ prefix, a no-op for https://myorg.visualstudio.com/ that
yields the whole URL; the proxy step took the last path segment, which returns
myorg.visualstudio.com for that same URL. Measured against both shapes: the
helper now returns "contoso" for https://dev.azure.com/contoso/ and for
https://contoso.visualstudio.com/.
Verified by extracting the emitted step from a compiled lock file and running
it on both collection forms.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Replaces the single pinned organization/project with a ScopeIndex built once at startup: the current scope seeded first, then any additional_scopes the policy carries. Five call sites previously asked "is this the pinned project" in their own way; folding them into one lookup means they cannot drift. Resolution is organization-relative by construction. Asking "is this project in any allowed list" would let a request addressed to organization B name a project granted only in organization A, so every lookup resolves the organization first and tests the project within THAT organization's entry. The same applies to repositories within a project. A project entry carries project_scoped so a repository grant does not imply a project grant: a scope derived from a repos: declaration grants the repository without the work items, pipelines and builds beside it. That mirrors the rule the front matter already has, where a project entry with no repositories: grants project-scoped reads without any repository-scoped read. The policy schema gains additional_scopes with the same fail-closed treatment as the rest of the document - unknown keys at either nesting level are fatal, and an entry naming no projects is refused outright, since in the front matter that would be a request to grant an entire organization. Response filtering now takes the organization the request was addressed to. Response bodies name a project but never an organization, so without it the project check could not stay organization-relative and a project granted in one organization would validate a response from another. Nothing emits additional_scopes yet, so compiled output is unchanged; the Rust emitter follows in scope-model-rust. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Adds the load-bearing negatives for the scope index: a project granted in organization A must not match the same project name in organization B, and a repos-derived scope must allow its repository without opening project-scoped reads beside it. Also guards the fail-closed policy schema at both nesting levels: unknown organization/project keys and an organization naming no projects are rejected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Lowers permissions.read.allow into the bundle's organization-relative additional_scopes tree. Each explicitly named project carries project_scoped=true, while repository-only grants derived from repos: will use false in the follow-up change. Adds optional project-id to the front-matter project scope, typed as a validated GUID. Azure DevOps clients may address an additional project by a cached GUID; without an author-supplied id, name-form requests work and GUID-form requests fail closed. The compiler now emits additional_scopes explicitly even when empty, so the current compiler never leaves that part of the policy undecided. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Removes the blanket rejection of the permissions.read object form now that capabilities and allow scopes are consumed by the compiler-owned policy document and enforced by the bundle. Structural validation stays on the compile path: an allow entry naming an organization with no projects still fails closed, because omission would otherwise request an entire organization. The Azure DevOps MCP fixture now uses the object form and asserts that its cross-organization scope survives into compiled policy JSON. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Fails compilation when tools.azure-devops is enabled without permissions.read. Previously no SC_READ_TOKEN was minted, so the engine reached startup with an empty bearer and failed on a base64 error - fail-closed, but too late and with no actionable explanation. The error names the missing front-matter key and explains that the token is delivered only to the trusted proxy, not to the agent or MCP. Explicitly disabling the tool remains valid and no longer counts as proxy enablement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Treats type: git repository resources as implicit read grants for the named Azure Repos repository. The build identity already resolves the resource and, when checked out, the working tree is already in the sandbox; denying its PR, branch and commit metadata would be incoherent. The grant is repository-only: project_scoped=false prevents the repository declaration from opening the work items, builds and pipelines beside it. Non-ADO repository types and bare current-project names grant nothing, while checkout: false still counts because the ADO resource remains explicitly declared and resolved. Also corrects the front-matter docs: Azure Repos type: git names are project/repo, not organization/repo. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Makes the integration fixture exercise a narrowed capability set, an explicit cross-organization project grant and an implicit repository-only grant. Compilation asserts all three surfaces agree: policy JSON, the az wrapper and the agent prompt expose only discovery/core/repos. Bundle authorization tests prove the load-bearing boundaries at route level: an explicitly granted cross-organization project works by name and GUID; the same project name in another organization is denied out-of-scope; a repos-derived repository is readable while project and build reads beside it remain denied; and projects outside every scope are denied. The built ado-proxy bundle was also started in Docker with the exact policy extracted from compiled YAML. It reported capabilities=discovery,core,repos, published its CA, and accepted both the explicit fabrikam/Shared scope and the implicit contoso/LocalProject repository-only scope at startup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Documents the live permissions.read grammar: scalar shorthand, capability narrowing, additive organization-relative allow scopes, optional project GUIDs and implicit repository-only grants from type: git repos entries. Clarifies the two hard limits: cross-organization access uses one service-connection identity within one AAD tenant, while cross-tenant reads need another credential and are unsupported; and a missing project-id permits name-form calls while cached GUID-form calls fail closed. Replaces stale pre-proxy descriptions of the MCP token mapping, npx startup, az authentication, rotating token files and AWF-managed sidecars with the implemented topology: stdin-only bearer custody in ado-proxy, sentinel clients, internal MCP network, CONNECT-based az wrapper, one-shot token with a 50-minute compile-time bound, and a host-started node:20-slim container attached by AWF. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Flips the author-facing catalog availability bit after the complete path is wired and proven: compiler-emitted policy, stdin-only credential custody, host-started proxy container, repeatable AWF topology attachment, internal MCP network, sentinel MCP and az clients, capability narrowing, current-scope identifiers, explicit cross-organization scopes and implicit repository-only grants. Regenerates the committed TypeScript catalog snapshot and converts the old "must remain disabled" tests into regression guards for availability. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Updates the top-level README and contributor instructions to reflect the shipped credential path: only ado-proxy holds SC_READ_TOKEN, while the Agent, MCPG, Azure DevOps MCP and wrapped az receive no real credential. Records the AWF chroot trap in AGENTS.md: runner /tmp is mounted into the agent at both /tmp and /host/tmp, so host steps must never stage credentials or private keys there and assume deletion makes the exchange safe. Private material must use stdin or a container-private volume; only intentionally public files may be published under /tmp. Also refreshes architecture entries and removes the last stale trusted-MCP and rotating-token-file wording. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
jamesadevine
commented
Aug 5, 2026
Collaborator
Author
|
/review |
Contributor
|
🚀 review-typescript has started processing this pull request comment |
Collaborator
Author
|
@copilot merge main |
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Contributor
Merged |
Collaborator
Author
|
/review |
Contributor
|
✅ Rust Code Quality Reviewer completed the Rust code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
TypeScript review — scripts/ado-script/ (ado-proxy sidecar)
Reviewed the new ado-proxy TypeScript sidecar (server.ts, route.ts, policy.ts, token.ts, headers.ts, config.ts, ca.ts, log.ts, index.ts, upstream.ts, scope.ts, api-version.ts, response.ts) plus the compiler-smoke-e2e assertion additions.
No high-confidence defects found on the changed lines:
- Async/error handling: every network path (
tunnel,handleProtected,forwardPlainHttp, the TLSsecureConnect/response promises) has expliciterror/catchhandling with fail-closed behavior; late socket errors are converted todestroy()calls to avoid unhandled'error'events crashing the sidecar.void handleProtected(...).catch(...)correctly guards the one place an async handler is invoked from a sync callback. - Timeouts:
UPSTREAM_TIMEOUT_MSbounds every upstream request;readBoundedcaps response size and destroys the stream on overflow. - Type safety on external input:
config.ts/ca.ts/response.tsall parse untrusted JSON throughasRecord/requireString/requireStringArrayhelpers with explicit key allowlisting before any cast — no unguardedanyreaches policy logic.route.ts's placeholder shape regexes are a good defense against value smuggling. - Credential handling: the bearer is read from stdin only, held behind a single-accessor
TokenSourceclass, applied to headers only after the allow decision (handleProtected's ordering is enforced by a documented invariant), and the decision log (log.ts) is explicitly restricted to shapes/outcomes, never bodies/headers/URLs. - Tests: new branches (route normalization, denied families, header stripping, config parsing) all have corresponding
*.test.tscoverage.
Nothing rises to a postable inline comment. Nice defensive design overall (deny-by-default parsing, allowlisted headers, structural family matching instead of substring matching).
🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 94.3 AIC · ⌖ 2.94 AIC · ⊞ 11K
Comment /review to run again
Contributor
There was a problem hiding this comment.
Security review — no regressions found
This PR adds a security boundary (deny-by-default ado-proxy) rather than weakening one, so most of my usual checklist doesn't apply — there is no prior "PR replaces a check" comparison to make.
Reviewed the trust-critical paths for regressions or gaps introduced by this diff:
- Credential custody (
token.ts,start_ado_proxy_step): bearer arrives via a container-local FIFO overdocker exec -i, never argv/env/file; private keys areshred-ed after handover;--rm-independent teardown records state beforedocker rm -fso a crash can't leave the credential running unnoticed. No path lets the raw bearer reach the agent, MCP container, oraz(verifiedaz_wrapper.rsonly ever emits a sentinelAZURE_DEVOPS_EXT_PAT, and there's an explicit test assertingSC_READ_TOKEN/System.AccessTokennever appear in the wrapper script). - Header/credential stripping (
headers.ts): allowlist-based both directions;authorization/cookie/www-authenticateetc. stripped from the client and never forwarded from upstream back to the agent. - Authorization ordering (
server.ts::handleProtected): normalize → authorize → then read token, so a denial never touches the credential — matches the doc comment's stated contract. - Scope enforcement (
policy.ts,scope.ts,response.ts): organization-relative project/repo matching (a project name grant in one org correctly does not leak into another, perscope.test.ts); response-body validation for the two operations whose URL alone can't be scope-checked; unhandled response-policy variants fail closed (exhaustive: never) rather than defaulting to forward. - Route normalization (
route.ts): rejects double-encoding, embedded separators, control chars, and../.traversal before any matching happens, so downstream capability/scope checks see the same bytes upstream receives. - Fail-closed policy parsing (
config.ts/policy.rscontract): unknown JSON keys, mismatchedcatalog_version, and unsubstituted${ADO_PROXY_*}placeholders are all treated as startup/compile failures rather than silently under-enforcing. azwrapper (az_wrapper.rs): interpolated values (engine_host,connect_port,sentinel, capability groups) are all compiler-owned constants/enum-derived strings, not user-controlled front-matter text, so no injection surface there.
Nothing in the diff reaches a Stage 1 (Agent) step with a write-capable token, and I didn't find a path where sanitized decision-log output (log.ts) could leak request content — it's schema-constrained to shapes/outcomes only.
No blocking findings. Nice defense-in-depth design (deny-by-default catalog, allowlisted headers, response-body scope validation, fail-closed parsing).
🔒 Security review by PR Security Reviewer · auto · 118.1 AIC · ⌖ 2.19 AIC · ⊞ 11.3K
Comment /review to run again
Contributor
There was a problem hiding this comment.
Compiler contract review — ado-proxy
Checked the codegen/drift, bundle registry, extension registration, and secure-type contracts this PR touches. All of them are correctly wired:
ado_bundle.rsregisters the newBundle::AdoProxyinALL, the path table, and theBundleAuthmatch with an explicit rationale comment for the stdin-only credential path.extensions/mod.rscorrectly gatesAzureCliExtensionbehindado_proxy_enabled(), and the accompanying test updates (extensions/tests.rs) match the new counts and behavior.- Codegen drift is guarded:
npm run codegennow also runsexport-ado-proxy-catalog-schema/export-ado-proxy-catalog,catalog.gen.jsonhas a dedicated drift test (catalog-drift.test.ts), andCATALOG_SCHEMA_VERSIONis checked at sidecar startup — matches the AGENTS.md description exactly. - New identifier types (
AdoOrganization,AdoRepository,Guid) were added tosrc/secure.rsand are used by the front-matter grammar (AdoReadOrganizationScope,AdoReadProjectScopeintypes.rs) rather than rawString.PolicyDocument'sStringfields are post-validation serialization output, not raw untyped input, so this is not a gap. docs/cli.mddocuments the new--kind ado-proxycatalog option and adds a 'Hidden Pipeline-Internal Commands' section for the proxy-only exporters.- No
.github/workflows/*.mdchanged without its lock file, and no newtests/safe-outputs/*.lock.ymlwas committed.
No compiler-contract findings to raise on this pass — the drift-prone surfaces (bundle registry, extension registration, codegen, front-matter grammar, secure newtypes, docs) are all internally consistent in this diff. Deferring to the Rust/TypeScript quality reviewers for anything outside contract scope.
🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 50.5 AIC · ⌖ 13.6 AIC · ⊞ 11.9K
Comment /review to run again
Contributor
There was a problem hiding this comment.
Rust engineering review
No merge-blocking defects found in the Rust portion of this diff (44 files under src/, ado-aw-derive/, tests/, scoped to 34 relevant .rs/Cargo.toml files after excluding generated/lock/bundle content).
💡 What I checked
- Error handling:
anyhow::Resultused consistently;.context()/.with_context()present on IO/parse paths (e.g.analyze_ado_proxy_logs,PolicyDocument).unwrap()/expect()usage is confined to tests and oneserde_json::to_string_pretty(...).expect(...)on a statically-constructed struct (catalog.rs), which is an acceptable invariant. - Determinism:
src/audit/analyzers/ado_proxy.rsusesBTreeMapfor operation/reason/status-class accumulation and explicitsort_bybefore emitting, so generated reports and (by extension) any downstream YAML/JSON stay stable across runs. - Security-sensitive surface (
ado_bundle.rs,ado_proxy/policy.rs,compile/az_wrapper.rs,secure.rs): theAdoProxybundle is correctlyBundleAuth::Nonewith the bearer routed via stdin material rather than env/args; newAdoOrganization/AdoRepository/Guidvalidated newtypes insecure.rsfollow the existingvalidated_string!pattern with reasonable character/length rules; theazwrapper substitutes a non-secret sentinel and is well covered by targeted tests (routing, CA trust, config dir, non-rewriting of org forms, capability-gated command groups). - Extension gating:
AzureCliExtensionis now correctly conditioned onado_proxy_enabled(front_matter)incollect_extensions, closing the previous always-on hostazmount — this is the core security fix and it looks correctly wired end-to-end (detection → wrapper install → policy prompt → PATH prepend).
I ran the rust-critic sub-agent against the same diff in parallel; it returned no output after two attempts, so I'm not including anything from it and relied entirely on my own pass.
Given the size of this PR (95 files, +14.6k/-635), I focused on the newly-introduced credential-isolation logic rather than re-litigating the existing human comments already on the thread (bash-embedding style, common.rs indent-passing, struct reuse suggestions) — those are valid maintainability points already raised by @jamesadevine and I'd defer to those threads rather than duplicate them.
🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 92.6 AIC · ⌖ 3 AIC · ⊞ 11.1K
Comment /review to run again
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa672954-b2fc-4c5d-82e5-5b066b8e0af8
This was referenced Aug 6, 2026
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
Add credential-isolated, deny-by-default Azure DevOps reads for agents without exposing the real ADO bearer to Copilot, the Azure DevOps MCP, or Azure CLI.
ado-proxyoperation catalog, compiler policy, TypeScript sidecar, response filtering, sanitized decision logs, and internal-network topologyazwrapper through the proxy;permissions.readactivates the proxy and wrappedaz, whiletools.azure-devopscontrols only the MCP clientaudit, JSON reports,trace, and MCP-author tools, including support for real nested PipelineArtifact download layoutsTest plan
cargo clippy --quiet --all-targetscargo test --quietcargo test --test bash_lint_testsaz/MCP reads, expected policy denials, Stage 3 proof tag, and live audit/trace verificationado-aw-proxy-630164; audit reported healthy lifecycle, 10 allowed requests, five denied requests across the four expected reason classes, and zero proxy errorsKnown follow-ups
multi-repochild attempted Cargo against public crates.io instead of the internal Azure Artifacts mirror; tracked in fix(smoke): route multi-repo Cargo through the internal feed #1823. The exact-tipado-proxychild was green.ghCLI: chore(security): minimize AWF agent environment and mask unused gh CLI #1818.