Skip to content

fix: secure HTTP redirect handling - #2099

Merged
ashwaniarya-adyen merged 5 commits into
Adyen:mainfrom
visheshupadhyay:fix/reject-insecure-redirects
Nov 3, 2026
Merged

ashwaniarya-adyen merged 5 commits into
Adyen:mainfrom
visheshupadhyay:fix/reject-insecure-redirects

Conversation

@visheshupadhyay

Copy link
Copy Markdown
Contributor

Fixes #2097 and #2098

@visheshupadhyay
visheshupadhyay requested a review from a team as a code owner October 24, 2026 19:12

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the custom redirect strategy to validate and only follow secure HTTPS redirects to Adyen domains for all redirect status codes, rather than restricting this check to 308 status codes. The tests have been updated and expanded to cover 307 redirects and non-HTTPS scenarios. Feedback suggests improving the host validation in isVerifyLocation to correctly handle bare domains and hostnames with a trailing dot.

@ashwaniarya-adyen

Copy link
Copy Markdown
Contributor

#2098
#2097
fixes these

@ashwaniarya-adyen
ashwaniarya-adyen force-pushed the fix/reject-insecure-redirects branch from 154749d to ab1e54b Compare October 28, 2026 09:34
@ashwaniarya-adyen

Copy link
Copy Markdown
Contributor

Hey @visheshupadhyay, thanks for the PR! The https check and validating all redirects instead of just 308 look good.

One small thing, could you revert the changes from the Gemini bot comment (allowing bare adyen.com / adyenpayments.com and stripping the trailing dot, plus the test for it)? All our APIs run on subdomains (*.adyen.com / *.adyenpayments.com), never on the bare domain, so we'd rather keep the original .endsWith(".adyen.com") || .endsWith(".adyenpayments.com") check as-is.

Once that's reverted, happy to merge!

@visheshupadhyay

Copy link
Copy Markdown
Contributor Author

Thanks @ashwaniarya-adyen for the clarification. I reverted the bare-domain and trailing-dot handling, added negative coverage for those cases, and kept the HTTPS validation for all redirects. The branch is updated and the tests pass.

Build Run
BuildRun
Test Run
TestRun

@ashwaniarya-adyen

Copy link
Copy Markdown
Contributor

Thanks @ashwaniarya-adyen for the clarification. I reverted the bare-domain and trailing-dot handling, added negative coverage for those cases, and kept the HTTPS validation for all redirects. The branch is updated and the tests pass.

Build Run
BuildRun ```shell Test Run ``` TestRun

Thanks for the quick update @visheshupadhyay! Could you just run mvn spotless:apply and push?

@visheshupadhyay

Copy link
Copy Markdown
Contributor Author

Hi @ashwaniarya-adyen,

I've applied the requested Spotless formatting changes and pushed them in commit 04484c1.
The earlier redirect validation feedback has also been addressed, including preserving the original Adyen subdomain restrictions.
Could you please take another look when you have a chance? Let me know if anything else needs to be adjusted.

Thanks again for your review!

@ashwaniarya-adyen
ashwaniarya-adyen force-pushed the fix/reject-insecure-redirects branch from 04484c1 to b260cca Compare November 3, 2026 08:33
@ashwaniarya-adyen
ashwaniarya-adyen added this pull request to the merge queue Nov 3, 2026
Merged via the queue into Adyen:main with commit 69b65b5 Nov 3, 2026
6 checks passed
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.

[Bug]: Reject untrusted 307 redirect locations in the HTTP client [Bug]: Reject HTTP destinations for 308 redirects in the HTTP client

2 participants