Bug 60674 - Allow subclassing of org.apache.catalina.filters.CorsFilter
Summary: Allow subclassing of org.apache.catalina.filters.CorsFilter
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 8.5.9
Hardware: All Linux
: P2 normal (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-01 09:52 UTC by Ralf Hauser
Modified: 2017-06-02 07:12 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ralf Hauser 2017-02-01 09:52:09 UTC
1) why is the class "final"?
  ==> pls remove that

Similarly, please make the variables like allowedHttpHeaders "protected" instead of "private final" or is there some "rationale" behind this (coding guidelines mandating this)?
Comment 1 Christopher Schultz 2017-02-01 15:03:45 UTC
(In reply to Ralf Hauser from comment #0)
> 1) why is the class "final"?
>   ==> pls remove that

Interesting: it's been final since the class was first introduced. markt would have to comment on that decision.

My guess is that, since this is a security-related class, it's best to have the configuration as immutable as possible.

> Similarly, please make the variables like allowedHttpHeaders "protected"
> instead of "private final" or is there some "rationale" behind this (coding
> guidelines mandating this)?

See above.

Rather than making the members protected, there should be a protected constructor, and the fields can therefore remain final.
Comment 2 Ralf Hauser 2017-02-12 20:19:51 UTC
Other security classes are not final.
And an attacker would also have to alter the web.xml to have the subclass used at all to begin with.

IMHO, putting such a class as "final" is also against the open source spirit:

"No one after me will be smarter than and adding more value with sub-classing it" or are there other reasonings behind this?
Comment 3 Mark Thomas 2017-02-13 16:58:38 UTC
(In reply to Ralf Hauser from comment #2)
> Other security classes are not final.
> And an attacker would also have to alter the web.xml to have the subclass
> used at all to begin with.
> 
> IMHO, putting such a class as "final" is also against the open source spirit:
> 
> "No one after me will be smarter than and adding more value with
> sub-classing it" or are there other reasonings behind this?

There is no need for such an antagonistic comment.

A review of the history of the file shows that the explanation is as simple as the class was marked as final in the contribution from the original developer.

It is always easier to start off with a more restrictive API and relax it as necessary, than to start with everything open and try and lock it down later.

The fields are private (and final where marked) by design. Getters are provided (which for the collections also allow modification). If you'd like additional getters (or setters) then please make your case. The expectation is that the configuration is set on init and remains unchanged for the lifetime of the Filter. Changing that expectation is not impossible but is likely to be a very invasive change.
Comment 4 Mark Thomas 2017-02-13 17:08:23 UTC
final removed in
- trunk for 9.0.0.M18 onwards
- 8.5.x for 8.5.12 onwards
- 8.0.x for 8.0.42 onwards
- 7.0.x for 7.0.76 onwards
Comment 5 Ralf Hauser 2017-06-02 07:12:04 UTC
Tested in 8.5.14 --> works fine.

Thx for the clarifications that make my comment obsolete - sorry.