I would like to subclass the the CorsFilter to be able to provide a custom list of allowed origins, instead of configure them from the init parameters. Unfortunately the method isOriginAllowed uses the instance variable allowedOrigns directly. This makes it impossible to override the available function: getAllowedOrigins and isAnyOriginAllowed. So i would propose the change the isOriginAllowed function from the CorsFilter class as follows: /** * Checks if the Origin is allowed to make a CORS request. * * @param origin * The Origin. * @return <code>true</code> if origin is allowed; <code>false</code> * otherwise. */ private boolean isOriginAllowed(final String origin) { if (isAnyOriginAllowed()) { return true; } // If 'Origin' header is a case-sensitive match of any of allowed // origins, then return true, else return false. return getAllowedOrigins().contains(origin); }
Sounds like a perfect opportunity for you to supply a patch (attached here) or a pull-request via GitHub. I would support its inclusion into Tomcat.
Just created a pull request: https://github.com/apache/tomcat/pull/432
(In reply to ekkelenkamp from comment #2) > Just created a pull request: https://github.com/apache/tomcat/pull/432 I am -0. Wouldn't it be better to provide configuration methods for those settings (e.g. setAllowedOrigins(boolean, Collection<String>) instead of overriding getter methods? BTW, the map returned by `getAllowedOrigins()` is mutable, though its mutations are not thread-safe.
Indeed I would prefer if there is a configuration method available for those settings. I just assumed the state was immutable. The setAllowedOrigins would have to set the anyOriginAllowed variable as well. Changing the mutable Map that currently is returned from the getAllowedOrigins will not do, since the anyOriginAllowed variable needs to be updated as well in case a wildcard domain is set.
The initial implementation was clearly on the basis that the state was set during init() and then never changed but if you have a requirement to change it then I don't see a good reason not to refactor the code and provide setters. I'll note that other built-in filters have getters and setters.
Thanks for the feedback. The requirement is indeed to be able to configure the allowed origins at runtime. Should I provide a pull request for this enhancement or is this kind of refactoring done by the core developers?
Just to let you know, I found a convenient way to get the job done without changing the code. Simply extending the getInitParameter method, does the job. As a reference, this is the pseudo setup of my code: /** * extended to allow configuration at runtime. */ public final class MyCorsFilter extends CorsFilter { private static String getCorsAllowedOrigins() { return config.getCorsAllowedOrigins(); // get config from database } @Override public String getInitParameter(String name) { if (name.equals("cors.allowed.origins")) { return getCorsAllowedOrigins(); } return super.getInitParameter(name); } }
This violates the servlet spec, but hey if you want to do that in your code, it's fine. Honestly, I was hoping you'd end up writing a proper patch because I think this is a good idea to have in the base class.
I've made the CorsFilter class sufficiently extensible to meet this requirement. I opted not to provide setters as that opens up additional concurrency issues. Fixed in: - 10.1.x for 10.1.0-M3 onwards - 10.0.x for 10.0.9 onwards - 9.0.x for 9.0.51 onwards - 8.5.x for 8.5.70 onwards
Thanks for providing the change. I'll use it in my code once available in the new releases.