Skip to content

Better handle redirects to https in ping#2225

Merged
jonjohnsonjr merged 1 commit intogoogle:mainfrom
jonjohnsonjr:basic-schemes
Mar 2, 2026
Merged

Better handle redirects to https in ping#2225
jonjohnsonjr merged 1 commit intogoogle:mainfrom
jonjohnsonjr:basic-schemes

Conversation

@jonjohnsonjr
Copy link
Copy Markdown
Collaborator

When we pass --insecure into crane, we permit http responses for any host, which means we do this fun little happy eyeballs race between https and http for hosts-that-we-expect-to-be-served-over-https.

A common thing for web servers to do is to redirect http to https.

Our little happy eyeballs race assumes that if we ping http and get a successful response, that we should use http going forward. This is naive in the case where the reason we received a happy response is that the server redirected us to the https version (and we followed it).

This happens rarely (when the https reponse takes longer than the fallback delay) so we don't often see it, but if that does happen we end up failing to attach credentials, which isn't great.

To fix this, we can just look the scheme we used in the (sometimes redirected) request URL rather than the scheme we used for the original request.

@jonjohnsonjr jonjohnsonjr marked this pull request as draft March 2, 2026 21:25
When we pass --insecure into crane, we permit http responses for any
host, which means we do this fun little happy eyeballs race between
https and http for hosts-that-we-expect-to-be-served-over-https.

A common thing for web servers to do is to redirect http to https.

Our little happy eyeballs race assumes that if we ping http and get a
successful response, that we should use http going forward. This is
naive in the case where the reason we received a happy response is that
the server redirected us to the https version (and we followed it).

This happens rarely (when the https reponse takes longer than the
fallback delay) so we don't often see it, but if that does happen we
end up failing to attach credentials, which isn't great.

To fix this, we can just look the scheme we used in the (sometimes
redirected) request URL rather than the scheme we used for the original
request.

Signed-off-by: Jon Johnson <jon.johnson@chainguard.dev>
@jonjohnsonjr jonjohnsonjr marked this pull request as ready for review March 2, 2026 21:31
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.17%. Comparing base (8b3c303) to head (23aec82).
⚠️ Report is 67 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (8b3c303) and HEAD (23aec82). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (8b3c303) HEAD (23aec82)
2 1
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2225       +/-   ##
===========================================
- Coverage   71.67%   53.17%   -18.51%     
===========================================
  Files         123      164       +41     
  Lines        9935    10977     +1042     
===========================================
- Hits         7121     5837     -1284     
- Misses       2115     4433     +2318     
- Partials      699      707        +8     

☔ View full report in Codecov by Sentry.
📢 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.

@jonjohnsonjr jonjohnsonjr enabled auto-merge (squash) March 2, 2026 22:00
@jonjohnsonjr jonjohnsonjr merged commit 9e0ccb0 into google:main Mar 2, 2026
17 checks passed
@jonjohnsonjr jonjohnsonjr deleted the basic-schemes branch March 2, 2026 22:05
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.

4 participants