AwsHttpServletRequest throws UnsupportedOperationException in getRequestedSessionId instead of returning null

[kdhunter]

• Framework version: 0.9
• Implementations: Spring Boot

Using aws-serverless-java-container-spring 0.9 and Spring Boot 1.5.9.RELEASE, which pulls in Spring Security 4.2.3.RELEASE.

The current implementation of com.amazonaws.serverless.proxy.internal.servlet.AwsHttpServletRequest throws an UnsupportedOperationException when the getRequestedSessionId() method is called.

The default Spring Boot configuration includes the Spring Security SessionManagementFilter in the filter chain. The problem is that the SessionManagementFilter checks the requested session ID under some scenarios:

From org.springframework.security.web.session.SessionManagementFilter, starting at line 118 in doFilter:

			else {
				// No security context or authentication present. Check for a session
				// timeout
				if (request.getRequestedSessionId() != null
						&& !request.isRequestedSessionIdValid()) {
					if (logger.isDebugEnabled()) {
						logger.debug("Requested session ID "
								+ request.getRequestedSessionId() + " is invalid.");
					}

					if (invalidSessionStrategy != null) {
						invalidSessionStrategy
								.onInvalidSessionDetected(request, response);
						return;
					}
				}

Thus, the call to request.getRequestedSessionId() throws and the exception then propagates through the system, destroying the request.

If getRequestedSessionId() returned null, instead of throwing an exception, this problem wouldn't occur.

A null return value from getRequestedSessionId() indicates that the user did not specify a session ID. This would be consistent with the fact that isRequestedSessionIdValid() returns false, and in conformance with the HttpServletRequest specification.

[kdhunter]

Neglected to mention that it appears that you can work around this by completely disabling session management:

@EnableWebSecurity  
public class TestSiteSecurity extends WebSecurityConfigurerAdapter { 
    @Override  
    protected void configure(HttpSecurity http) throws Exception {  
        http.sessionManagement().disable();
     } 
}

which prevents the SessionManagmentFilter from being added to the chain. Our preferred configuration was

http.sessionManagement().sessionCreationPolicy(SessionCreationPolicy.STATELESS);

which tells Spring never to create a session. However this configuration keeps the SessionManagementFilter in the Spring filter chain, with the effect noted above.

[sapessi]

Thanks for the feedback @kdhunter. Throwing that exception was a deliberate choice. Writing applications with Lambda, we expect you not to rely on a session - your code needs to be completely stateless. I will definitely include your snippet on how to disable session checking in the Spring Boot docs.

If this is something you want to leverage, can you tell me a little bit more about the use-case and what you are trying to achieve? I'll see if there's anything we can do.

[kdhunter]

I completely agree that the code needs to be stateless. However, “getRequestedSessionId” does not create a session. To enforce statelessness, the appropriate place to throw an exception would be in getSession - a call to getSession() or getSession(true) should throw. My concern is that there is a lot of common software that may check for the existence of a session (via any number of means), but may be perfectly happy operating without one - this was just such a case. So it seems to me that any of the methods that create a session should throw, but none of the ones that merely check for its existence should.

…

On Jan 30, 2018, at 1:32 PM, Stefano Buliani ***@***.***> wrote: Thanks for the feedback @kdhunter <https://github.com/kdhunter>. Throwing that exception was a deliberate choice. Writing applications with Lambda, we expect you not to rely on a session - your code needs to be completely stateless. I will definitely include your snippet on how to disable session checking in the Spring Boot docs. If this is something you want to leverage, can you tell me a little bit more about the use-case and what you are trying to achieve? I'll see if there's anything we can do. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#111 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAc4_9uG2V3ZbkTMCdGYjY1CbDguaRTnks5tP2BDgaJpZM4RyfrY>.

[sapessi]

@kdhunter fair point. Would returning null from the getSessionId() call work here? I can make the change today and push it out in the 0.9.1 release.

[sapessi]

I've just tested returning from the getRequestSessionId() method, the security framework then moves on to requesting the session - which would throw the exception. If I return a non-null value, it proceeds trying to cache the session in an internal class. I'm not sure there's a clean and safe way around this. I'm no spring expert so open to suggestions. Given the above, I'd rather update the documentation to reflect the required security configuration for spring and not change the framework (throw the UnsupportedOperationException everywhere).

[kdhunter]

In my particular case, things were blowing up at the point in the code that I posted - I didn’t get any farther, so it’s very possible that I could have run into trouble later even if getRequestSessionId had returned null. I had configured my application to be “stateless" - I had assumed (possibly incorrectly) that nothing would then attempt to create a session.

@Override
  protected void configure(HttpSecurity http) throws Exception {
        http
          .sessionManagement().sessionCreationPolicy(SessionCreationPolicy.STATELESS);
  }
}

Possibly that's incorrect without the additional configuration - as I said, I didn't get farther than the code I posted earlier.

I absolutely agree that including explicit documentation as to how you need to set up Spring Security would be a tremendous help for others that might run into similar problems.

All that being said, I still kind of think that having getRequestSessionId return null and have the exception being thrown from getSession() or getSession(true) feels “more correct." In my opinion, it would better draw attention to exactly what it is that you were trying to prevent (the creation of an HttpSession when sessions aren't supported). Throwing an UnsupportedOperationException from getSession that says "sessions aren't supported in this environment" would (to me at least) pretty explicitly explain that. It would also better draw attention to the offending portion of whatever code was triggering the attempted session creation so that people would have a better idea how to work around it. With the current implementation, I was forced to kind of infer your intent.

Just my $0.02.