1.34.0 breaks SAML login when PKCE set to Not Required

kristianvld commented 2 years ago

SAML login broken for some applications with update 1.34.0 when PKCE is set to `Not Required`.

Description

After updating to 1.34.0 docker image (fusionauth/fusionauth-app:1.34.0), applications using SAML can no longer log in. Downgrading back town to fusionauth/fusionauth-app:1.33.0 fixes the issue.

Affects versions

1.34.0

Steps to reproduce

Steps to reproduce the behavior:

Have an application configured to use SAML (e.g. Nextcloud with SAML add-on)
Try to log in within the given application
Get redirected to FusionAuth for login
After login, get redirected to application and see some error message stating that you were able to log in.

Expected behavior

Expected to be able to log in normally.

Platform

(Please complete the following information)

Device: macbook, iphone, desktop
OS: [e.g. iOS, macOS, Windows ME]
Browser + version [e.g. chrome, safari plus a version]
Database: postgres:11.9-alpine

Community guidelines

All issues filed in this repository must abide by the FusionAuth community guidelines.

Additional context

If I set PKCE to Required in both 1.33.0 and 1.34.0 I get the error:

{
  "message" : "OAuth return is missing the authorization code and/or the SAML encoded state."
}

If I set it to Not Required in 1.33.0 everything works as expected. If I set it to Not Required in 1.34.0 I get the following console error:

2022-02-22 2:27:25.478 PM ERROR io.fusionauth.app.action.BaseOAuthCallbackAction - Returned Exception
java.lang.NullPointerException: Cannot invoke "String.length()" because "s" is null
    at java.base/java.net.URLEncoder.encode(URLEncoder.java:224)
    at java.base/java.net.URLEncoder.encode(URLEncoder.java:196)
    at com.inversoft.rest.FormDataBodyHandler.lambda$serializeRequest$0(FormDataBodyHandler.java:63)
    at java.base/java.util.HashMap.forEach(HashMap.java:1421)
    at com.inversoft.rest.FormDataBodyHandler.serializeRequest(FormDataBodyHandler.java:57)
    at com.inversoft.rest.FormDataBodyHandler.setHeaders(FormDataBodyHandler.java:49)
    at com.inversoft.rest.RESTClient.go(RESTClient.java:232)
    at io.fusionauth.client.FusionAuthClient.exchangeOAuthCodeForAccessTokenUsingPKCE(FusionAuthClient.java:1600)
    at io.fusionauth.app.action.BaseOAuthCallbackAction.exchangeCodeForToken(BaseOAuthCallbackAction.java:64)
    at io.fusionauth.app.action.samlv2.CallbackAction.get(CallbackAction.java:105)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:568)
    at org.primeframework.mvc.util.ReflectionUtils.invoke(ReflectionUtils.java:414)
    at org.primeframework.mvc.action.DefaultActionInvocationWorkflow.execute(DefaultActionInvocationWorkflow.java:79)
    at org.primeframework.mvc.action.DefaultActionInvocationWorkflow.perform(DefaultActionInvocationWorkflow.java:62)
    at org.primeframework.mvc.workflow.SubWorkflowChain.continueWorkflow(SubWorkflowChain.java:51)
    at org.primeframework.mvc.validation.DefaultValidationWorkflow.perform(DefaultValidationWorkflow.java:47)
    at org.primeframework.mvc.workflow.SubWorkflowChain.continueWorkflow(SubWorkflowChain.java:51)
    at org.primeframework.mvc.security.DefaultSecurityWorkflow.perform(DefaultSecurityWorkflow.java:60)
    at org.primeframework.mvc.workflow.SubWorkflowChain.continueWorkflow(SubWorkflowChain.java:51)
    at org.primeframework.mvc.parameter.DefaultPostParameterWorkflow.perform(DefaultPostParameterWorkflow.java:50)
    at org.primeframework.mvc.workflow.SubWorkflowChain.continueWorkflow(SubWorkflowChain.java:51)
    at org.primeframework.mvc.content.DefaultContentWorkflow.perform(DefaultContentWorkflow.java:52)
    at org.primeframework.mvc.workflow.SubWorkflowChain.continueWorkflow(SubWorkflowChain.java:51)
    at org.primeframework.mvc.parameter.DefaultParameterWorkflow.perform(DefaultParameterWorkflow.java:57)
    at org.primeframework.mvc.workflow.SubWorkflowChain.continueWorkflow(SubWorkflowChain.java:51)
    at org.primeframework.mvc.parameter.DefaultURIParameterWorkflow.perform(DefaultURIParameterWorkflow.java:102)
    at org.primeframework.mvc.workflow.SubWorkflowChain.continueWorkflow(SubWorkflowChain.java:51)
    at org.primeframework.mvc.scope.DefaultScopeRetrievalWorkflow.perform(DefaultScopeRetrievalWorkflow.java:58)
    at org.primeframework.mvc.workflow.SubWorkflowChain.continueWorkflow(SubWorkflowChain.java:51)
    at org.primeframework.mvc.message.DefaultMessageWorkflow.perform(DefaultMessageWorkflow.java:44)
    at org.primeframework.mvc.workflow.SubWorkflowChain.continueWorkflow(SubWorkflowChain.java:51)
    at org.primeframework.mvc.action.DefaultActionMappingWorkflow.perform(DefaultActionMappingWorkflow.java:126)
    at org.primeframework.mvc.workflow.SubWorkflowChain.continueWorkflow(SubWorkflowChain.java:51)
    at org.primeframework.mvc.workflow.StaticResourceWorkflow.perform(StaticResourceWorkflow.java:97)
    at org.primeframework.mvc.workflow.SubWorkflowChain.continueWorkflow(SubWorkflowChain.java:51)
    at org.primeframework.mvc.parameter.RequestBodyWorkflow.perform(RequestBodyWorkflow.java:91)
    at org.primeframework.mvc.workflow.SubWorkflowChain.continueWorkflow(SubWorkflowChain.java:51)
    at org.primeframework.mvc.security.DefaultSavedRequestWorkflow.perform(DefaultSavedRequestWorkflow.java:64)
    at org.primeframework.mvc.workflow.SubWorkflowChain.continueWorkflow(SubWorkflowChain.java:51)
    at io.fusionauth.app.primeframework.CORSFilter.doFilter(CORSFilter.java:262)
    at io.fusionauth.app.primeframework.CORSRequestWorkflow.perform(CORSRequestWorkflow.java:49)
    at org.primeframework.mvc.workflow.SubWorkflowChain.continueWorkflow(SubWorkflowChain.java:51)
    at io.fusionauth.app.primeframework.FusionAuthMVCWorkflow.perform(FusionAuthMVCWorkflow.java:86)
    at org.primeframework.mvc.workflow.DefaultWorkflowChain.continueWorkflow(DefaultWorkflowChain.java:44)
    at org.primeframework.mvc.servlet.FilterWorkflowChain.continueWorkflow(FilterWorkflowChain.java:50)
    at org.primeframework.mvc.servlet.PrimeFilter.doFilter(PrimeFilter.java:78)
    at com.inversoft.maintenance.servlet.MaintenanceModePrimeFilter.doFilter(MaintenanceModePrimeFilter.java:63)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
    at com.inversoft.servlet.UTF8Filter.doFilter(UTF8Filter.java:27)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
    at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:196)
    at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:97)
    at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:542)
    at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:135)
    at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:81)
    at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:78)
    at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:364)
    at org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:624)
    at org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:65)
    at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:831)
    at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1650)
    at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:49)
    at org.apache.tomcat.util.threads.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1191)
    at org.apache.tomcat.util.threads.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:659)
    at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61)
    at java.base/java.lang.Thread.run(Thread.java:833)93)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
    at com.inversoft.servlet.UTF8Filter.doFilter(UTF8Filter.java:27)
    at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
    at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
    at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:196)
    at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:97)
    at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:542)
    at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:135)
    at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:81)
    at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:78)
    at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:364)
    at org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:624)
    at org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:65)
    at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:831)
    at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1650)
    at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:49)
    at org.apache.tomcat.util.threads.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1191)
    at org.apache.tomcat.util.threads.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:659)
    at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61)
    at java.base/java.lang.Thread.run(Thread.java:833)

robotdan commented 2 years ago

Will take a look. If a regression, it is related to:

https://github.com/FusionAuth/fusionauth-issues/issues/1598

robotdan commented 2 years ago

Thanks for all of the great debug and context @kristianvld !!

If I set PKCE to Required in both 1.33.0 and 1.34.0 I get the error:
{
  "message" : "OAuth return is missing the authorization code and/or the SAML encoded state."
}

Yes, that is what we were fixing in 1.34.0, there were a few use cases where we were not sending in PKCE when calling back to our selves in sub oauth flows such as this.

We'll have a patch out shortly, thanks for brining it to our attention.

kristianvld commented 2 years ago

Thank you guys for your swift action and handling of the problem. Very much appreciated 👍

FusionAuth / fusionauth-issues