Clarify behaviour of enableSessionUrlRewriting #7644
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
There was a problem hiding this comment.
Thanks, @OrangeDog! I have a couple of questions, which I've left for you inline.
| * {@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. |
There was a problem hiding this comment.
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:
| * rewriting, including resource chain functionality. | |
| * rewriting, including disallowing HTTP sessions from being included in the URL. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
The problem is when it's false: #3087
Other things (e.g. Spring's resource chain) use encodeURL to do other stuff.
jzheaux
added
status: waiting-for-feedback
|
@OrangeDog, I created a sample application that demonstrates resource chaining working by default in conjunction with Spring Security. Given that, I don't believe it's accurate to say that What would you think about something like: * <p>
* This is achieved by guarding {@link HttpServletResponse#encodeURL} and {@link HttpServletResponse#encodeRedirectURL} invocations.
* Any code that also overrides either of these two methods,
* like {@link org.springframework.web.servlet.resource.ResourceUrlEncodingFilter},
* needs to come after the security filter chain or risk being skipped.Since JavaDoc is quite simple to adjust, if I don't hear anything else from you in a week or so, I'll go ahead and change it to read like the above. Either way, don't hesitate to reach out if you find additional refinements. |
See #3087