Skip to content

fix(sign): Fixed proxy routing with native fetch - #3898

Open
NUAAwqy wants to merge 3 commits into
mozilla:masterfrom
NUAAwqy:taiyi/fix-signing-api-proxy
Open

NUAAwqy wants to merge 3 commits into
mozilla:masterfrom
NUAAwqy:taiyi/fix-signing-api-proxy

Conversation

@NUAAwqy

@NUAAwqy NUAAwqy commented Oct 13, 2026

Copy link
Copy Markdown

Fixes #3413.

The signing client passes https-proxy-agent using the agent option, which native Node fetch ignores. As a result, web-ext sign --api-proxy ... sends requests directly instead of through the requested proxy. This affects uploads, validation/approval polling, source patches, and signed-file downloads.

Use Undici's ProxyAgent via the supported dispatcher option. Replace the unused https-proxy-agent dependency with Undici 6, which supports the existing Node >=20.0.0 engine range. The lockfile retains Cheerio's existing Undici 7 version under its own dependency tree.

Each request closes its dispatcher gracefully once the response body is consumed. Cleanup is initiated after fetch returns headers without awaiting it, so callers can consume multipart responses and streamed downloads; failed requests also initiate cleanup. No global dispatcher is changed.

Executed validation

The new tests use real loopback origin and HTTP CONNECT proxy servers, with no external service or real AMO credentials. They check GET polling, JSON PUT, multipart POST/PATCH, chunked binary downloads, URL-encoded proxy credentials, rejection without a direct fallback, and no-proxy requests.

  • Original source: 7 new tests failed, 1 passed; the origin received direct requests while the proxy received none.
  • Fixed source: all 8 new tests passed.
  • Original npm test: 589 passed.
  • Fixed npm test: 597 passed, including ESLint checks.
  • npm run test-functional: 20 passed, 4 existing pending.
  • npm run build and Prettier checks: passed.

Validated on Node 22.22.0. The transport regressions use HTTP origins and an HTTP CONNECT proxy; they do not exercise a real AMO submission or a live enterprise TLS proxy.

Implementation and test execution were assisted by Codex.

Replace the ignored agent option with an Undici proxy dispatcher. Close each dispatcher gracefully without blocking response body consumption. Add real CONNECT proxy regressions for uploads, polling, downloads, credentials and failure behavior.

Fixes mozilla#3413

Signed-off-by: NUAAwqy <146524024+NUAAwqy@users.noreply.github.com>
@NUAAwqy NUAAwqy closed this Oct 22, 2026
@NUAAwqy NUAAwqy reopened this Oct 22, 2026
@NUAAwqy

NUAAwqy commented Oct 22, 2026

Copy link
Copy Markdown
Author

Closed/reopened to re-trigger CI. The previous build-nodejs-current failure (2026-10-13) did not reproduce locally: npm run prettier-ci and npm run test-coverage (597 unit tests, node 22.22) both pass on the current head, so it looked like one of the known-flaky functional test intermittent failures. Note: npm run audit-deps currently fails on the unchanged dev-dependency chain (nyc -> sprintf-js, GHSA-hp3w-g68c-fv3c), which is a baseline issue unrelated to this PR.

@NUAAwqy NUAAwqy closed this Oct 25, 2026
@NUAAwqy NUAAwqy reopened this Oct 25, 2026
Signed-off-by: NUAAwqy <NUAAwqy@users.noreply.github.com>
@NUAAwqy

NUAAwqy commented Oct 25, 2026 •

Copy link
Copy Markdown
Author

The build-nodejs-current failure is caused by two undici security advisories published on 2026-10-23: GHSA-xjj6-f33q-929c (high, DoS via unhandled error on excessive HTTP/2 frames) and GHSA-4qv8-2p9j-2685 (medium), which affect undici >= 6.23.0 < 6.29.1 — including the undici@6.29.0 this PR adds. npm run audit-deps (the last step of the build job) therefore blocks the build.

I pushed a fix bumping our direct dependency to undici@6.29.1 (patched), and also bumped the transitive cheerio -> undici 7.30.0 to 7.30.1 in the lockfile, since that one now hits GHSA-xjj6-f33q-929c and GHSA-f72v-6w45-q472 on the 7.x line too. Note the second part is a baseline issue that will also affect master audit runs until its lockfile is refreshed. Happy to drop the second bump if maintainers prefer handling it separately.

…new advisories

Signed-off-by: NUAAwqy <NUAAwqy@users.noreply.github.com>
@NUAAwqy

NUAAwqy commented Oct 25, 2026

Copy link
Copy Markdown
Author

Follow-up: the re-run failed npm run audit-deps again on newly published advisories affecting other packages in the tree: fast-uri >= 3.0.0 < 3.1.9 (GHSA-w74x-v6f9-j3xg, GHSA-xhq7-xmqj-769h, GHSA-wjr9-j795-jxcr, GHSA-333x-w4r6-6crp) and serialize-javascript < 7.1.3 (GHSA-m5mm-p8h7-m29v). I verified locally with Node 22.22 that npm audit fix (fast-uri 3.1.8 -> 3.1.9, serialize-javascript 7.1.2 -> 7.1.4) makes npm run audit-deps pass (exit 0; only the .nsprc-ignored node-forge items remain). Pushed as 06006dd. These two bumps are baseline drift that would hit master too — happy to split them out if maintainers prefer.

@NUAAwqy NUAAwqy closed this Oct 27, 2026
@NUAAwqy NUAAwqy reopened this Oct 27, 2026
@NUAAwqy

NUAAwqy commented Oct 27, 2026

Copy link
Copy Markdown
Author

Re-triggering the CircleCI job (close/reopen) after the lockfile refresh: is green on the current head, so this remaining failure is likely unrelated/flaky.

@NUAAwqy NUAAwqy closed this Oct 27, 2026
@NUAAwqy NUAAwqy reopened this Oct 27, 2026
@NUAAwqy

NUAAwqy commented Oct 27, 2026

Copy link
Copy Markdown
Author

Re-triggering the CircleCI job (close/reopen) after the lockfile refresh: is green on the current head, so this remaining failure is likely unrelated/flaky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--api-proxy is ignore when signing

2 participants