Skip to content

fix: Resolve relative redirect locations - #4605

Closed
DorianChn wants to merge 3 commits into
google:masterfrom
DorianChn:shanchuanzhi/fix-relative-redirect-resolution
Closed

DorianChn wants to merge 3 commits into
google:masterfrom
DorianChn:shanchuanzhi/fix-relative-redirect-resolution

Conversation

@DorianChn

Copy link
Copy Markdown

Summary

  • Resolve relative redirect locations against the request URL that received the response, not the API base URL.
  • Apply the behavior to 301 redirects and artifact, workflow-log, and archive download links.
  • Preserve redirect validation and add regression coverage for rate-limited and non-rate-limited artifact downloads.

Why

Relative Location values are resolved against the responding request URL; using the API base URL can discard the endpoint path or return an unusable relative URL. This follows http.Response.Location.

Validation

  • bash script/test.sh ./... — all 11 modules passed.
  • go vet ./... — passed.
  • Root custom golangci-lint — 0 issues; gofmt and git diff --check passed.
  • script/lint.sh reached the generated-file check, which reports the local go.sum CRLF/LF mismatch; no unrelated line-ending change is included.

AI assistance

AI assistance helped draft the implementation and regression tests; the contributor reviewed the diff and ran the listed validations.

@codecov

codecov Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.62%. Comparing base (c273cd8) to head (f3a5bf2).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4605   +/-   ##
=======================================
  Coverage   98.61%   98.62%           
=======================================
  Files         199      199           
  Lines       18664    18668    +4     
=======================================
+ Hits        18406    18411    +5     
+ Misses        258      257    -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gmlewis

gmlewis commented Oct 3, 2026

Copy link
Copy Markdown
Collaborator

Is this PR fixing a hypothetical issue that has never been emperically found in the wild, or has this problem ever been actually seen when using this client library?

When would a relative URL ever be returned by GitHub?

How has this client library been working for decades without ever encountering a relative URL?

@DorianChn

Copy link
Copy Markdown
Author

Thanks for asking. I do not have a production capture showing GitHub.com returned a relative Location, so I cannot claim this was observed in a live GitHub response. I agree the current PR should be understood as standards-compliant client handling, not evidence that GitHub.com currently emits relative redirect values.

The protocol permits it: RFC 9110 §10.2.2 defines Location as a URI-reference and requires relative references to be resolved against the target URI (https://www.rfc-editor.org/rfc/rfc9110.html#section-10.2.2). The regression here is in the client’s resolution base: using c.baseURL can lose the responding endpoint path (including a GitHub Enterprise /api-v3/ prefix); resolution against the request URL preserves it. The tests use a controlled HTTP server to verify that behavior, rather than asserting that GitHub’s production service returns relative values.

I also addressed the Codecov line by parsing each Location once, checking traversal on the parsed URL, and covering malformed values. After syncing current master, all 11 Go module test directories passed, go vet ./... passed, and targeted coverage for resolveAndCheckRedirect is 100%. The repository lint script remains locally blocked by Windows/Git Bash tooling and schema-cache path behavior; GitHub checks on this head will provide the authoritative CI result.

@gmlewis

gmlewis commented Oct 14, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the reply, @DorianChn, but I'm very uncomfortable with this change to a critical part of the client library without there being evidence-based reasons from actual GitHub v3 API usage.

Closing.

@gmlewis gmlewis closed this Oct 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants