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

Fix CookieHttpSessionStrategy encodeUrl for multi sessions #562

Closed
wants to merge 4 commits into from
Closed

Conversation

edeandrea
Copy link

@edeandrea edeandrea commented Jun 28, 2016

Fix to https://support.pivotal.io/tickets/30469 & issue #563

I do notice 2 bugs in the spring-session code, both inside the
CookieHttpSessionStrategy class. I have a small sample spring-boot application which shows this issue that I can provide if needed.

The first, using their own sample application, after getting a new
session alias from the HttpSessionManager, it calls
HttpSessionManager.encodeURL and then stores the output into a request
attribute which is later rendered. However, in a Spring MVC
application, a Controller may want to issue a redirect to a
newly-encoded URL, like this:

HttpSessionManager sessionManager = (HttpSessionManager)
request.getAttribute(HttpSessionManager.class.getName());
String newSessionAlias = sessionManager.getNewSessionAlias(request);
String currentSessionAlias =
sessionManager.getCurrentSessionAlias(request);

return String.format("redirect:%s",
sessionManager.encodeURL("/test", newSessionAlias));

The problem here is that Spring MVC will funnel the redirect back
through
CookieHttpSessionStrategy.MultiSessionHttpServletResponse.encodeRedirect
URL, which then looks up the current alias & re-encodes with that
alias, effectively replacing the new alias which the controller added.
The fix would be for the
CookieHttpSessionStrategy.MultiSessionHttpServletResponse to examine
the inputted url for the appropriate parameter & don't re-encode if it
finds it there.

The second issue is that inside CookieHttpSessionStrategy line 346
(return path + "?" + query;) is that if there is no query string (on
the default session alias) then the resulting URL will end in a ? with
nothing after it. In that situation the ? should be removed as well
because the query string is empty.

Fix to https://support.pivotal.io/tickets/30469

I do notice 2 bugs in the spring-session code, both inside the
CookieHttpSessionStrategy class.

The first, using their own sample application, after getting a new
session alias from the HttpSessionManager, it calls
HttpSessionManager.encodeURL and then stores the output into a request
attribute which is later rendered. However, in a Spring MVC
application, a Controller may want to issue a redirect to a
newly-encoded URL, like this:

HttpSessionManager sessionManager = (HttpSessionManager)
request.getAttribute(HttpSessionManager.class.getName());
String newSessionAlias = sessionManager.getNewSessionAlias(request);
String currentSessionAlias =
sessionManager.getCurrentSessionAlias(request);

return String.format("redirect:%s",
sessionManager.encodeURL("/effectiveUser", newSessionAlias));

The problem here is that Spring MVC will funnel the redirect back
through
CookieHttpSessionStrategy.MultiSessionHttpServletResponse.encodeRedirect
URL, which then looks up the current alias & re-encodes with that
alias, effectively replacing the new alias which the controller added.
The fix would be for the
CookieHttpSessionStrategy.MultiSessionHttpServletResponse to examine
the inputted url for the appropriate parameter & don't re-encode if it
finds it there.

The second issue is that inside CookieHttpSessionStrategy line 346
(return path + "?" + query;) is that if there is no query string (on
the default session alias) then the resulting URL will end in a ? with
nothing after it. In that situation the ? should be removed as well
because the query string is empty.
@pivotal-issuemaster
Copy link

@edeandrea Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@edeandrea Thank you for signing the Contributor License Agreement!

@rwinch
Copy link
Member

rwinch commented Sep 7, 2016

Thanks for the PR!

  • Can you please add tests?
  • Spring Session's web module cannot rely on Spring framework. Any chance you can update the PR to no longer use UriComponents?

@rwinch rwinch added the status: waiting-for-feedback We need additional information before we can continue label Sep 7, 2016
@edeandrea
Copy link
Author

I can surely do that. As for the module not relying on Spring Framework - I can certainly do that. My initial reaction that that though is that if the dependency isn't allowed - shouldn't the build fail with a compilation error? The fact that it didn't means that Spring Framework is on the compile classpath (I checked before I made the change as I did not want to introduce a dependency that wasn't already there). If indeed the Spring Framework shouldn't be there, then the build itself should be cleaned up as well so that the dependency isn't there to begin with.

@edeandrea
Copy link
Author

edeandrea commented Sep 8, 2016

I think I noticed another bug as well in CookieHttpSessionStrategy.encodeUrl.

Run this test - it fails...

@Test
public void encodeURLWithSameAlias() {
  String url = String.format("/url?%s=1", CookieHttpSessionStrategy.DEFAULT_SESSION_ALIAS_PARAM_NAME);
  assertThat(this.strategy.encodeURL(url, "1")).isEqualTo(url);
}

It will give you this output:
org.junit.ComparisonFailure: expected:<"/url?_s=1[]"> but was:<"/url?_s=1[&_s=1]">

I will fix this as well in my next PR (since some of my other tests will fail until this is fixed).

2) No longer use UriComponents
3) Fix bug in CookieHttpSessionStrategy.encodeURL
@rwinch rwinch self-assigned this Sep 12, 2016
@rwinch rwinch added type: bug A general bug in: core and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 12, 2016
@rwinch
Copy link
Member

rwinch commented Sep 12, 2016

Thanks for the updates! This is merged into master via bff0f8f

@rwinch rwinch closed this Sep 12, 2016
@rwinch rwinch changed the title Fix issue when using SpringMVC to issue redirect in controller Fix CookieHttpSessionStrategy encodeUrl for multi sessions Sep 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants