Bug 65443

Summary: Allow subclassing CorsFilter
Product: Tomcat 9 Reporter: ekkelenkamp
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: enhancement    
Priority: P2    
Version: unspecified   
Target Milestone: -----   
Hardware: PC   
OS: All   

Description ekkelenkamp 2021-07-09 18:08:08 UTC
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);
    }
Comment 1 Christopher Schultz 2021-07-09 21:45:50 UTC
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.
Comment 2 ekkelenkamp 2021-07-09 22:27:48 UTC
Just created a pull request: https://github.com/apache/tomcat/pull/432
Comment 3 Konstantin Kolinko 2021-07-10 21:36:58 UTC
(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.
Comment 4 ekkelenkamp 2021-07-12 06:40:15 UTC
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.
Comment 5 Mark Thomas 2021-07-13 07:54:34 UTC
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.
Comment 6 ekkelenkamp 2021-07-13 07:58:55 UTC
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?
Comment 7 ekkelenkamp 2021-07-21 14:35:12 UTC
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);
    }
}
Comment 8 Christopher Schultz 2021-07-22 00:36:54 UTC
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.
Comment 9 Mark Thomas 2021-07-26 15:57:47 UTC
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
Comment 10 ekkelenkamp 2021-07-27 11:54:13 UTC
Thanks for providing the change. I'll use it in my code once available in the new releases.