fix(auth): keep the OAuth return target through the provider callback

OAuth2AuthorizationRequestRedirectFilter invokes the resolver on every
request in the chain and the delegate answers null for anything that is
not an authorization request. Recording the return target on those calls
cleared it again on the next request without a returnTo parameter — the
provider callback included, which this filter processes before login
succeeds. The success handler therefore always found an empty session
attribute and fell back to the default target, so returnTo never worked.

Guard the write on a non-null authorization request. As a side effect,
anonymous API requests no longer allocate a session via getSession().

Signed-off-by: Gal Eyal <gal.e@popai.health>
This commit is contained in:
Gal Eyal 2026-07-29 12:42:57 +03:00
parent e9cd8322a0
commit 35c080b65c
2 changed files with 35 additions and 5 deletions

View file

@ -28,15 +28,26 @@ public class SkillHubOAuth2AuthorizationRequestResolver
@Override
public OAuth2AuthorizationRequest resolve(HttpServletRequest request) {
OAuth2AuthorizationRequest authorizationRequest = delegate.resolve(request);
oauthLoginFlowService.rememberReturnTo(request);
return authorizationRequest;
return rememberIfAuthorizationRequest(request, delegate.resolve(request));
}
@Override
public OAuth2AuthorizationRequest resolve(HttpServletRequest request, String clientRegistrationId) {
OAuth2AuthorizationRequest authorizationRequest = delegate.resolve(request, clientRegistrationId);
oauthLoginFlowService.rememberReturnTo(request);
return rememberIfAuthorizationRequest(request, delegate.resolve(request, clientRegistrationId));
}
/**
* {@code OAuth2AuthorizationRequestRedirectFilter} calls the resolver on every request in the
* chain, not only on authorization requests; the delegate simply answers null for the rest.
* Recording the return target on those calls would clear it again on the very next request —
* including the provider callback, which carries no {@code returnTo} and is processed by this
* filter before authentication succeeds. Only an actual authorization request may touch it.
*/
private OAuth2AuthorizationRequest rememberIfAuthorizationRequest(
HttpServletRequest request, OAuth2AuthorizationRequest authorizationRequest) {
if (authorizationRequest != null) {
oauthLoginFlowService.rememberReturnTo(request);
}
return authorizationRequest;
}
}

View file

@ -54,6 +54,25 @@ class OAuth2AuthorizationRequestResolverTest {
.isEqualTo("/dashboard/publish?draft=1");
}
@Test
void resolve_keepsReturnToOnNonAuthorizationRequests() {
// The redirect filter runs the resolver on every request in the chain, the provider
// callback included. That request carries no returnTo, so treating it as an
// authorization request would clear the target before the success handler reads it.
MockHttpServletRequest authorization = new MockHttpServletRequest("GET", "/oauth2/authorization/github");
authorization.setParameter("returnTo", "/device");
resolver.resolve(authorization, "github");
HttpSession session = authorization.getSession(false);
MockHttpServletRequest callback = new MockHttpServletRequest("GET", "/login/oauth2/code/github");
callback.setParameter("code", "auth-code");
callback.setSession(session);
assertThat(resolver.resolve(callback)).isNull();
assertThat(session.getAttribute(OAuthLoginRedirectSupport.SESSION_RETURN_TO_ATTRIBUTE))
.isEqualTo("/device");
}
@Test
void resolve_ignoresUnsafeReturnTo() {
MockHttpServletRequest request = new MockHttpServletRequest("GET", "/oauth2/authorization/github");