Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use relative URLs in /login redirects #14714

Merged
merged 2 commits into from
Dec 18, 2024
Merged

Conversation

Pistolnik
Copy link
Contributor

@Pistolnik Pistolnik commented Mar 10, 2024

TL;DR

Closes #7273

Motivation

I noticed inconsistency in all tests that are using andExpect(MockMvcResultMatchers.redirectedUrl("/xyz")). I was able to test everything using relative urls with one exception - /login - absolute paths had to be used there. Same pattern is in the whole repo - /login redirect asserts are using http://localhost/login (or similar).
LoginUrlAuthenticationEntryPoint is responsible - namely buildRedirectUrlToLoginPage that always constructs absolute path.

There are probably no other than historical reasons for this behavior (as mentioned in #7273). At time of introducing the the class, absolute URI was required by RFC 2616. The RFC is obsoleted by RFC 7231 since 2014. RFC 7231 allows relative URIs (current RFC 9110 also allows relative URIs).

Expected behavior should be that LoginUrlAuthenticationEntryPoint would override relative path to absolute only in cases where it's necessary. That is when forceHttps is set to true and incoming requests are http.

Change

After this change, LoginUrlAuthenticationEntryPoint will use relative paths if possible (http -> http, https -> https would use relative paths). However if forceHttps is set to true, and incoming request is http, it'll be redirected using absolute path (https://example.com/login).

More complicated flows

When incoming request is http, forceHttps is true, but portResolver is not able to resolve ports correctly: previous behavior was to fall back to redirect to http (using absolute path) and log a warning. After the change, the warning is still logged, but relative path will be used.

Potentially problematic is one corner case - when we have incoming https request, but using a "matching" http port (not https!). Let's say we have 8080 for http and 8443 for https - what should the behavior be for https://example.com:8080/login? Previously app redirected to https://example.com:8443/login - but only in case when a port "matching" http port was used. If different port was used (e.g. 8081), app would redirect to original https://example.com:8081/login. I think it was unwanted side effect. After the change, the special behavior is removed. It does not matter if port "matches" http when accessing via https. Let me know if I should change this.

Tests

As a result of the small change, many asserts in many tests had to be updated. Behavior is covered by tests (e.g. testHttpsOperationFromOriginalHttpUrl testHttpsOperationFromOriginalHttpsUrl, testOperationWhenHttpsRequestsButHttpsPortUnknown in LoginUrlAuthenticationEntryPointTests).
I only updated testHttpsOperationFromOriginalHttpsUrl to cover the corner case flow explicitly.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 10, 2024
@jzheaux jzheaux self-assigned this Nov 23, 2024
@jzheaux jzheaux added in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 23, 2024
@jzheaux jzheaux added this to the 6.5.x milestone Nov 23, 2024
@jzheaux jzheaux modified the milestones: 6.5.x, 6.5.0-M1 Dec 18, 2024
Pistolnik and others added 2 commits December 17, 2024 22:09
This places the new functionality behind a setting so that
we can remain passive until we can change the setting in
the next major release.

Issue spring-projectsgh-7273
@jzheaux jzheaux merged commit 3eeb431 into spring-projects:main Dec 18, 2024
6 checks passed
@jzheaux
Copy link
Contributor

jzheaux commented Dec 18, 2024

Thanks for the PR, @Pistolnik! I added a polish to place this feature behind a setting until the next major release when we can change the defaults. This will allow the code to remain passive.

When we being development for Spring Security 7 next year, I can reapply the changes you made to the test cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Redirect using a relative URL
3 participants