Repository navigation
Conversation
Follow-up to 7a6bd02: make sure data->info.wouldredirect (exposed via CURLINFO_REDIRECT_URL and %{redirect_url}) does not contain credentials inherited from the original request URL or netrc when resolving relative redirect targets in FOLLOW_FAKE mode. Verified in test 1877
| HTTP/1.1 301 redir | ||
| Content-Length: 0 | ||
| Content-Type: text/html | ||
| Location: %TESTNUMBER0002 |
There was a problem hiding this comment.
This URL does not contain any credentials. Doesn't that make this test succeed even without this patch?
There was a problem hiding this comment.
Nah right, it does the same as the effective url...
There was a problem hiding this comment.
Exactly, since it's a relative redirect resolved against data->state.uh (which holds the netrc credentials), without this patch wouldredirect retains and outputs the user:password@ credentials, causing the test to fail. With the patch, they are stripped cleanly.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Sanitization failures can still expose the original credential-bearing redirect URL.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Prevents redirect metadata from exposing credentials inherited during relative URL resolution.
Changes:
- Strips user and password fields from fake redirect URLs.
- Adds and registers a netrc regression test.
| File | Description |
|---|---|
tests/data/test1877 |
Verifies redirect URLs omit netrc credentials. |
tests/data/Makefile.am |
Registers the regression test. |
lib/http.c |
Sanitizes stored redirect URLs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if(u) { | ||
| if(!curl_url_set(u, CURLUPART_URL, follow_url, | ||
| CURLU_NON_SUPPORT_SCHEME | | ||
| (data->set.path_as_is ? CURLU_PATH_AS_IS : 0)) && | ||
| !curl_url_set(u, CURLUPART_USER, NULL, 0) && | ||
| !curl_url_set(u, CURLUPART_PASSWORD, NULL, 0)) | ||
| (void)curl_url_get(u, CURLUPART_URL, &nocred, CURLU_GET_EMPTY); | ||
| curl_url_cleanup(u); | ||
| } | ||
| if(nocred) { | ||
| curlx_free(follow_url); | ||
| follow_url = nocred; | ||
| } |
There was a problem hiding this comment.
Updated to fail closed in 54d40c7: follow_url is now unconditionally freed and wouldredirect is set directly to nocred (NULL on allocation/parsing failure), preventing credential leakage under memory pressure.
Ensure that if curl_url() or URL redaction fails, the credential-bearing follow_url is freed and wouldredirect is set to NULL rather than leaking credentials under allocation failure.
When Location URL parsing fails (e.g. test 1518 with invalid space in URL), keep the raw duplicated Location header without credential stripping. When URL parsing succeeded, strip credentials and return CURLE_OUT_OF_MEMORY if curl_url() fails.
|
Thanks! |
Summary
Follow-up to commit 7a6bd02 (#23016): make sure
data->info.wouldredirect(exposed viaCURLINFO_REDIRECT_URLand%{redirect_url}) does not contain credentials inherited from the original request URL or netrc when resolving relative redirect targets inFOLLOW_FAKEmode.Background & Mechanism
In 7a6bd02,
CURLINFO_EFFECTIVE_URLwas hardened to ensure credentials fromnetrcor userinfo are not leaked to callers.However, when a redirect (HTTP 3xx) is received and
CURLOPT_FOLLOWLOCATIONis not enabled (ormaxredirsis reached),Curl_http_follow()runs intype == FOLLOW_FAKEmode (lib/http.c:1289). In this branch:data->state.uhresolves the relativeLocation:target URL against the base URL, which retains the original user and password.curl_url_get(data->state.uh, CURLUPART_URL, &follow_url, 0)generates the full redirect URL containinguser:password@.follow_urlwas stored directly intodata->info.wouldredirect(lib/http.c:1292) and returned verbatim to callers viacurl_easy_getinfo(..., CURLINFO_REDIRECT_URL, ...)and%{redirect_url}.Fix
In
Curl_http_follow(), whentype == FOLLOW_FAKE, strip credentials (CURLUPART_USERandCURLUPART_PASSWORD) fromfollow_urlbefore storing it intodata->info.wouldredirect.Verification
Added
tests/data/test1877(mirroringtest1876), verifying that%{redirect_url}on a 301 redirect with netrc credentials outputs the target URL without credentials. Validated withscripts/checksrc.pl.