fix: authenticate python index requests on same host regardless of path#114
fix: authenticate python index requests on same host regardless of path#114thavaahariharangit wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts the Python index request authentication logic in the proxy so credentials can be applied based on host matching even when the request path differs from the configured index-url, and adds a test intended to cover package downloads on different paths.
Changes:
- Update
PythonIndexHandlerto consider host:port matches forindex-urlcredentials (in addition to path-prefix matching and explicithostmatching). - Add a new test case for authenticating a package download URL on a different path on the same host.
Show a summary per file
| File | Description |
|---|---|
internal/handlers/python_index.go |
Expands matching logic to authenticate by host:port (ignoring path) for index-url credentials. |
internal/handlers/python_index_test.go |
Adds a test case for a package download path on the same host. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 4
| // 2. Host:port matches (regardless of path), OR | ||
| // 3. Explicit host field matches | ||
| if !helpers.UrlMatchesRequest(req, indexURL, true) && | ||
| !helpers.UrlMatchesRequest(req, indexURL, false) && |
There was a problem hiding this comment.
helpers.UrlMatchesRequest(req, indexURL, false) makes index-url credentials apply to any request on the same host:port (regardless of path). This is a behavioral change from URL+path prefix scoping and will cause previously-path-mismatched requests (e.g. /foo when configured for /pyreg/) to be authenticated, potentially leaking credentials to unrelated endpoints on the same host. Consider keeping path scoping for index-url and only allowing host-only matching when an explicit host field is configured (as other handlers do), or introduce an explicit opt-in for host-only matching rather than inferring it from index-url.
| // 2. Host:port matches (regardless of path), OR | |
| // 3. Explicit host field matches | |
| if !helpers.UrlMatchesRequest(req, indexURL, true) && | |
| !helpers.UrlMatchesRequest(req, indexURL, false) && | |
| // 2. Explicit host field matches | |
| if !helpers.UrlMatchesRequest(req, indexURL, true) && |
| // Simulates: config pypi.cyco.fun/pypi, but request to pypi.cyco.fun/packages/... | ||
| // Using corp.deltaforce.com which has / as the index path | ||
| req = httptest.NewRequest("GET", "https://corp.deltaforce.com/packages/somepkg/1.0/wheel.whl", nil) | ||
| req = handleRequestAndClose(handler, req, nil) | ||
| assertHasBasicAuth(t, req, deltaForceUser, deltaForcePassword, "cert registry with package download on different path") |
There was a problem hiding this comment.
This new test case doesn't actually validate the new host-only matching behavior: the configured index-url for corp.deltaforce.com is /, and the existing prefix-matching logic already authenticates every path on that host (because the trimmed path prefix becomes empty). To cover the intended behavior, add a case where the configured index-url has a non-root path (e.g. /pypi/ or /dependabot/) and the request goes to a different top-level path (e.g. /packages/...), and assert the expected auth behavior.
| if !helpers.UrlMatchesRequest(req, indexURL, true) && | ||
| !helpers.UrlMatchesRequest(req, indexURL, false) && |
There was a problem hiding this comment.
This multiline if condition has trailing whitespace after &&, which will fail gofmt/linters in many setups. Please run gofmt (it will also reformat the line breaks/indentation for this condition).
| if !helpers.UrlMatchesRequest(req, indexURL, true) && | |
| !helpers.UrlMatchesRequest(req, indexURL, false) && | |
| if !helpers.UrlMatchesRequest(req, indexURL, true) && | |
| !helpers.UrlMatchesRequest(req, indexURL, false) && |
| // Fall back to static credentials | ||
| for _, cred := range h.credentials { | ||
| indexURL := simpleSuffixRe.ReplaceAllString(cred.indexURL, "/") | ||
| if !helpers.UrlMatchesRequest(req, indexURL, true) && !helpers.CheckHost(req, cred.host) { | ||
| // Apply credentials if: | ||
| // 1. URL matches with path (e.g., /pypi/...), OR | ||
| // 2. Host:port matches (regardless of path), OR | ||
| // 3. Explicit host field matches | ||
| if !helpers.UrlMatchesRequest(req, indexURL, true) && | ||
| !helpers.UrlMatchesRequest(req, indexURL, false) && | ||
| !helpers.CheckHost(req, cred.host) { |
There was a problem hiding this comment.
The new host-only matching is only applied to static credentials in the fallback loop. When a python_index credential is OIDC-configured, NewPythonIndexHandler registers it in oidcRegistry and does not add it to h.credentials, so requests on the same host but different paths will still not be authenticated via OIDC (since OIDCRegistry.TryAuth is path-prefix based). If the reported issue affects OIDC-based python index auth too, this change won't address it; consider whether OIDC registrations for python indexes should also support host-only matching (or an explicit opt-in).
What are you trying to accomplish?
Anything you want to highlight for special attention from reviewers?
How will you know you've accomplished your goal?
Checklist