Skip to content

GROOVY-12274: HttpBuilder: drop default headers on a redirect to anot… - #2811

Open
paulk-asert wants to merge 1 commit into
apache:masterfrom
paulk-asert:groovy12274
Open

GROOVY-12274: HttpBuilder: drop default headers on a redirect to anot…#2811
paulk-asert wants to merge 1 commit into
apache:masterfrom
paulk-asert:groovy12274

Conversation

@paulk-asert

Copy link
Copy Markdown
Contributor

…her origin

With followRedirects and no confinement, redirect following was delegated to the JDK client, which re-issued the request to the Location target carrying the headers configured on the builder. Those headers are applied to every request, so a token or key among them was sent to whatever origin a redirect named.

The platform cannot be relied on for this. Measured against a cross-host redirect, on a chain that ends at a server which records what it received:

                    JDK 17.0.20    JDK 21.0.6    JDK 23.0.2
                    (2026-07)      (2025-01)     (2025-01)

Authorization stripped forwarded forwarded
Cookie stripped forwarded forwarded
Proxy-Authorization stripped stripped stripped
X-Api-Key forwarded forwarded forwarded

Two things follow. Whether the well known credential headers are protected depends on the update level of the JDK in use, which an application cannot choose. And a header the platform does not recognise is forwarded on every JDK, while the builder's headers may hold anything the caller put there, so a policy naming header names would repeat the same mistake at one remove.

Follow redirects here in every case rather than only under confinement, and drop the caller's headers for good once a hop leaves the origin the request started from. Same-origin redirects are unaffected, as is confinement, which already rejected a cross-origin hop outright. A chain which returns to the original origin does not get the headers back, since by then they have been seen by another server.

Note this drops all of the caller's headers rather than a chosen few: which of them carry credentials is not something this class can know.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.1862%. Comparing base (f140841) to head (aa5e91c).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...der/src/main/groovy/groovy/http/HttpBuilder.groovy 78.5714% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##               master      #2811        +/-   ##
==================================================
+ Coverage     70.1837%   70.1862%   +0.0025%     
- Complexity      35844      35845         +1     
==================================================
  Files            1562       1562                
  Lines          132528     132536         +8     
  Branches        24379      24381         +2     
==================================================
+ Hits            93013      93022         +9     
  Misses          31106      31106                
+ Partials         8409       8408         -1     
Files with missing lines Coverage Δ
...der/src/main/groovy/groovy/http/HttpBuilder.groovy 75.2809% <78.5714%> (+1.1496%) ⬆️

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@testlens-app

This comment has been minimized.

This comment was marked as outdated.

…her origin

With followRedirects and no confinement, redirect following was delegated to
the JDK client, which re-issued the request to the Location target carrying
the headers configured on the builder. Those headers are applied to every
request, so a token or key among them was sent to whatever origin a
redirect named.

The platform cannot be relied on for this. Measured against a cross-host
redirect, on a chain that ends at a server which records what it received:

                        JDK 17.0.20    JDK 21.0.6    JDK 23.0.2
                        (2026-07)      (2025-01)     (2025-01)
  Authorization         stripped       forwarded     forwarded
  Cookie                stripped       forwarded     forwarded
  Proxy-Authorization   stripped       stripped      stripped
  X-Api-Key             forwarded      forwarded     forwarded

Two things follow. Whether the well known credential headers are protected
depends on the update level of the JDK in use, which an application cannot
choose. And a header the platform does not recognise is forwarded on every
JDK, while the builder's headers may hold anything the caller put there, so
a policy naming header names would repeat the same mistake at one remove.

Follow redirects here in every case rather than only under confinement, and
drop the caller's headers for good once a hop leaves the origin the request
started from. Same-origin redirects are unaffected, as is confinement, which
already rejected a cross-origin hop outright. A chain which returns to the
original origin does not get the headers back, since by then they have been
seen by another server.

Note this drops all of the caller's headers rather than a chosen few: which
of them carry credentials is not something this class can know.
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.

3 participants