Bug 49811 - [PATCH] Disable UrlRewriting
Summary: [PATCH] Disable UrlRewriting
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 6
Classification: Unclassified
Component: Catalina (show other bugs)
Version: unspecified
Hardware: All All
: P2 enhancement (vote)
Target Milestone: default
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
: 50305 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-08-23 14:26 UTC by Wesley
Modified: 2010-11-21 15:50 UTC (History)
1 user (show)



Attachments
Attachment adds a custom attribute to the context interface to disable url encoding/parsing of sessions. (6.13 KB, application/octet-stream)
2010-08-23 14:26 UTC, Wesley
Details
Version 2 disableURLRewrite (8.17 KB, patch)
2010-08-30 15:35 UTC, Wesley
Details | Diff
V3 with tabs fixed and parameter name on Context interface changed. (8.25 KB, patch)
2010-09-11 17:54 UTC, Wesley
Details | Diff
Updated patch (7.31 KB, patch)
2010-10-07 06:56 UTC, Mark Thomas
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wesley 2010-08-23 14:26:28 UTC
Created attachment 25930 [details]
Attachment adds a custom attribute to the context interface to disable url encoding/parsing of sessions.

Encoding sessions in URLS is generally thought of as bad practice. Information on sessions can be passed out to third parties by the Referrer http header.  

It can also be problematic if a user of the application attempts to send out a link to their friends unwittingly passing their session information.

The original need for this patch was raised in http://marc.info/?t=128208259900001&r=1&w=2 on the users mailing list.

The attached patch allows users to enter an attribute on the Context to disable session url encoding and parsing, the attribute is allowURLSessions.  

I attempted to change the documentation too but couldn't create a patch file from it, I think its in svn.ignore.


I've tested locally by disabling session cookies for localhost and ensuring sessions were lost when the attribute was set to false.

I've checked the URL's to ensure jsessionid doesn't appear in them.

I've also checked that sessions where retained when this attribute was set to true or absent.

Finally I've tested when the attribute is set to false and cookies are enabled to ensure sessions work in this senario.

**NOTE** If this is set to false and cookies are denied no session information is retained.
Comment 1 Wesley 2010-08-23 14:34:39 UTC
I don't know what importance to set so I've left at default.

I've two simple JSP files to test this if wanted.
Comment 2 Wesley 2010-08-25 16:00:32 UTC
possible duplicate of #40222 ?
Comment 3 Christopher Schultz 2010-08-26 17:48:19 UTC
Unlikely duplicate of bug #40222: that one has to do with session id switching during authentication.

Some comments on the patch:

I would recommend calling this configuration attribute/parameter "disableURLRewriting" instead of "allowURLSessions". First, it includes the word "rewriting" which is what the servlet spec calls this, and second, it indicates that the default is that URL rewriting is ENABLED.

This, of course, requires that you invert the logic of the entire patch :)

Please mention in the Javadoc that by setting this config parameter to TRUE (that is, disabling URL rewriting), you are breaking the servlet specification (mention chapter and verse, just to be clear). It may even be worth writing to the log file during Context startup.

Also, spell-check your javadoc ;)

You should probably also change the URL-parsing code that accepts jsessionid parameters and have it ignore URL-supplied jsessionids, otherwise you aren't really preventing session hijacking... you're just limiting the damage stupidity can cause.

Other than that, it looks good to me.
Comment 4 Wesley 2010-08-26 18:19:22 UTC
(In reply to comment #3)
> Unlikely duplicate of bug #40222: that one has to do with session id switching
> during authentication.
> 

Good I didn't think so but it did seem to conflict with the inital bug report.


> I would recommend calling this configuration attribute/parameter
> "disableURLRewriting" instead of "allowURLSessions". First, it includes the
> word "rewriting" which is what the servlet spec calls this, and second, it
> indicates that the default is that URL rewriting is ENABLED.

100% agreed. I thought of this afterwards. Initally I thought it may clarify for people who don't read the entire docs to mention specifically what the effect was though.


> Please mention in the Javadoc that by setting this config parameter to TRUE
> (that is, disabling URL rewriting), you are breaking the servlet specification
> (mention chapter and verse, just to be clear). It may even be worth writing to
> the log file during Context startup.

Javadoc agreed. I'll need to find the area of the document.

log message. I'm not sure when I put in debugging logs to the setter it was being called multiple times. I don't know where I would put it exactly.


> 
> Also, spell-check your javadoc ;)

Owch. Yeah I'm a bit dyslexic. 100% agree again though.

> 
> You should probably also change the URL-parsing code that accepts jsessionid
> parameters and have it ignore URL-supplied jsessionids, otherwise you aren't
> really preventing session hijacking... you're just limiting the damage
> stupidity can cause.

Okay thats fair enough but I thought that I had :(

Specifically I thought the changes to CoyoteAdapter.java covered this.

> Other than that, it looks good to me.

Thanks this is helpful.
Comment 5 Christopher Schultz 2010-08-27 09:47:16 UTC
(In reply to comment #4)
> > You should probably also change the URL-parsing code that accepts jsessionid
> > parameters and have it ignore URL-supplied jsessionids, otherwise you aren't
> > really preventing session hijacking... you're just limiting the damage
> > stupidity can cause.
> 
> Okay thats fair enough but I thought that I had :(
> 
> Specifically I thought the changes to CoyoteAdapter.java covered this.

Oops, I think I must have missed that. I haven't reviewed the other code in those classes -- just your patch file. If you've found all the places where "session" and/or "id" (or even ";") exist in those files, then you should be good.
Comment 6 Mark Thomas 2010-08-29 11:18:42 UTC
Overall this looks pretty good. Just some minor clean-up required before it is ready to be proposed for 6.0.x

(In reply to comment #3)
> I would recommend calling this configuration attribute/parameter
> "disableURLRewriting" instead of "allowURLSessions". First, it includes the
> word "rewriting" which is what the servlet spec calls this, and second, it
> indicates that the default is that URL rewriting is ENABLED.
+1

> Please mention in the Javadoc that by setting this config parameter to TRUE
> (that is, disabling URL rewriting), you are breaking the servlet specification
> (mention chapter and verse, just to be clear).
+1

> It may even be worth writing to the log file during Context startup.
-0

> Also, spell-check your javadoc ;)
+1

> You should probably also change the URL-parsing code that accepts jsessionid
> parameters and have it ignore URL-supplied jsessionids,
The proposed changes look sufficient to me.
Comment 7 Wesley 2010-08-30 06:30:03 UTC
(In reply to comment #6)

> > You should probably also change the URL-parsing code that accepts jsessionid
> > parameters and have it ignore URL-supplied jsessionids,
> The proposed changes look sufficient to me.

Actually there is an error in the existing patch where at the point where the details are read out of the context the context hasn't been attached to the request yet. 

Basically the null check that I did is always entered currently and the parsing always defaults to reading the sessionID.

I'm working on a new patch once I figure out a reliable way to retrieve the context at this point.
Comment 8 Wesley 2010-08-30 15:35:12 UTC
Created attachment 25961 [details]
Version 2 disableURLRewrite

Patch v2 all suggestions implemented except for startup.  

Spell checked javadoc.
renamed attribute disableURLRewrite
fixed logical error parsing session id.

I'm not a great fan of the way I've disabled parsing but I deemed it necessary as there would have been side effects from the previous method even if the context hadn't been null.
Comment 9 Wesley 2010-09-09 13:57:54 UTC
>It was a good initiative and I'm sure we will have a look at the patch. Please be patient though. If you don't see any progress (comments in the Bugzilla issue), then it is fine to nag after about one or two weeks.

Em: Nag.

Seriously though I'm not 100% happy with this but there didn't seem to be a cheap way of getting the context reference at the point of URL parsing. Delaying the url parsing had side effects (for contexts which didn't disable urlRewriting).

So does this seem like a reasonable way to do it?
Comment 10 Mark Thomas 2010-09-11 13:13:15 UTC
Functionally, the patch looks good to me and I'd propose it for Tomcat if it wasn't for some minor cosmetic issues:
- use 4 spaces rather than tabs
- setDisableURLRewriting(boolean allow) is confusing. setDisableURLRewriting(boolean disable) would be better

Fix those and I'll propose it for 6.0.x
Comment 11 Wesley 2010-09-11 13:27:48 UTC
(In reply to comment #10)

> - use 4 spaces rather than tabs

Em do you have a style guide? If theres any code formatters I can use for eclipse that would be great. Then I can just use the format shortcut.

> - setDisableURLRewriting(boolean allow) is confusing.
> setDisableURLRewriting(boolean disable) would be better
> 
+1
Comment 12 Mark Thomas 2010-09-11 13:39:50 UTC
(In reply to comment #11)
> Em do you have a style guide? If theres any code formatters I can use for
> eclipse that would be great. Then I can just use the format shortcut.

'fraid not. In short: no tabs, '{' at the end of lines, 80 char line length, spacing to hard to explain - as per existing files.

The tabs were the only issue I saw in your patch.

If you follow the style of the file you are editing you should be fine.
Comment 13 Wesley 2010-09-11 17:54:00 UTC
Created attachment 26018 [details]
V3 with tabs fixed and parameter name on Context interface changed.

V3 of the patch. Hopefully the last one.

Doing a scan of the source reveals several java files that already begin with a tab.

Searching for ^\t
Comment 14 Wesley 2010-09-27 13:57:52 UTC
(In reply to comment #13)
> Created an attachment (id=26018) [details]
> V3 with tabs fixed and parameter name on Context interface changed.
> 
> V3 of the patch. Hopefully the last one.
> 
> Doing a scan of the source reveals several java files that already begin with a
> tab.
> 
> Searching for ^\t

Its been two weeks. I guess I should stop nudging this, Its not a critical issue, but if you want any more changes let me know.
Comment 15 Mark Thomas 2010-10-07 06:56:09 UTC
Created attachment 26135 [details]
Updated patch

I have reviewed the patch and made a couple of changes. The updated patch is attached. I'll propose this updated patch for 6.0x. shortly.

The changes include: tabs -> 4 spaces, corrected mbean descriptor (also place in alphabetical order), shortened very long lines, "specifications" -> "specification" in the JavaDoc, in-lined a couple of methods where I felt it aided clarity.
Comment 16 Wesley 2010-10-10 19:28:52 UTC
(In reply to comment #15)
> Created an attachment (id=26135) [details]
> Updated patch
> 
> I have reviewed the patch and made a couple of changes. The updated patch is
> attached. I'll propose this updated patch for 6.0x. shortly.
> 
> The changes include: tabs -> 4 spaces, corrected mbean descriptor (also place
> in alphabetical order), shortened very long lines, "specifications" ->
> "specification" in the JavaDoc, in-lined a couple of methods where I felt it
> aided clarity.

Thanks Mark,

Thought I'd got all the spaces.
Comment 17 Mark Thomas 2010-10-25 13:01:38 UTC
This has been fixed in 6.0.x and will be included in 6.0.30 onwards.
Comment 18 Sylvain Laurent 2010-11-20 14:06:39 UTC
*** Bug 50305 has been marked as a duplicate of this bug. ***
Comment 19 Sylvain Laurent 2010-11-20 14:08:30 UTC
Why not porting this feature to tc7 ? even if servlet 3 apps can do it through web.xml, we should think of servlet 2 applications that are deployed in tc7...
Comment 20 Mark Thomas 2010-11-21 15:50:54 UTC
(In reply to comment #19)
> Why not porting this feature to tc7 ?
Servlet 3.0 already supports it.

> even if servlet 3 apps can do it through web.xml, we should think of servlet 2 applications that are deployed in tc7...

They can upgrade web.xml to Servlet 3.0 and add the session configuration.

There is already enough code around session parsing for Servlet 3.0 that there was a deliberate decision for Tomcat 7 to remove some of the configuration options that were available in Tomcat 6. The aim was to reduce code and configuration complexity. It would need a very convincing argument (and I haven't seen one yet) to convince me it was worth adding this to Tomcat 7.