Bug 55046 - CORS Filter
Summary: CORS Filter
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 8.0.x-trunk
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-02 00:48 UTC by Mohit Soni
Modified: 2013-06-05 10:01 UTC (History)
1 user (show)



Attachments
A patch file containing CORS filter source code, units tests, and changes to tomcat documentation's filter.xml. (148.39 KB, patch)
2013-06-02 00:48 UTC, Mohit Soni
Details | Diff
CORS Filter's request processing flowchart. (84.53 KB, image/png)
2013-06-02 00:53 UTC, Mohit Soni
Details
Revised patch to address several comments. (161.94 KB, patch)
2013-06-04 05:41 UTC, Mohit Soni
Details | Diff
Updated documentation filter.xml. (7.25 KB, patch)
2013-06-05 01:54 UTC, Mohit Soni
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mohit Soni 2013-06-02 00:48:49 UTC
Created attachment 30363 [details]
A patch file containing CORS filter source code, units tests, and changes to tomcat documentation's filter.xml.

CORS Filter implementation

This is an enhancement request to include CORS Filter as one of Tomcat's container provided filters.

CORS (Cross Origin Resource Sharing) is a W3C specification (http://www.w3.org/TR/cors/) that defines a mechanism to enable cross origin requests. This is a Java Servlet Filter implementation of server-side CORS.

Here are few reasons that makes this implementation is a good fit:
* Implements all required sections of the spec for servers. Handles simple/actual and pre-flight requests as per the specification.
* Written specifically to fit well with Tomcat's source, and is formatted the same as Tomcat's source.
* Filter implementation is just one class and is well Javadoc'd
* Includes ample unit tests to validate the implementation against the spec
* Simple to configure minimally and use
* Easy to override default configuration, if required
* Protects against CRLF injection / response splitting attacks.

We(eBay) would like to contribute this implementation to Apache Software
Foundation, to be included in Tomcat 8. And, I would also like to maintain and develop it, going forward. 

It's true that as a Servlet Filter, any webapp developer may add it to their app whether Tomcat includes it or not, but we believe that this is fundamental server behavior that should be present and easy to enable in the server, as it is in some other servers.
Comment 1 Mohit Soni 2013-06-02 00:53:15 UTC
Created attachment 30364 [details]
CORS Filter's request processing flowchart.

Please include this flowchart image into webapps/docs/images/ folder. I have linked this from CORS documentation section in filter.xml.
Comment 2 Mohit Soni 2013-06-02 00:54:00 UTC
Comment on attachment 30363 [details]
A patch file containing CORS filter source code, units tests, and changes to tomcat documentation's filter.xml.

Marked attachment as a patch.
Comment 3 Mark Thomas 2013-06-03 22:26:32 UTC
I've started to merge the patch into Tomcat trunk (with the assumption it will be back-ported to 7.0.x). So far I have the following comments:
- ALv2 license headers are missing
- No i18n (use of StringManager)
- Trailing whitespace
- No use of <> (Tomcat 8 is Java 7 minimum)
- Javadoc warnings
- Generics warnings
- Autoboxing warnings
- In Tomcat unit tests mock objects are named TesterXxx
- There is already a mock HttpServletRequest that could have been re-used
- Missing @Override

The issues are all minor but this is a large patch so there are a fair few of them.

In addition to expecting a clean build (with Checkstyle enabled) we also provide IDE settings for Eclipse that can help with aligning patches with the coding standards (such as there are) for Tomcat.

I'll continue working on this tomorrow.
Comment 4 Mohit Soni 2013-06-04 05:41:12 UTC
Created attachment 30384 [details]
Revised patch to address several comments.

Thanks Mark, it's great to know that a back-port for 7.0.x is being considered.

I have provided a new patch with:
+ ALv2 license headers
+ Removed trailing whitespace (ran ant -Dexecute.validate=true validate, to verify)
+ Updated test code to use Generics.
+ Removed Generics warnings
+ Renamed mock object to follow TesterXxx pattern.
+ Added missing @Override for interface implementations.
+ Fixed Autoboxing warnings
+ Fixed Javadoc warnings

Still pending:
- No i18n (use of StringManager)
  Saw couple of examples from other tomcat classes. Should CORSFilter extends from FilterBase to use the StringManager ? 
- There is already a mock HttpServletRequest that could have been re-used.
  Not sure, which one you are referring to. Found couple of implementations. But, agree that a duplicate shouldn't be there. Can you recommend one that can be used ?
Comment 5 Mark Thomas 2013-06-04 10:53:58 UTC
Thanks for the updated patch but as I has already started work on the previous patch I have continued with that. The feedback was intended for future patches rather than a list of changes required for this patch.
Additional notes from this morning's work:
- Use of ServletContext.log() has been removed
- Visibility, particularly of methods, has been reduced
- line length in docs
- s/Ex/Eg/

The patch has been applied to trunk.

I'll wait on back-porting it to 7.0.x to give you a chance to review the applied patch. I'd like to include this in 7.0.41 which will be tagged in the next day or so the sooner you can review the applied patch the better.
Comment 6 Mohit Soni 2013-06-05 01:54:06 UTC
Created attachment 30395 [details]
Updated documentation filter.xml.

I have reviewed the last patch, it looks fine to me functionality wise. 

I have updated the filter.xml to reflect change in class name from CORSFilter to CorsFilter, and have made other minor changes to documentation.

Really excited to know that it's getting included in 7.0.41 as well.
Comment 7 Mark Thomas 2013-06-05 10:01:34 UTC
The updated docs has been applied to trunk and the CORS filter changes have been back-ported to 7.0.x and will be included in 7.0.41 onwards.

Thanks for the patch.