Repository navigation
feat(compile)!: credential-isolated package feed access via permissions.packages - #2315
Conversation
|
/review |
|
Azure Pipelines: 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
✅ TypeScript Code Quality Reviewer completed the TypeScript code quality review.
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Security review: no regressions found
This diff adds the package-feed credential-isolation proxy (permissions.packages). I reviewed the full diff against the threat model (token custody, injection into generated YAML/shell, path traversal, redirect SSRF, network allowlisting, safe-output integrity) and found no weakened security posture.
Notable positives that satisfied my checks rather than raised concerns:
- Credential isolation: the REST bearer and the new Azure Artifacts package credential are kept in separate material-document fields (
token/package_token), only ever read from env/stdin into the trustedado-proxyprocess, never written to disk, argv, or the Agent/Detection environment (ca.ts,agentic_pipeline.rs). - URL-shape authorization (
package.ts) rejects path traversal, encoded/double-encoded separators, backslashes, control characters, and mismatched scope/view/protocol grants, each pinned by an explicit deny-reason test — including a dedicated test asserting denial details never echo request content. - Redirect handling (
authorizePackageRedirect) never follows a redirect while holding the credential; it only relays to an allowlisted, suffix-anchored blob-storage host (.blob.core.windows.net,.vsblob.visualstudio.com) or a same-host route that is itself re-authorized, with explicit tests against look-alike hosts, userinfo, non-default ports, and non-https schemes. - Fail-closed wiring:
isInterceptedHostextends the HTTPS-only/port-443-only enforcement to package hosts, so a package host cannot be reached in cleartext or on a non-standard port to bypass policy;credentialProblemrefuses to start if a configured family lacks its credential or a package host lacks its TLS leaf. - New feed/view/project identifiers are validated newtypes (
secure.rs::AdoFeedName) rather than raw strings, consistent with the repo convention, so they cannot carry path/shell metacharacters into generated policy or the agent-facing prompt text. - Header allow-lists (
headers.ts) explicitly excludeauthorization,x-nuget-apikey,npm-otp, and other credential-bearing headers from both directions.
No merge-blocking findings. Nothing in pr-review-comments.json to avoid duplicating (list was empty).
🔒 Security review by PR Security Reviewer · auto · 89.1 AIC · ⌖ 2.64 AIC · ⊞ 11.4K
Comment /review to run again
There was a problem hiding this comment.
Exceptionally thorough test suite for a security-sensitive feature (credential isolation, URL-shape authorization, redirect validation, header sanitization) — the adversarial-encoding and canary-leak tests in package.test.ts and proxy.e2e.test.ts are a model for this kind of work.
Two gaps worth a look, both advisory:
validate_feed_exclusivity(new insrc/runtimes/mod.rs, wired into all three runtime extensions) has no direct unit test for its own error paths — only the pre-migration codemod shape is tested, not the compiler-level rejection ofruntimes.<x>.feedcombined withfeed-url/config.- The "cuts off a streamed body" e2e test swallows the promise's outcome with
.catch(() => undefined)and only asserts when it happens to resolve, with a bound loose enough to pass even if truncation silently regressed.
Nothing here blocks merge; the rest of the diff's test coverage (codemod round-tripping, policy cross-validation, per-protocol route tables, header allow/deny lists, audit-log scrubbing) is comprehensive and behavior-focused rather than implementation-mirroring.
🧪 Test quality analysis by Test Quality Sentinel · auto · 191 AIC · ⌖ 1.77 AIC · ⊞ 11K
Comment /review to run again
There was a problem hiding this comment.
Rust Code Quality Review 🦀
Verdict: no merge-blocking issues found. This is a large (26-file) but carefully engineered addition of permissions.packages credential-isolated Azure Artifacts access. cargo check passes cleanly on the branch.
💡 Review notes
- Error handling: consistent
anyhow::bail!/.context()usage; nounwrap()/expect()outside#[cfg(test)]modules. - Correctness: percent-decoding in the codemod URL parser is bounds-checked (
get(..)returnsOption, not panicking slice indexing); feed-handle collision resolution (upsert_grant) is deterministic; Cargo's/Cargo/index/URL casing is intentionally matched case-insensitively by the TS-sidematchProtocol, not a bug. - Determinism: protocol lists are sorted/deduped before serialization (
policy.rs), and audit stats useBTreeMap— consistent with the project's "generated YAML must be deterministic" rule. - Security-adjacent properties (credential custody, fail-closed wrapper routing) read as sound from the Rust side; the TS enforcement (
package.ts,resolve-feeds.ts) is out of this reviewer's remit and is covered by the TypeScript and compiler-contract reviewers. - Test coverage is thorough: codemod migration/idempotency, cross-field validation (
upstream: deny+ Collaborator role rejection), runtimefeedcross-referencing, and full pipeline-wiring assertions for both build-identity and WIF credential modes.
Process note: the rust-critic background sub-agent failed to start (400 model "gpt-5.4-mini" is not accessible), an infrastructure issue unrelated to this diff. This review reflects only my own direct pass over the Rust-only diff slice.
🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 190.7 AIC · ⌖ 1.94 AIC · ⊞ 11.3K
Comment /review to run again
There was a problem hiding this comment.
TypeScript Code Quality Review — scripts/ado-script/src/ado-proxy/
Note: the ts-critic sub-agent failed to start (model-access error: 400 model "gpt-5.4-mini" is not accessible via the /chat/completions endpoint), so this review reflects only my own direct pass, not a cross-check.
Scope reviewed
All new/changed TypeScript under scripts/ado-script/src/ado-proxy/ for the package-feed proxy feature: package.ts/.test.ts, config.ts/.test.ts, headers.ts/.test.ts, server.ts (handlePackage, openUpstream, streamBounded), resolve-feeds.ts/.test.ts, ca.ts, catalog.ts, index.ts.
Findings
No merge-blocking defects found.
- Async/error handling:
openUpstream/handlePackage/streamBoundedcorrectly attacherrorhandlers on every socket/stream before awaiting, back-pressure is honoured instreamBounded, and the single sharedopenUpstreamhelper avoids duplicated upstream-dialing logic between the REST and package paths. Thefinally { secured?.destroy() }cleanup is present on both handlers. - Credential isolation:
token/packageTokensare now optional and read viareadCredential(), each family's upstream only ever receives its own credential, andca.ts'scredentialProblem()fails closed at startup if a configured family lacks its credential or TLS leaf. Good design. - Type safety: the few
as Segment/as stringcasts inpackage.ts::matchFeedRouteare all preceded by explicit length/undefined guards, so they're safe despite being non-null-assertion-adjacent. Thecaught as Errorpattern inresolve-feeds.tsis pre-existing repo-wide idiom, not newly introduced risk. - Secret handling:
resolve-feeds.tsredacts the token from every log line viaredact()beforeescapeLoggingCommand(), andca.tsfails closed on an empty-but-present credential field. I did spot what looked like a bareAuthorizationheader / bearer-string leak inpackage.ts::packageAuthorizationHeaderandresolve-feeds.ts'sget()headers when viewing the files through tooling, but confirmed againstgit show HEAD:...that this is a sandbox/tool redaction artifact (the real source reads`Bearer ${token}`), not an actual code defect — flagging here only for transparency since it looked alarming at first glance. - Tests: every new branch (redirect allow/deny, 401/203 handling, response-too-large before and during streaming, packages-only policy denying REST, missing-credential paths, cleartext/CONNECT refusal for package hosts) has a matching
proxy.e2e.test.ts/resolve-feeds.test.ts/package.test.tscase.
No inline comments posted — nothing met the bar for an actionable, line-specific defect.
🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 172.1 AIC · ⌖ 6.61 AIC · ⊞ 11.3K
Comment /review to run again
…ctly Review follow-ups on #2315: - validate_feed_exclusivity had no direct test. Add unit tests for both conflicting combinations plus a per-runtime check through parse_markdown and each runtime extension. - The "cuts off a streamed body" e2e test swallowed rejections and only asserted when the request resolved. It now accepts exactly two outcomes, a partial body bounded by the limit or a connection reset, and requires a new response-too-large decision record either way. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b0ab0d4-23f4-4a89-90ea-3c40758ea550
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
/review |
|
✅ TypeScript Code Quality Reviewer completed the TypeScript code quality review.
|
There was a problem hiding this comment.
Verdict: COMMENT. I found no merge-blocking Rust defects in the changed lines.
I read the Rust changes in the codemod, the package-feeds extension, runtimes/mod.rs and the compile/types changes. I did not start the rust-critic sub-agent. I also did not read every changed Rust file in full, so this is not an exhaustive review.
Notes
- Error paths in the 0009 codemod carry the offending URL or key in their messages.
- The remaining
expect()calls in the codemod are guarded by earlier reads of the same keys, and the otherunwrap()calls sit in tests. - Percent-decoding uses checked slicing, so malformed input returns
Noneinstead of panicking. - Advisory: when
upsert_grantreuses an existing grant for the same feed, it does not apply theupstream: allowsetting that a newly created grant gets. A runtime that previously relied on upstream saves would silently narrow. Consider documenting this or warning about it.
🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 35.5 AIC · ⊞ 11.3K
Comment /review to run again
There was a problem hiding this comment.
Security review: no end-to-end exploitable regressions found. I reviewed the core of ado-proxy/package.ts and did not read every one of the ~7.4k added lines.
- The policy is deny-by-default: it fixes the package host, allows only GET/HEAD, and requires an exact feed, view and protocol grant.
- Target parsing rejects double encoding, encoded slashes outside npm scoped names,
./..segments, backslashes, control characters and fragments. - Redirects are never followed. They are relayed only to an https blob-storage suffix with an anchored label match, or to the same host after the route is re-authorized. Userinfo and non-default ports are refused.
- The credential is attached only after the policy check.
Low-confidence hardening note: the redirect allowlist accepts any *.blob.core.windows.net account, not just the Azure Artifacts storage account. Only the upstream controls the Location, so I did not trace an exploit. Consider pinning to the account actually returned by the feed if feasible.
🔒 Security review by PR Security Reviewer · auto · 20.8 AIC · ⊞ 11.5K
Comment /review to run again
There was a problem hiding this comment.
Test quality review
Test coverage for this PR is strong overall:
- TypeScript tests cover
ca,catalog,catalog-drift,config,headers,package,resolve-feedsand an end-to-end proxy case. - Rust tests cover the new codemod and extension code.
tests/compiler_tests.rsadds a compile-level test.- The smoke cases are markdown plus a
cases.jsonentry, which fits the lane model. - I found no deleted or ignored assertions. The dotnet test edits only swap the example host from
pkgs.dev.azure.comtopackages.example.testand still assert the same behaviour.
One advisory finding, posted inline: the bash-lint required-step list should include the new package-wrapper install step.
🧪 Test quality analysis by Test Quality Sentinel · auto · 20.9 AIC · ⊞ 11.1K
Comment /review to run again
There was a problem hiding this comment.
TypeScript review
The proxy changes look solid: the upstream sockets and requests have their late-error handlers, the credential is read after the allow decision, and redirects are never followed. One minor robustness finding is posted inline. The ts-critic sub-agent was not run in this pass; the findings above are from my own review.
🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 44.2 AIC · ⊞ 11.3K
Comment /review to run again
|
Thanks for the reviews. They're all addressed in
|
This comment has been minimized.
This comment has been minimized.
…ns.packages Agents can restore npm, PyPI, NuGet, and Cargo packages from Azure Artifacts feeds without holding a feed credential. The trusted ado-proxy container holds the build identity or a WIF-minted token and attaches it only to GET/HEAD requests for granted feeds, views, and protocols. - permissions.packages front matter (feeds, protocols, view, upstream, identity-role, optional service connection) and permissions-required.packages - ado-proxy catalog v2 package family with a separate credential slot, host-side feed GUID resolution and access preflight, blob-only redirect relay - per-process package-manager wrappers (npm, npx, pip, pip3, uv, dotnet, cargo) - runtimes.<x>.feed source selection and runtimes.<x>.public-registry: block - audit rollups by family/protocol and an upstream-unauthorized finding - codemod 0009 migrates Azure Artifacts runtimes.<x>.feed-url BREAKING CHANGE: Azure Artifacts runtimes.<x>.feed-url is replaced by permissions.packages plus runtimes.<x>.feed. Codemod 0009 migrates existing sources automatically. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b0ab0d4-23f4-4a89-90ea-3c40758ea550
…iable substitution Run 647310 proved the build-identity npm path end to end: three package GETs through ado-proxy, all allowed with 2xx upstream responses, including the 3.7 KB is-number tarball. npm then failed only because --pack-destination must already exist and the agent has no mkdir. Write the tarball to the working directory instead. The WIF agent stopped before running any check: it compared its prompt with the repository copy, saw $(Build.BuildId) replaced by a literal ID, and took that for prompt injection. Both prompts now say up front that pipeline variables are substituted and that repository files are not to be compared. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b0ab0d4-23f4-4a89-90ea-3c40758ea550
Run 647315 proved npm end to end with both credentials (build identity and WIF), but pip failed with SSLCertVerificationError against the proxy's leaf. Recent pip builds its TLS context with truststore (the system trust store) and PIP_CERT did not take effect; on Linux truststore reads OpenSSL's default verify paths, which honour SSL_CERT_FILE. The pip wrapper now sets SSL_CERT_FILE to the same system-roots-plus-proxy-CA bundle, still scoped to the one process. PIP_CERT stays for pip's legacy-certs mode. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b0ab0d4-23f4-4a89-90ea-3c40758ea550
…mpts Run 647325 proved npm and PyPI end to end with the WIF credential, including the npm tarball's 3xx relay to *.vsblob.vsassets.io. NuGet restore then failed with NU1301: Squid logged NuGet's CONNECT to pkgs.dev.azure.com coming straight from the agent, not from ado-proxy. NuGet takes its proxy from http_proxy even for HTTPS feeds, and that variable still named Squid. The dotnet wrapper now points http_proxy and HTTP_PROXY at the engine too. The engine forwards other cleartext to Squid and refuses cleartext to intercepted hosts. The build-identity agent ran no checks: it found its prompt as a repository file and decided the message was not its task. Both smoke prompts now open by saying this is the task, that the repo copy is expected, and that the repository is not to be inspected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b0ab0d4-23f4-4a89-90ea-3c40758ea550
Run 647387 still saw NuGet reach pkgs.dev.azure.com straight from the agent: dotnet never ran the package wrapper. AWF's entrypoint builds the agent PATH from the host PATH (which carries our $GITHUB_PATH prepends) and then prepends $DOTNET_ROOT, $CARGO_HOME/bin, $JAVA_HOME/bin, and others, so the real dotnet shadowed the wrapper; cargo would have done the same. Re-assert every awf_path_prepends directory at the front of PATH inside the engine command, which runs after the entrypoint. Entries are compiler-owned and character-checked because they are spliced into a single-quoted shell word. The package-feed smokes also pass --no-custom-instructions: the ado-aw repository's AGENTS.md otherwise loads as Copilot custom instructions and cast the agent as a repository contributor, which made it decline the smoke in several runs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b0ab0d4-23f4-4a89-90ea-3c40758ea550
…he registry Run 647440 proved the whole package path with both credentials: npm, pip, and NuGet restores through ado-proxy, the ungranted feed refused with 403, and no credential in the sandbox (the WIF case passed every check). The one failure: `public-registry: block` left registry.npmjs.org reachable, because the node ecosystem also allows the bare apex npmjs.org and AWF matches subdomains. Removal now drops any allowlist entry that would admit a blocked host under AWF's subdomain matching, and the npm block list names the npmjs.org, npmjs.com, and yarnpkg.com apexes explicitly. A new test compiles all three runtimes blocked (plus covering network.allowed entries) and asserts no remaining entry admits any blocked host. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b0ab0d4-23f4-4a89-90ea-3c40758ea550
Full candidate run 647482 passed every case, including package-feeds (build identity) and package-feeds-wif, so the "needs live validation" limitation is replaced by a description of what the smoke suite proves. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b0ab0d4-23f4-4a89-90ea-3c40758ea550
- resolve-feeds: a stdin read failure now returns the structured ##vso[task.logissue] error (exit 1) instead of rejecting with a raw stack trace; covered by a test. - bash lint: "Install package-manager wrappers (ado-proxy)" joins REQUIRED_STEP_DISPLAY_NAMES, so losing that fixture coverage fails the test. - codemods: add a warnings channel. Codemods stay pure: CodemodContext::warn records a note, CodemodReport::warnings carries it, and compile prints it under the "applied codemods" list. - codemod 0009 uses it when it migrates a feed-url onto an author's existing view-less grant that does not set `upstream: allow`, since the agent can no longer pull new upstream packages through it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b0ab0d4-23f4-4a89-90ea-3c40758ea550
17d6868 to
45c3ca4
Compare
This comment has been minimized.
This comment has been minimized.
…ge config
The package proxy only intercepts pkgs.dev.azure.com, so a committed
nuget.config, .npmrc, pip.conf, uv.toml, pyproject.toml, .yarnrc.yml, or
.cargo/config(.toml) that still names {org}.pkgs.visualstudio.com sends
restores to Azure Artifacts with no credential: they fail with 401 and
nothing says why. Codemod 0009 already rewrites front matter; this covers the
files the compiler does not own.
A new warning-only check scans those files at the source repository's git
root, finds legacy-host URLs (including npm's protocol-relative //host keys),
and suggests the canonical pkgs.dev.azure.com/{org}/... URL. It runs only
when the workflow can restore packages (permissions.packages or a
node/python/dotnet runtime). compile prints it, and `ado-aw lint` and the
lint_workflow MCP tool report it as `legacy-feed-url` warnings.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7b0ab0d4-23f4-4a89-90ea-3c40758ea550
This comment has been minimized.
This comment has been minimized.
…fuse Extends the committed package-config check: besides legacy-host URLs, it now reports pkgs.dev.azure.com feed URLs in nuget.config, .npmrc, pip.conf, and the rest that permissions.packages does not cover. The match mirrors the proxy's authorizePackageRequest. Each finding names its reason: feed-not-granted (organization, project, feed; an organization-scoped grant never covers a project-scoped URL), view-not-granted, protocol-not-granted, unknown-route, or no permissions.packages at all. It never reports a URL the proxy would allow. GUID identifiers and grants without organization cannot be decided before runtime resolution, so they are treated as matching. compile prints the warnings, and `ado-aw lint` and the lint_workflow MCP tool report them as `ungranted-feed-url`. Also fixes the AGENTS.md entry from the previous commit, where escaping had turned the backticks around `legacy-feed-url` into backslashes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b0ab0d4-23f4-4a89-90ea-3c40758ea550
This comment has been minimized.
This comment has been minimized.
|
/review |
|
✅ Rust Code Quality Reviewer completed the Rust code quality review.
|
There was a problem hiding this comment.
Compiler-contract review: no drift found. Codemod 0009 is registered, the PackageFeeds extension is wired into collect_extensions(), the ado-proxy catalog generated files moved with their source, and docs and AGENTS.md were updated. The new smoke cases don't add committed lock files, per-case definition IDs or format!-built shell. I did not check .gen.ts or .gen.json content against the Rust sources. Run npm run codegen in scripts/ado-script to confirm nothing drifts.
🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 21.5 AIC · ⊞ 12.6K
Comment /review to run again
There was a problem hiding this comment.
TypeScript review of scripts/ado-script/ found no merge-blocking defects. I checked the ado-proxy changes (resolve-feeds.ts, index.ts, server.ts) for async and error handling, secret leakage and external-input validation.
- Credential handling: In
resolve-feeds.tsthe token comes from stdin only, redirects are set tomanual, and every message passes throughredact. Logging-command output is escaped. - Network calls: Requests have a timeout and abort handling.
- Advisory, unchanged code:
void main()inado-proxy/index.tshas no.catch, so a throw fromrun()would be an unhandled rejection. This line is not part of the diff, so no inline comment was possible. - Advisory, minor:
(caught as Error).messagecasts inresolve-feeds.tsassume the thrown value is anError. Narrowing withcaught instanceof Errorwould be safer. - Sub-agent: I did not start
ts-critic, so this review is my own pass only.
🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 28.5 AIC · ⊞ 11.4K
Comment /review to run again
There was a problem hiding this comment.
Test quality review
Overall coverage is strong (TS unit/e2e for package.ts and resolve-feeds, codemod, audit, compile-level wiring, bash-lint fixture registered, smokes as markdown + cases.json). No assertions were weakened; the removed ones were split into stricter cases or retargeted to a neutral host. Advisory gaps are in the inline comments: cargo/npm wrapper branches unasserted and one vacuous || assertion.
🧪 Test quality analysis by Test Quality Sentinel · auto · 46.6 AIC · ⊞ 11.1K
Comment /review to run again
There was a problem hiding this comment.
Security review: no regressions found. The package-feed family is deny-by-default with GET/HEAD only. The credential is injected only after the policy decision and the client's own credential headers are stripped. Redirects are never followed, only relayed to blob-storage hosts or granted same-host routes. The wrapper and CA scripts use quoted bindings and carry no token (the Cargo helper prints a sentinel). The package token reaches only the trusted proxy step as a secret env var, not the Agent. Path parsing refuses traversal, encoded separators and double encoding. I read the diff, not the running code, and did not run the tests.
🔒 Security review by PR Security Reviewer · auto · 30.6 AIC · ⊞ 11.5K
Comment /review to run again
There was a problem hiding this comment.
Verdict: COMMENT — no merge-blocking Rust defects found.
Two advisory findings on the duplicated percent_decode helpers.
Themes
- Percent-decoding accepts signed hex (
%+a); duplicated in two files. - Not posted (low value / unverified line mapping): blocking
std::fsreads in async compile path, swallowedread_direrror and symlink-following in the config scan.
🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 31.5 AIC · ⊞ 11.3K
Comment /review to run again
Use shared strict percent-decoding so malformed package feed URL escapes are rejected consistently. Strengthen package-wrapper and service-connection regression assertions to cover the corrected behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2118822c-f81d-4b93-a8a9-662fca1d23fb
Prompt evaluationNote This is an advisory static review. Only Prompt Contracts is merge-blocking.
The only change to Potential regressionsNone identified. Per-case scores
Absolute per-criterion scores were not computed because the diff is a single added link, so base and candidate score the same on every criterion.
|
Summary
Agents can now restore packages from internal Azure Artifacts feeds with
npm,pip/uv,dotnet, andcargowithout ever holding a feed credential.The trusted
ado-proxycontainer, already used forpermissions.read, gainsa second package family. It holds the feed credential and attaches it only
to
GET/HEADrequests for the feeds, views, and protocols the workflowgrants. This is the follow-up to #2310 (merged), which removed the
*Authenticatetasks because they leaked the token into AWF. It addresses theinternal-feed half of #1823 and #253.
How it works
$(System.AccessToken)is mapped as a secret only intothe proxy start step.
AzureCLI@3mintsSC_PACKAGES_TOKEN, which is secret and in--exclude-env.starts. It is never placed in the Agent or Detection environment, argv,
workspace, or runner
/tmp.token(REST) andpackage_token(packages).
pkgs.dev.azure.com, and the packagecredential never reaches
dev.azure.com.GET/HEAD, a catalogued protocol route(
npm/registry/,pypi/simple|download/,nuget/v3/,cargo/), and agranted org/project/feed/view tuple. Feed GUID aliases are resolved on the
host before AWF starts.
ado-proxy.js resolve-feedsalso probes access. A failurestops the job with the feed, identity, and role to grant.
Locationonlyfor
*.vsblob.visualstudio.comand*.blob.core.windows.net, and theclient fetches the signed blob URL through Squid with no credential.
npm,npx,pip,pip3,uv,dotnet,cargo)set
HTTPS_PROXYand CA trust for that process only. CA trust is neverinstalled container-wide.
pkgs.dev.azure.comURLs, including ones in repository config,work unchanged.
python -m pip) fails closed with nocredential.
runtimes.<x>.feedsetsPIP_INDEX_URL/UV_DEFAULT_INDEX,NPM_CONFIG_REGISTRY, or an ensure-nuget.configstep, allcredential-free.
public-registry: blockremoves that ecosystem's public registry hostsfrom the AWF allowlist.
upstream: deny(the default) therefore requires either avieworidentity-role: reader, otherwise compilation fails.upstream: allowis an explicit opt-in.familyandprotocolfields;the log stays v1-compatible. The changes add rollups and an
upstream-unauthorizedfinding.Breaking change and codemod
An Azure Artifacts
runtimes.<x>.feed-urlis replaced bypermissions.packagesplusruntimes.<x>.feed. Codemod 0009 migrates itautomatically:
@viewis kept;upstream: allowis added when no view is present, preserving the buildidentity's previous behavior.
Non-Artifacts
feed-urlvalues are unchanged.Deviations from the plan (for reviewers)
{org}.pkgs.visualstudio.comnuget.config,.npmrc,pip.conf,.cargo/config.toml, …),compileandado-aw lintprint alegacy-feed-urlwarning with the canonical replacement (45c3ca4c).permissions.read.config:compileandado-aw lintreportlegacy-feed-url(bb5c55eb) andungranted-feed-url(1aa91802). The second covers feed, view, and protocol grants and mirrors the proxy's matching. Seedocs/package-feeds.md..cargo/config.toml(#1823).Live validation (AgentPlayground)
Two new candidate smoke cases run against a real Azure Artifacts feed
(
AgentPlaygroundTestFeed):package-feedsuses the build identity and blocks the public registries.package-feeds-wifusesagent-playground-readvia WIF.Feed, upstreams and grants are recorded in
tests/smoke/REGISTERED.md. A newsmokeCasesruntime parameter on the candidate orchestrator lets a run coveronly named cases.
Full candidate run 647482 at
b76ec48d(pre-rebase; the same tree is4a36c005after rebasing ontomain): all 7 cases green, each with its proof tag.package-feedsado-aw-packages-647486package-feeds-wifado-aw-packages-wif-647492ado-proxy(REST)ado-aw-proxy-647490custom-safe-outputado-aw-custom-job-647488canary,noop-target,multi-repoTogether these runs prove:
npm pack,pip downloadanddotnet add packagerestore through theproxy, including the redirect to
*.vsblob.vsassets.io;per-protocol scheme the catalog selects;
403;public-registry: blockholds.Bugs the live runs found (all fixed here, with tests)
*.vsblob.vsassets.io, which wasn't on the redirect allowlist4296be59adds the suffix to the catalogGETreturns200even withoutReadPackages, so the preflight passed for identities that couldn't restore4296be59: the preflight now lists one packageshell(npm)allows only a barenpm, so every real npm/pip/dotnet/git call was refusedmainseparatelytruststore, which ignoresPIP_CERTa053a74c: the pip wrapper also setsSSL_CERT_FILEhttp_proxy, which bypassed the engine8b01b1b4: the dotnet wrapper also setshttp_proxy$DOTNET_ROOTand similar dirs, so the realdotnetshadowed the wrapperb21407d9:awf_path_prependsdirs are re-asserted first inside the engine commandnpmjs.orgstill admittedregistry.npmjs.orgunderpublic-registry: block4a36c005: removal drops every entry that would admit a blocked hostTwo earlier wrong diagnoses (
231f9e22,37531104) are reverted by3d01595d.The smokes pass
--no-custom-instructions. Otherwise the checked-outrepository's AGENTS.md casts the agent as a contributor, and it declines the
task.
Review follow-ups from the first
/review:fb5496cc: exclusivity tests;a strict streamed-body truncation e2e test.
Docs:
new
docs/package-feeds.md;updated
ado-proxy-design.md,runtimes.md,network.md,codemods.md,imports.md,front-matter.md,safe-output-permissions.md, AGENTS.md,and README.
Test plan
All run locally on Windows with
shellcheckavailable:cargo build: passedcargo test --no-fail-fast: passed (3750 passed, 0 failed, 2 ignored)cargo clippy --all-targets: passed (no warnings)ENFORCE_BASH_LINT=1 cargo test --bin ado-aw compile::shell: passed (59)ENFORCE_BASH_LINT=1 cargo test --test bash_lint_tests: passed (2)cd scripts/ado-script && npm run codegen: passed; no drift beyond thecommitted catalog v2 artifacts
npm test: passed (97 files, 1369 tests, including newpackage,resolve-feeds, and proxy e2e cases for each protocol, denial, view, slotisolation, and canary absence)
npm run typecheck: passednpm run build: passednpx vitest run src/compiler-smoke-e2e: passed (292)npm run build:compiler-smoke-e2e: passedgit diff --check: flags only README table rows that keep the file'sexisting CRLF endings
Live: full candidate smoke run 647482 green (see Live validation above). After rebasing onto
main(which now holds #2310 and #2325), the branch tree is byte-identical to the one that was tested.