Skip to content

Clarify behaviour of enableSessionUrlRewriting #7644

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Clarify behaviour of enableSessionUrlRewriting #7644

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions ...ringframework/security/config/annotation/web/configurers/SessionManagementConfigurer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,9 @@ public SessionManagementConfigurer<H> sessionAuthenticationFailureHandler(
/**
* If set to true, allows HTTP sessions to be rewritten in the URLs when using
* {@link HttpServletResponse#encodeRedirectURL(String)} or
* {@link HttpServletResponse#encodeURL(String)}, otherwise disallows HTTP sessions to
* be included in the URL. This prevents leaking information to external domains.
* {@link HttpServletResponse#encodeURL(String)}, otherwise disallows all URL
* rewriting, including resource chain functionality.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two questions here:

  • Why is it necessary to list "resource chain functionality" directly?

  • Since the first part of this JavaDoc says "if set to true, allows HTTP sessions to be ....", then it makes sense to reiterate this in the converse, so I'd suggest:

Suggested change
* rewriting, including resource chain functionality.
* rewriting, including disallowing HTTP sessions from being included in the URL.

Copy link
Contributor Author

@OrangeDog OrangeDog Nov 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well this is why I suggested the developers clarify it...

The surprising behaviour is that this also controls non-Spring Security features. From experience, the resource chain (seemingly) randomly not working in a Spring project is the most common issue.

You could just make "sessions" an aside remark for the whole thing, but then it's more obvious that the method is badly named or there's a bug.

If set to true, allows URLs to be rewritten in response templates. For example, the session key may be added if cookies are unavailable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this method aligns with the servlet JavaDoc, which says for HttpServletResponse#encodeURL:

Encodes the specified URL by including the session ID,
or, if encoding is not needed, returns the URL unchanged.
The implementation of this method includes the logic to
determine whether the session ID needs to be encoded in the URL.
For example, if the browser supports cookies, or session
tracking is turned off, URL encoding is unnecessary.

The way I read this is that implementers of HttpServletResponse should add the jsessionid, encoding it as necessary, but if no jsessionid is to be added, leave the URL unchanged. I agree that HttpServletResponse#encodeURL is a bit of an odd name, given the JavaDoc.

The servlet spec, which refers in 7.1.3 to the function of adding jsessionid as a request parameter as "URL Rewriting".

Because of these, the method enableSessionUrlRewriting seems like a natural name, evoking that it is to make sure jsessionid is not managed as a request parameter.

Maybe what I'm missing is a more detailed description of the problem you are seeing when setting this to true. What are you expecting the Servlet container to do with the URL that gets skipped because HttpServletResponse#encodeURL is bypassed?

Copy link
Contributor Author

@OrangeDog OrangeDog Nov 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is when it's false: #3087
Other things (e.g. Spring's resource chain) use encodeURL to do other stuff.

* This prevents leaking information to external domains.
*
* @param enableSessionUrlRewriting true if should allow the JSESSIONID to be
* rewritten into the URLs, else false (default)
Expand Down