Skip to content

url: detect proxy changes read from environment#21666

Closed
bagder wants to merge 3 commits into
masterfrom
bagder/env-proxy-change
Closed

url: detect proxy changes read from environment#21666
bagder wants to merge 3 commits into
masterfrom
bagder/env-proxy-change

Conversation

@bagder
Copy link
Copy Markdown
Member

@bagder bagder commented May 19, 2026

No description provided.

@testclutch
Copy link
Copy Markdown

Analysis of PR #21666 at a8605a04:

Test 3207 failed, but it has been 1.4% flaky lately, so it's probably NOT a fault of the PR. Note that this test has failed in 4 different CI jobs (the link just goes to one of them). Note that this CI job has had a number of other flaky tests recently (3, to be exact) so it may be that this failure is rather a systemic issue with this job and not with this specific PR.

Generated by Testclutch

@bagder bagder requested a review from Copilot May 19, 2026 13:09
@bagder bagder marked this pull request as ready for review May 19, 2026 13:09
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds tracking for proxy changes (notably those coming from environment proxy detection) so libcurl can reset proxy Digest auth state when the proxy changes between transfers on the same easy handle. It also introduces a new libtest-based regression test case to exercise this behavior.

Changes:

  • Track the last-used proxy string in UrlState and clear state.proxydigest when the proxy changes.
  • Add new libtest lib1630 plus test spec test1630 to validate proxy change handling across two transfers.
  • Wire the new test into the test build system.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/libtest/Makefile.inc Adds lib1630.c to the libtest build.
tests/libtest/lib1630.c New libtest that performs two transfers relying on env proxies and proxy Digest auth.
tests/data/test1630 New test definition for the regression scenario.
tests/data/Makefile.am Adds the new test case to the test list (currently duplicated).
lib/urldata.h Adds UrlState.envproxy to track last proxy string.
lib/url.c Frees envproxy on cleanup and clears proxy Digest state when proxy changes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/data/Makefile.am Outdated
Comment thread lib/url.c Outdated
Comment thread tests/data/test1630 Outdated
Comment thread lib/urldata.h Outdated
bagder added 2 commits May 19, 2026 15:23
and make sure Digest state is flushed in the second use
When a proxy is set from an environment variable, detect if that proxy
is not the same as previously and flush state.
@bagder bagder force-pushed the bagder/env-proxy-change branch from 303401f to a5a3988 Compare May 19, 2026 13:26
@bagder
Copy link
Copy Markdown
Member Author

bagder commented May 19, 2026

augment review

@augmentcode
Copy link
Copy Markdown

augmentcode Bot commented May 19, 2026

🤖 Augment PR Summary

Summary: This PR makes libcurl detect when the effective proxy (as resolved from environment variables) changes between transfers, and resets proxy Digest auth state accordingly.

Changes:

  • Track the last proxy string used in a new UrlState.envproxy field.
  • In url_set_conn_proxies(), compare the current proxy string against the cached one and, when it changes, clear the cached proxy Digest auth data (proxydigest).
  • Free the cached proxy string during Curl_close() cleanup.
  • Add a new regression test (test1647 + lib1647.c) that first authenticates to an HTTP proxy with Digest, then switches proxies via env vars and verifies state is not reused.
  • Update test Makefiles to include the new test case and libtest source.

Technical Notes: The intent appears to be preventing proxy Digest state (nonce/realm/etc) from being reused after the proxy endpoint changes, which could otherwise cause incorrect/unsafe preemptive auth behavior.

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 1 suggestion posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread lib/url.c
@bagder bagder closed this in 5c22538 May 19, 2026
@bagder bagder deleted the bagder/env-proxy-change branch May 19, 2026 15:16
outcast36 pushed a commit to greearb/curl that referenced this pull request Jun 3, 2026
When a proxy is set from an environment variable, detect if that proxy
is not the same as previously and flush state.

Verified by test1647: verify changing proxy with env variables and make
sure Digest state is flushed in the second use

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

Development

Successfully merging this pull request may close these issues.

3 participants