Repository navigation
http: keep discovery credentials when curl sanitizes the URL - #142
Merged
Merged
Conversation
friel-openai
marked this pull request as ready for review
October 14, 2026 19:11
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This was referenced Oct 14, 2026
friel-openai
force-pushed
the
af/codex/webdav-redirect-credentials
branch
from
October 16, 2026 05:23
3db54bf to
0ad3526
Compare
ttaylorr-oai
approved these changes
Oct 19, 2026
ttaylorr-oai
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the URL tracking change and regression test. The requested URL retains credentials when curl strips userinfo, while real redirects still select the destination credential context. Existing CI passes; this fixes the WebDAV failure on the current release base.
This was referenced Oct 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WebDAV pushes with credentials in the URL fail after curl 8.23.0 or a distribution backport of its CURLINFO_EFFECTIVE_URL security fix. Curl now removes userinfo from that result even without an HTTP redirect. Git treats the sanitized URL as a redirect and starts git-http-push without the username, so its PROPFIND request gets HTTP 401.
Use the requested URL already tracked by http_request_with_redirects() for effective_url and update_url_from_redirect(). Actual redirects still select the destination credential context before requesting it. Add a test that supplies a username and password, checks the remote ref, and rejects an unexpected prompt or redirect warning.
The source branch starts at the intended prerequisite
af/codex/http-auth-retry-redirect-v2(b81ca645fdf0), rather than a generatedcodextip. The fix and regression patch is unchanged by this ancestry correction (git range-diffreports equality). A separate commit retains the three exact CI workflow blobs accepted by the controller. The review target remainscodex; this is a review-only topic PR.Found in linux-meson t5540.18 while validating #141. The failure reproduces at #141's unchanged codex base; WebDAV uses git-http-push, not the send-pack negotiation code changed by #141. The same base tree passed on October 5 with Ubuntu curl 8.18.0-1ubuntu2.7. The failing run installed .8, which includes the October 14 CURLINFO_EFFECTIVE_URL security fix. This was a dependency change after passing CI, rather than an untested Git change. The regression test and code comment preserve the distinction between a requested URL and curl's sanitized URL.
The same curl behavior also causes dumb-HTTP authentication tests t5550.15, .16, and .20 to fail on #141’s explicit-haves prerequisite
a3eae730133768ea334731d2f48a80aff724fb5d. All three failures reproduce on that unchanged prerequisite with curl 8.23; all 63 tests pass with curl 8.5. The HTTP source and affected tests are unchanged by #141. PR126, the prerequisite of this PR, already fixes those dumb-HTTP cases by preserving authentication across discovery requests. Its source and this PR’s source each pass all 63 tests with curl 8.23 and SHA-256. This PR adds the WebDAV correction; its patch alone does not apply to the older automatic-redirect implementation.Validation:
A CI-only combined candidate tests #141, this PR, #143, and the existing HTTP prerequisites, including the version-specific Debian HTTP/2 quarantine. All 306 local tests across
t0610,t5540,t5549,t5550,t5556, andt5702pass on that composition with curl 8.23; combined CI completed successfully: 45 jobs passed, two expected jobs were skipped, and none failed. The candidate leaves topic source ancestry intact and contains no release or controller workflow.This is a client-only change compatible with existing HTTP servers and both tested curl versions. Clients can be updated independently. Reverting restores the WebDAV failure with the newer curl behavior; repository data formats are unchanged.
The tested candidate is
2006a35b7b78c1024e56f1658f7f1190f4b76a7c, assembled from public masterb8242b093d9e941a34460d715e3ce616a34ac3fewith these exact sources: explicit-haves prerequisitea3eae730133768ea334731d2f48a80aff724fb5d, #141772653bcb2b069505961b0b10604b0abf5a13062, HTTP redirect prerequisiteb81ca645fdf0bf70789691823f9d7a4abbb05697, gzip authentication prerequisite50f753ee38ef84de2fc22ca5cdde6698f0479f85, Debian HTTP/2 quarantine003e6089a2598e2f37eb2326ab3947836ebb86c0, #1420ad3526206bbe3c820e9aced7832c6d3e2c9e7ac, and #143b1013cd9c374194c94a35889dca67d7ca57fdb7a. This validates the combined source; human review and normal controller staging remain required.-frielbot
Codex Task:
codex://threads/01a13b95-2df7-75a1-bc99-4266c08d7d89