Skip to content

getinfo: make sure CURLINFO_EFFECTIVE_URL does not contain creds - #23016

Closed
bagder wants to merge 3 commits into
masterfrom
bagder/effective-url-creds
Closed

bagder wants to merge 3 commits into
masterfrom
bagder/effective-url-creds

Conversation

@bagder

@bagder bagder commented Sep 21, 2026

Copy link
Copy Markdown
Member

Reported-by: Joshua Rogers

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The implementation corrupts CURLINFO_REDIRECT_URL state and regresses valid unsupported-scheme and path-as-is URLs.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 Medium severity · 1 Low severity

Open (3)
What changed in this PR

Prevents credentials from appearing in CURLINFO_EFFECTIVE_URL.

Changes:

  • Sanitizes effective URLs through the URL API.
  • Adds netrc redirect coverage.
  • Removes an unsupported-scheme test case.
File Description
lib/​getinfo.c Sanitizes effective URLs.
lib/​urldata.h Updates redirect storage documentation.
tests/​data/​test1876 Adds netrc credential regression coverage.
tests/​data/​test424 Removes unsupported-scheme coverage.
tests/​data/​Makefile.am Registers test 1876.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/getinfo.c Outdated
Comment thread lib/getinfo.c Outdated
Comment thread tests/data/test1876 Outdated
@bagder
bagder force-pushed the bagder/effective-url-creds branch from 8f9a29e to 5ec0321 Compare September 21, 2026 10:12
bagder and others added 2 commits September 21, 2026 12:13
ok then

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@bagder
bagder marked this pull request as ready for review September 21, 2026 10:27
@testclutch

Copy link
Copy Markdown

Analysis of PR #23016 at d6abaf89:

Test ../../tests/http/test_21_resolve.py::TestResolve::test_21_07_dnsd_a[DoH] failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them).

Test ../../tests/http/test_21_resolve.py::TestResolve::test_21_08_dnsd_aaaa[DoH] failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them).

Test ../../tests/http/test_21_resolve.py::TestResolve::test_21_09_dnsd_a_delay[DoH] failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them).

Test ../../tests/http/test_21_resolve.py::TestResolve::test_21_10_dnsd_aaaa_delay[DoH] failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them).

Test ../../tests/http/test_21_resolve.py::TestResolve::test_21_13_dnsd_nxdomain_cached[DoH] failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them).

Generated by Testclutch

@bagder bagder closed this in 7a6bd02 Sep 21, 2026
@bagder
bagder deleted the bagder/effective-url-creds branch September 21, 2026 12:05
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