Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz merged PR #681: URL: https://github.com/apache/tomcat/pull/681 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on PR #681: URL: https://github.com/apache/tomcat/pull/681#issuecomment-1875476100 > > Re 4: I think that if one is wise enough to write a RegExp, they could use "|" to combine several patterns, and do not really need splitting by comma. > > I suppose if you are going to use a regular expression, maybe the entire pattern should be used since regex is very expensive already. After thinking about this for a while, I agree with @kkolinko on this one: a regular expression is going to be expensive enough to run that the user may as well implement the whole matching pattern in a single regular expression instead of having Tomcat chop it up into little pieces and potentially run multiple regular expression matches for every URL-check. I will change the implementation to allow *either* a single regular expression (offset by leading `/` and trailing `/` characters) *or* a series of simpler comma-separated matchers. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
isapir commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1437906912
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+static {
+ArrayList> defaultNoNonceURLPatterns = new
ArrayList<>();
+
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".css"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".js"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".gif"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".png"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".jpg"));
Review Comment:
"I never type `.jpeg` because I can get the same money for `.jpg`" - Mark
Twain
But sure, it doesn't hurt to add it as others do like the longhand (is that
a word? opposite of shorthand) form
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
isapir commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1437905241
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -110,45 +285,70 @@ public void doFilter(ServletRequest request,
ServletResponse response, FilterCha
HttpSession session = req.getSession(false);
+String requestedPath = getRequestedPath(req);
boolean skipNonceCheck = skipNonceCheck(req);
NonceCache nonceCache = null;
if (!skipNonceCheck) {
String previousNonce =
req.getParameter(nonceRequestParameterName);
if (previousNonce == null) {
-if (log.isDebugEnabled()) {
-log.debug("Rejecting request for " +
getRequestedPath(req) + ", session " +
-(null == session ? "(none)" : session.getId())
+
-" with no CSRF nonce found in request");
-}
-
-res.sendError(getDenyStatus());
-return;
-}
+if (enforce(req, requestedPath)) {
+if (log.isDebugEnabled()) {
+log.debug("Rejecting request for " +
getRequestedPath(req) + ", session " +
Review Comment:
I personally think that the code is cleaner and easier to maintain when
repetitive blocks are encapsulated in a function, but do I "really think it
needs it"? Nahh - your code, your decision :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
isapir commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1437903976
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -87,11 +104,170 @@ public void setNonceRequestParameterName(String
parameterName) {
this.nonceRequestParameterName = parameterName;
}
+/**
+ * Sets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @param enforce true to enforce CSRF protections or
+ *false to log DEBUG messages and allow
+ *all requests.
+ */
+public void setEnforce(boolean enforce) {
+this.enforce = enforce;
+}
+
+/**
+ * Gets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @return true if CSRF protections will be enforced or
+ * false if all requests will be allowed and
+ * failures will be logged as DEBUG messages.
+ */
+public boolean getEnforce() {
+return this.enforce;
+}
+
+/**
+ * Sets the list of URL patterns to suppress nonce-addition for.
+ *
+ * Some URLs do not need nonces added to them such as static resources.
+ * By not adding nonces to those URLs, HTTP caches can be more
+ * effective because the CSRF prevention filter won't generate what
+ * look like unique URLs for those commonly-reused resources.
+ *
+ * @param patterns A comma-separated list of URL patterns that will not
+ *have nonces added to them. Patterns may begin or end with a
+ ** character to denote a suffix-match or
+ *prefix-match. Any matched URL will not have a CSRF nonce
+ *added to it when passed through
+ *{@link HttpServletResponse#encodeURL(String)}.
+ */
+public void setNoNonceURLPatterns(String patterns) {
+this.noNoncePatterns = patterns;
+
+if (null != context) {
+this.noNoncePredicates = createNoNoncePredicates(context,
this.noNoncePatterns);
+}
+}
+
+/**
+ * Creates a collection of matchers from a comma-separated string of
patterns.
+ *
+ * @param patterns A comma-separated string of URL matching patterns.
+ *
+ * @return A collection of predicates representing the URL patterns.
+ */
+protected static Collection>
createNoNoncePredicates(ServletContext context, String patterns) {
+if (null == patterns || 0 == patterns.trim().length()) {
Review Comment:
Passionate is a strong word, so I can't say that I'm "passionate" about
using String.isBlank(), but OTOH I do like seeing modern Java in the source
code where applicable, so I'm a +1 on that one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1437901624
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -87,11 +104,170 @@ public void setNonceRequestParameterName(String
parameterName) {
this.nonceRequestParameterName = parameterName;
}
+/**
+ * Sets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @param enforce true to enforce CSRF protections or
+ *false to log DEBUG messages and allow
+ *all requests.
+ */
+public void setEnforce(boolean enforce) {
+this.enforce = enforce;
+}
+
+/**
+ * Gets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @return true if CSRF protections will be enforced or
+ * false if all requests will be allowed and
+ * failures will be logged as DEBUG messages.
+ */
+public boolean getEnforce() {
+return this.enforce;
+}
+
+/**
+ * Sets the list of URL patterns to suppress nonce-addition for.
+ *
+ * Some URLs do not need nonces added to them such as static resources.
+ * By not adding nonces to those URLs, HTTP caches can be more
+ * effective because the CSRF prevention filter won't generate what
+ * look like unique URLs for those commonly-reused resources.
+ *
+ * @param patterns A comma-separated list of URL patterns that will not
+ *have nonces added to them. Patterns may begin or end with a
+ ** character to denote a suffix-match or
+ *prefix-match. Any matched URL will not have a CSRF nonce
+ *added to it when passed through
+ *{@link HttpServletResponse#encodeURL(String)}.
+ */
+public void setNoNonceURLPatterns(String patterns) {
+this.noNoncePatterns = patterns;
+
+if (null != context) {
+this.noNoncePredicates = createNoNoncePredicates(context,
this.noNoncePatterns);
+}
+}
+
+/**
+ * Creates a collection of matchers from a comma-separated string of
patterns.
+ *
+ * @param patterns A comma-separated string of URL matching patterns.
+ *
+ * @return A collection of predicates representing the URL patterns.
+ */
+protected static Collection>
createNoNoncePredicates(ServletContext context, String patterns) {
+if (null == patterns || 0 == patterns.trim().length()) {
+return null;
+}
+
+String values[] = patterns.split(",");
+
+ArrayList> matchers = new ArrayList<>(values.length);
+for (String value : values) {
+Predicate p = createNoNoncePredicate(context,
value.trim());
+
+if (null != p) {
+matchers.add(p);
+}
+}
+
+matchers.trimToSize();
+
+return matchers;
+}
+
+/**
+ * Creates a predicate that can match the specified type of pattern.
+ *
+ * @param pattern The pattern to match e.g. *.foo or
+ */bar/*.
+ *
+ * @return A Predicate which can match the specified pattern, or
+ * >null if the pattern is null or blank.
+ */
+protected static Predicate createNoNoncePredicate(ServletContext
context, String pattern) {
+if (null == pattern || 0 == pattern.trim().length()) {
+return null;
+}
+if (pattern.startsWith("mime:")) {
+return new MimePredicate(context, createNoNoncePredicate(context,
pattern.substring(5)));
+} else if (pattern.startsWith("*")) {
+return new SuffixPredicate(pattern.substring(1));
+} else if (pattern.endsWith("*")) {
+return new PrefixPredicate(pattern.substring(0, pattern.length() -
1));
+} else if (pattern.startsWith("/") && pattern.endsWith("/")) {
+return new PatternPredicate(pattern.substring(1, pattern.length()
- 1));
+} else {
+throw new IllegalArgumentException("Unsupported pattern: " +
pattern);
+}
+}
+
+protected static class MimePredicate implements Predicate {
+private final ServletContext context;
+private final Predicate predicate;
+
+public MimePredicate(ServletContext context, Predicate
predicate) {
+this.context = context;
+this.predicate = predicate;
+}
+
+@Override
+public boolean test(String t) {
+String mimeType = context.getMimeType(t);
+
+return predicate.test(mimeType);
Review Comment:
Is it often that the application will invoke
`HttpServletResponse.encodeURL(null)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail:
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1437900565
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -87,11 +104,170 @@ public void setNonceRequestParameterName(String
parameterName) {
this.nonceRequestParameterName = parameterName;
}
+/**
+ * Sets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @param enforce true to enforce CSRF protections or
+ *false to log DEBUG messages and allow
+ *all requests.
+ */
+public void setEnforce(boolean enforce) {
+this.enforce = enforce;
+}
+
+/**
+ * Gets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @return true if CSRF protections will be enforced or
+ * false if all requests will be allowed and
+ * failures will be logged as DEBUG messages.
+ */
+public boolean getEnforce() {
Review Comment:
There are 308 classes in the Tomcat 11.0.x source tree which contain
`boolean isFoo()` and only 141 which contain `boolean getFoo()`. I didn't
bother checking is any contain both.
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -87,11 +104,170 @@ public void setNonceRequestParameterName(String
parameterName) {
this.nonceRequestParameterName = parameterName;
}
+/**
+ * Sets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @param enforce true to enforce CSRF protections or
+ *false to log DEBUG messages and allow
+ *all requests.
+ */
+public void setEnforce(boolean enforce) {
+this.enforce = enforce;
+}
+
+/**
+ * Gets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @return true if CSRF protections will be enforced or
+ * false if all requests will be allowed and
+ * failures will be logged as DEBUG messages.
+ */
+public boolean getEnforce() {
Review Comment:
There are 308 classes in the Tomcat 11.0.x source tree which contain
`boolean isFoo()` and only 141 which contain `boolean getFoo()`. I didn't
bother checking if any contain both.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1437899157
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -87,11 +104,170 @@ public void setNonceRequestParameterName(String
parameterName) {
this.nonceRequestParameterName = parameterName;
}
+/**
+ * Sets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @param enforce true to enforce CSRF protections or
+ *false to log DEBUG messages and allow
+ *all requests.
+ */
+public void setEnforce(boolean enforce) {
+this.enforce = enforce;
+}
+
+/**
+ * Gets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @return true if CSRF protections will be enforced or
+ * false if all requests will be allowed and
+ * failures will be logged as DEBUG messages.
+ */
+public boolean getEnforce() {
Review Comment:
I think this is a style question. This isn't really expected to be a "java
bean" and doesn't require the `boolean isFoo` and `void setFoo(boolean)`
specifically for boolean members. I'll have a look around to see what's common
and where.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1437898144
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -87,11 +104,170 @@ public void setNonceRequestParameterName(String
parameterName) {
this.nonceRequestParameterName = parameterName;
}
+/**
+ * Sets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @param enforce true to enforce CSRF protections or
+ *false to log DEBUG messages and allow
+ *all requests.
+ */
+public void setEnforce(boolean enforce) {
+this.enforce = enforce;
+}
+
+/**
+ * Gets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @return true if CSRF protections will be enforced or
+ * false if all requests will be allowed and
+ * failures will be logged as DEBUG messages.
+ */
+public boolean getEnforce() {
+return this.enforce;
+}
+
+/**
+ * Sets the list of URL patterns to suppress nonce-addition for.
+ *
+ * Some URLs do not need nonces added to them such as static resources.
+ * By not adding nonces to those URLs, HTTP caches can be more
+ * effective because the CSRF prevention filter won't generate what
+ * look like unique URLs for those commonly-reused resources.
+ *
+ * @param patterns A comma-separated list of URL patterns that will not
+ *have nonces added to them. Patterns may begin or end with a
+ ** character to denote a suffix-match or
+ *prefix-match. Any matched URL will not have a CSRF nonce
+ *added to it when passed through
+ *{@link HttpServletResponse#encodeURL(String)}.
+ */
+public void setNoNonceURLPatterns(String patterns) {
+this.noNoncePatterns = patterns;
+
+if (null != context) {
+this.noNoncePredicates = createNoNoncePredicates(context,
this.noNoncePatterns);
+}
+}
+
+/**
+ * Creates a collection of matchers from a comma-separated string of
patterns.
+ *
+ * @param patterns A comma-separated string of URL matching patterns.
+ *
+ * @return A collection of predicates representing the URL patterns.
+ */
+protected static Collection>
createNoNoncePredicates(ServletContext context, String patterns) {
+if (null == patterns || 0 == patterns.trim().length()) {
Review Comment:
> why is `#isEmpty()`?
I'm not sure I understand this, @michael-o
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1437898048
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -87,11 +104,170 @@ public void setNonceRequestParameterName(String
parameterName) {
this.nonceRequestParameterName = parameterName;
}
+/**
+ * Sets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @param enforce true to enforce CSRF protections or
+ *false to log DEBUG messages and allow
+ *all requests.
+ */
+public void setEnforce(boolean enforce) {
+this.enforce = enforce;
+}
+
+/**
+ * Gets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @return true if CSRF protections will be enforced or
+ * false if all requests will be allowed and
+ * failures will be logged as DEBUG messages.
+ */
+public boolean getEnforce() {
+return this.enforce;
+}
+
+/**
+ * Sets the list of URL patterns to suppress nonce-addition for.
+ *
+ * Some URLs do not need nonces added to them such as static resources.
+ * By not adding nonces to those URLs, HTTP caches can be more
+ * effective because the CSRF prevention filter won't generate what
+ * look like unique URLs for those commonly-reused resources.
+ *
+ * @param patterns A comma-separated list of URL patterns that will not
+ *have nonces added to them. Patterns may begin or end with a
+ ** character to denote a suffix-match or
+ *prefix-match. Any matched URL will not have a CSRF nonce
+ *added to it when passed through
+ *{@link HttpServletResponse#encodeURL(String)}.
+ */
+public void setNoNonceURLPatterns(String patterns) {
+this.noNoncePatterns = patterns;
+
+if (null != context) {
+this.noNoncePredicates = createNoNoncePredicates(context,
this.noNoncePatterns);
+}
+}
+
+/**
+ * Creates a collection of matchers from a comma-separated string of
patterns.
+ *
+ * @param patterns A comma-separated string of URL matching patterns.
+ *
+ * @return A collection of predicates representing the URL patterns.
+ */
+protected static Collection>
createNoNoncePredicates(ServletContext context, String patterns) {
+if (null == patterns || 0 == patterns.trim().length()) {
Review Comment:
@michael-o None of this can be back-ported farther than 10.1.x without
significant changes. The whole `jakarta.*` namespace needs to change,
`Predicate` needs to be defined locally, etc. I'm okay using `String.isBlank`
here if anyone is passionate about it. I generally prefer things to be as close
as possible across the branches, so I would err on the side of using
`null`-check-plus-zero-length-check until we dump Tomcat 8.5.x.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1437897171
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -87,11 +104,170 @@ public void setNonceRequestParameterName(String
parameterName) {
this.nonceRequestParameterName = parameterName;
}
+/**
+ * Sets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @param enforce true to enforce CSRF protections or
+ *false to log DEBUG messages and allow
+ *all requests.
+ */
+public void setEnforce(boolean enforce) {
+this.enforce = enforce;
+}
+
+/**
+ * Gets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @return true if CSRF protections will be enforced or
+ * false if all requests will be allowed and
+ * failures will be logged as DEBUG messages.
+ */
+public boolean getEnforce() {
+return this.enforce;
+}
+
+/**
+ * Sets the list of URL patterns to suppress nonce-addition for.
+ *
+ * Some URLs do not need nonces added to them such as static resources.
+ * By not adding nonces to those URLs, HTTP caches can be more
+ * effective because the CSRF prevention filter won't generate what
+ * look like unique URLs for those commonly-reused resources.
+ *
+ * @param patterns A comma-separated list of URL patterns that will not
+ *have nonces added to them. Patterns may begin or end with a
+ ** character to denote a suffix-match or
+ *prefix-match. Any matched URL will not have a CSRF nonce
+ *added to it when passed through
+ *{@link HttpServletResponse#encodeURL(String)}.
+ */
+public void setNoNonceURLPatterns(String patterns) {
+this.noNoncePatterns = patterns;
+
+if (null != context) {
+this.noNoncePredicates = createNoNoncePredicates(context,
this.noNoncePatterns);
+}
+}
+
+/**
+ * Creates a collection of matchers from a comma-separated string of
patterns.
+ *
+ * @param patterns A comma-separated string of URL matching patterns.
+ *
+ * @return A collection of predicates representing the URL patterns.
+ */
+protected static Collection>
createNoNoncePredicates(ServletContext context, String patterns) {
+if (null == patterns || 0 == patterns.trim().length()) {
+return null;
Review Comment:
I think this is a matter of taste these days. I usually prefer `null` to
empty collections just because the null-check is far faster than creating an
iterator from an empty collection, then iterating zero times over it. All kinds
of control-flow is skipped with a simple null-check.
It does make the code a little cleaner, though. Does anyone else want to
weigh-in?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1437896682
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -110,45 +285,70 @@ public void doFilter(ServletRequest request,
ServletResponse response, FilterCha
HttpSession session = req.getSession(false);
+String requestedPath = getRequestedPath(req);
boolean skipNonceCheck = skipNonceCheck(req);
NonceCache nonceCache = null;
if (!skipNonceCheck) {
String previousNonce =
req.getParameter(nonceRequestParameterName);
if (previousNonce == null) {
-if (log.isDebugEnabled()) {
-log.debug("Rejecting request for " +
getRequestedPath(req) + ", session " +
-(null == session ? "(none)" : session.getId())
+
-" with no CSRF nonce found in request");
-}
-
-res.sendError(getDenyStatus());
-return;
-}
+if (enforce(req, requestedPath)) {
+if (log.isDebugEnabled()) {
+log.debug("Rejecting request for " +
getRequestedPath(req) + ", session " +
Review Comment:
If you really think it needs it. I don't find the control-flow difficult to
follow, here, with the log messages in there. I think further obfuscating the
log messages will just generate more code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
michael-o commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1436107052
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -87,11 +104,170 @@ public void setNonceRequestParameterName(String
parameterName) {
this.nonceRequestParameterName = parameterName;
}
+/**
+ * Sets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @param enforce true to enforce CSRF protections or
+ *false to log DEBUG messages and allow
+ *all requests.
+ */
+public void setEnforce(boolean enforce) {
+this.enforce = enforce;
+}
+
+/**
+ * Gets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @return true if CSRF protections will be enforced or
+ * false if all requests will be allowed and
+ * failures will be logged as DEBUG messages.
+ */
+public boolean getEnforce() {
+return this.enforce;
+}
+
+/**
+ * Sets the list of URL patterns to suppress nonce-addition for.
+ *
+ * Some URLs do not need nonces added to them such as static resources.
+ * By not adding nonces to those URLs, HTTP caches can be more
+ * effective because the CSRF prevention filter won't generate what
+ * look like unique URLs for those commonly-reused resources.
+ *
+ * @param patterns A comma-separated list of URL patterns that will not
+ *have nonces added to them. Patterns may begin or end with a
+ ** character to denote a suffix-match or
+ *prefix-match. Any matched URL will not have a CSRF nonce
+ *added to it when passed through
+ *{@link HttpServletResponse#encodeURL(String)}.
+ */
+public void setNoNonceURLPatterns(String patterns) {
+this.noNoncePatterns = patterns;
+
+if (null != context) {
+this.noNoncePredicates = createNoNoncePredicates(context,
this.noNoncePatterns);
+}
+}
+
+/**
+ * Creates a collection of matchers from a comma-separated string of
patterns.
+ *
+ * @param patterns A comma-separated string of URL matching patterns.
+ *
+ * @return A collection of predicates representing the URL patterns.
+ */
+protected static Collection>
createNoNoncePredicates(ServletContext context, String patterns) {
+if (null == patterns || 0 == patterns.trim().length()) {
Review Comment:
Can't be backported
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
isapir commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1435945699
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -87,11 +104,170 @@ public void setNonceRequestParameterName(String
parameterName) {
this.nonceRequestParameterName = parameterName;
}
+/**
+ * Sets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @param enforce true to enforce CSRF protections or
+ *false to log DEBUG messages and allow
+ *all requests.
+ */
+public void setEnforce(boolean enforce) {
+this.enforce = enforce;
+}
+
+/**
+ * Gets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @return true if CSRF protections will be enforced or
+ * false if all requests will be allowed and
+ * failures will be logged as DEBUG messages.
+ */
+public boolean getEnforce() {
+return this.enforce;
+}
+
+/**
+ * Sets the list of URL patterns to suppress nonce-addition for.
+ *
+ * Some URLs do not need nonces added to them such as static resources.
+ * By not adding nonces to those URLs, HTTP caches can be more
+ * effective because the CSRF prevention filter won't generate what
+ * look like unique URLs for those commonly-reused resources.
+ *
+ * @param patterns A comma-separated list of URL patterns that will not
+ *have nonces added to them. Patterns may begin or end with a
+ ** character to denote a suffix-match or
+ *prefix-match. Any matched URL will not have a CSRF nonce
+ *added to it when passed through
+ *{@link HttpServletResponse#encodeURL(String)}.
+ */
+public void setNoNonceURLPatterns(String patterns) {
+this.noNoncePatterns = patterns;
+
+if (null != context) {
+this.noNoncePredicates = createNoNoncePredicates(context,
this.noNoncePatterns);
+}
+}
+
+/**
+ * Creates a collection of matchers from a comma-separated string of
patterns.
+ *
+ * @param patterns A comma-separated string of URL matching patterns.
+ *
+ * @return A collection of predicates representing the URL patterns.
+ */
+protected static Collection>
createNoNoncePredicates(ServletContext context, String patterns) {
+if (null == patterns || 0 == patterns.trim().length()) {
+return null;
+}
+
+String values[] = patterns.split(",");
+
+ArrayList> matchers = new ArrayList<>(values.length);
+for (String value : values) {
+Predicate p = createNoNoncePredicate(context,
value.trim());
+
+if (null != p) {
+matchers.add(p);
+}
+}
+
+matchers.trimToSize();
+
+return matchers;
+}
+
+/**
+ * Creates a predicate that can match the specified type of pattern.
+ *
+ * @param pattern The pattern to match e.g. *.foo or
+ */bar/*.
+ *
+ * @return A Predicate which can match the specified pattern, or
+ * >null if the pattern is null or blank.
+ */
+protected static Predicate createNoNoncePredicate(ServletContext
context, String pattern) {
+if (null == pattern || 0 == pattern.trim().length()) {
+return null;
+}
+if (pattern.startsWith("mime:")) {
+return new MimePredicate(context, createNoNoncePredicate(context,
pattern.substring(5)));
+} else if (pattern.startsWith("*")) {
+return new SuffixPredicate(pattern.substring(1));
+} else if (pattern.endsWith("*")) {
+return new PrefixPredicate(pattern.substring(0, pattern.length() -
1));
+} else if (pattern.startsWith("/") && pattern.endsWith("/")) {
+return new PatternPredicate(pattern.substring(1, pattern.length()
- 1));
+} else {
+throw new IllegalArgumentException("Unsupported pattern: " +
pattern);
+}
+}
+
+protected static class MimePredicate implements Predicate {
+private final ServletContext context;
+private final Predicate predicate;
+
+public MimePredicate(ServletContext context, Predicate
predicate) {
+this.context = context;
+this.predicate = predicate;
+}
+
+@Override
+public boolean test(String t) {
+String mimeType = context.getMimeType(t);
+
+return predicate.test(mimeType);
+}
+}
+
+protected static class PrefixPredicate implements Predicate {
+private final String prefix;
+public PrefixPredicate(String prefix) {
+this.prefix = prefix;
+}
+
+@Override
+public boolean test(String t) {
+return t.endsWith(this.prefix);
Review
Re: [PR] Csrf filter improvements [tomcat]
isapir commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1435945537
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -110,45 +285,70 @@ public void doFilter(ServletRequest request,
ServletResponse response, FilterCha
HttpSession session = req.getSession(false);
+String requestedPath = getRequestedPath(req);
boolean skipNonceCheck = skipNonceCheck(req);
NonceCache nonceCache = null;
if (!skipNonceCheck) {
String previousNonce =
req.getParameter(nonceRequestParameterName);
if (previousNonce == null) {
-if (log.isDebugEnabled()) {
-log.debug("Rejecting request for " +
getRequestedPath(req) + ", session " +
-(null == session ? "(none)" : session.getId())
+
-" with no CSRF nonce found in request");
-}
-
-res.sendError(getDenyStatus());
-return;
-}
+if (enforce(req, requestedPath)) {
+if (log.isDebugEnabled()) {
+log.debug("Rejecting request for " +
getRequestedPath(req) + ", session " +
Review Comment:
Looks like quite a few repetitions of log.debug blocks with very similar
content. Might be a good opportunity for a private function?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
isapir commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1435943790
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -87,11 +104,170 @@ public void setNonceRequestParameterName(String
parameterName) {
this.nonceRequestParameterName = parameterName;
}
+/**
+ * Sets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @param enforce true to enforce CSRF protections or
+ *false to log DEBUG messages and allow
+ *all requests.
+ */
+public void setEnforce(boolean enforce) {
+this.enforce = enforce;
+}
+
+/**
+ * Gets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @return true if CSRF protections will be enforced or
+ * false if all requests will be allowed and
+ * failures will be logged as DEBUG messages.
+ */
+public boolean getEnforce() {
+return this.enforce;
+}
+
+/**
+ * Sets the list of URL patterns to suppress nonce-addition for.
+ *
+ * Some URLs do not need nonces added to them such as static resources.
+ * By not adding nonces to those URLs, HTTP caches can be more
+ * effective because the CSRF prevention filter won't generate what
+ * look like unique URLs for those commonly-reused resources.
+ *
+ * @param patterns A comma-separated list of URL patterns that will not
+ *have nonces added to them. Patterns may begin or end with a
+ ** character to denote a suffix-match or
+ *prefix-match. Any matched URL will not have a CSRF nonce
+ *added to it when passed through
+ *{@link HttpServletResponse#encodeURL(String)}.
+ */
+public void setNoNonceURLPatterns(String patterns) {
+this.noNoncePatterns = patterns;
+
+if (null != context) {
+this.noNoncePredicates = createNoNoncePredicates(context,
this.noNoncePatterns);
+}
+}
+
+/**
+ * Creates a collection of matchers from a comma-separated string of
patterns.
+ *
+ * @param patterns A comma-separated string of URL matching patterns.
+ *
+ * @return A collection of predicates representing the URL patterns.
+ */
+protected static Collection>
createNoNoncePredicates(ServletContext context, String patterns) {
+if (null == patterns || 0 == patterns.trim().length()) {
+return null;
Review Comment:
What do you think about returning `Collections.emptyList()` instead of null?
Then you can use it in iterators with no issues and avoid having to check for
nulls / NPEs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
isapir commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1435943366
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -87,11 +104,170 @@ public void setNonceRequestParameterName(String
parameterName) {
this.nonceRequestParameterName = parameterName;
}
+/**
+ * Sets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @param enforce true to enforce CSRF protections or
+ *false to log DEBUG messages and allow
+ *all requests.
+ */
+public void setEnforce(boolean enforce) {
+this.enforce = enforce;
+}
+
+/**
+ * Gets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @return true if CSRF protections will be enforced or
+ * false if all requests will be allowed and
+ * failures will be logged as DEBUG messages.
+ */
+public boolean getEnforce() {
+return this.enforce;
+}
+
+/**
+ * Sets the list of URL patterns to suppress nonce-addition for.
+ *
+ * Some URLs do not need nonces added to them such as static resources.
+ * By not adding nonces to those URLs, HTTP caches can be more
+ * effective because the CSRF prevention filter won't generate what
+ * look like unique URLs for those commonly-reused resources.
+ *
+ * @param patterns A comma-separated list of URL patterns that will not
+ *have nonces added to them. Patterns may begin or end with a
+ ** character to denote a suffix-match or
+ *prefix-match. Any matched URL will not have a CSRF nonce
+ *added to it when passed through
+ *{@link HttpServletResponse#encodeURL(String)}.
+ */
+public void setNoNonceURLPatterns(String patterns) {
+this.noNoncePatterns = patterns;
+
+if (null != context) {
+this.noNoncePredicates = createNoNoncePredicates(context,
this.noNoncePatterns);
+}
+}
+
+/**
+ * Creates a collection of matchers from a comma-separated string of
patterns.
+ *
+ * @param patterns A comma-separated string of URL matching patterns.
+ *
+ * @return A collection of predicates representing the URL patterns.
+ */
+protected static Collection>
createNoNoncePredicates(ServletContext context, String patterns) {
+if (null == patterns || 0 == patterns.trim().length()) {
Review Comment:
In Java 11+ you can use `patterns.isBlank()` which will return true if the
string is empty or contains only whitespace
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
michael-o commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1434524088
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -87,11 +104,170 @@ public void setNonceRequestParameterName(String
parameterName) {
this.nonceRequestParameterName = parameterName;
}
+/**
+ * Sets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @param enforce true to enforce CSRF protections or
+ *false to log DEBUG messages and allow
+ *all requests.
+ */
+public void setEnforce(boolean enforce) {
+this.enforce = enforce;
+}
+
+/**
+ * Gets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @return true if CSRF protections will be enforced or
Review Comment:
Same here
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -87,11 +104,170 @@ public void setNonceRequestParameterName(String
parameterName) {
this.nonceRequestParameterName = parameterName;
}
+/**
+ * Sets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @param enforce true to enforce CSRF protections or
Review Comment:
Singular or plural? Above is singular
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -87,11 +104,170 @@ public void setNonceRequestParameterName(String
parameterName) {
this.nonceRequestParameterName = parameterName;
}
+/**
+ * Sets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @param enforce true to enforce CSRF protections or
+ *false to log DEBUG messages and allow
+ *all requests.
+ */
+public void setEnforce(boolean enforce) {
+this.enforce = enforce;
+}
+
+/**
+ * Gets the flag to enforce CSRF protection or just log failures as DEBUG
+ * messages.
+ *
+ * @return true if CSRF protections will be enforced or
+ * false if all requests will be allowed and
+ * failures will be logged as DEBUG messages.
+ */
+public boolean getEnforce() {
+return this.enforce;
+}
+
+/**
+ * Sets the list of URL patterns to suppress nonce-addition for.
+ *
+ * Some URLs do not need nonces added to them such as static resources.
+ * By not adding nonces to those URLs, HTTP caches can be more
+ * effective because the CSRF prevention filter won't generate what
+ * look like unique URLs for those commonly-reused resources.
+ *
+ * @param patterns A comma-separated list of URL patterns that will not
+ *have nonces added to them. Patterns may begin or end with a
+ ** character to denote a suffix-match or
+ *prefix-match. Any matched URL will not have a CSRF nonce
+ *added to it when passed through
+ *{@link HttpServletResponse#encodeURL(String)}.
+ */
+public void setNoNonceURLPatterns(String patterns) {
+this.noNoncePatterns = patterns;
+
+if (null != context) {
+this.noNoncePredicates = createNoNoncePredicates(context,
this.noNoncePatterns);
+}
+}
+
+/**
+ * Creates a collection of matchers from a comma-separated string of
patterns.
+ *
+ * @param patterns A comma-separated string of URL matching patterns.
+ *
+ * @return A collection of predicates representing the URL patterns.
+ */
+protected static Collection>
createNoNoncePredicates(ServletContext context, String patterns) {
+if (null == patterns || 0 == patterns.trim().length()) {
+return null;
+}
+
+String values[] = patterns.split(",");
+
+ArrayList> matchers = new ArrayList<>(values.length);
+for (String value : values) {
+Predicate p = createNoNoncePredicate(context,
value.trim());
+
+if (null != p) {
+matchers.add(p);
+}
+}
+
+matchers.trimToSize();
+
+return matchers;
+}
+
+/**
+ * Creates a predicate that can match the specified type of pattern.
+ *
+ * @param pattern The pattern to match e.g. *.foo or
+ */bar/*.
+ *
+ * @return A Predicate which can match the specified pattern, or
+ * >null if the pattern is null or blank.
+ */
+protected static Predicate createNoNoncePredicate(ServletContext
context, String pattern) {
+if (null == pattern || 0 == pattern.trim().length()) {
+return null;
+}
+if (pattern.startsWith("mime:")) {
+return new MimePredicate(context, createNoNoncePredicate(context,
pattern.substring(5)));
+} else if (pattern.startsWith("*")) {
+
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681: URL: https://github.com/apache/tomcat/pull/681#discussion_r1434521612 ## webapps/docs/config/filter.xml: ## @@ -319,6 +326,34 @@ of java.security.SecureRandom will be used. + +A list of URL patterns that will not have CSRF nonces added +to them. You may not want to add nonces to certain URLs to avoid +creating unique URLs which may defeat resource caching, etc. + +There are 3 types of patterns supported: Review Comment: 2ab2317d581fb6657fe02529d5ad55af00161726 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
michael-o commented on code in PR #681: URL: https://github.com/apache/tomcat/pull/681#discussion_r1434518590 ## webapps/docs/config/filter.xml: ## @@ -291,6 +291,13 @@ request. The default value is 403. + +A flag to enable or disable enforcement. When enforcement is +disabled, the CsrfPreventionFilter will allow all requests and +log CSRF failures as DEBUG messages. The default is true, +enabling the enforcement of CSRF protections. + Review Comment: I see, agreed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681: URL: https://github.com/apache/tomcat/pull/681#discussion_r1434514917 ## webapps/docs/config/filter.xml: ## @@ -291,6 +291,13 @@ request. The default value is 403. + +A flag to enable or disable enforcement. When enforcement is +disabled, the CsrfPreventionFilter will allow all requests and +log CSRF failures as DEBUG messages. The default is true, +enabling the enforcement of CSRF protections. + Review Comment: Removing the filter from `web.xml` will not produce log messages for CSRF failures, nor will it add CSRF tokens to URLs produced by the application. Running in an non-enforcement mode is helpful to collect real-world information about your application without breaking it. Please see https://lists.apache.org/thread/47syblyghh3tromyf6bkvl8q14w70f3x for the initial conversation, where I make the case for a non-enforcement mode. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
michael-o commented on code in PR #681: URL: https://github.com/apache/tomcat/pull/681#discussion_r1434513367 ## webapps/docs/config/filter.xml: ## @@ -319,6 +326,34 @@ of java.security.SecureRandom will be used. + +A list of URL patterns that will not have CSRF nonces added +to them. You may not want to add nonces to certain URLs to avoid +creating unique URLs which may defeat resource caching, etc. + +There are 3 types of patterns supported: Review Comment: Most style guides will likely tell your to which number should be written up. APA, Chicago, etc. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
michael-o commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1434512783
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -198,15 +416,27 @@ protected boolean skipNonceCheck(HttpServletRequest
request) {
String requestedPath = getRequestedPath(request);
-if (!entryPoints.contains(requestedPath)) {
-return false;
+if (entryPoints.contains(requestedPath)) {
+if (log.isTraceEnabled()) {
+log.trace("Skipping CSRF nonce-check for GET request to entry
point " + requestedPath);
+}
+
+return true;
}
-if (log.isTraceEnabled()) {
-log.trace("Skipping CSRF nonce-check for GET request to entry
point " + requestedPath);
+if (null != noNoncePredicates && !noNoncePredicates.isEmpty()) {
+for (Predicate p : noNoncePredicates) {
+if (p.test(requestedPath)) {
+if (log.isTraceEnabled()) {
+log.trace("Skipping CSRF nonce-check for GET request
to no-nonce path " + requestedPath);
Review Comment:
Agreed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681: URL: https://github.com/apache/tomcat/pull/681#discussion_r1434511388 ## webapps/docs/config/filter.xml: ## @@ -319,6 +326,34 @@ of java.security.SecureRandom will be used. + +A list of URL patterns that will not have CSRF nonces added +to them. You may not want to add nonces to certain URLs to avoid +creating unique URLs which may defeat resource caching, etc. + +There are 3 types of patterns supported: Review Comment: 3 == three Though there are actually 4 (four?), now, so that needs a fix. Is there a documented preference for spelled-out numerals in Tomcat documentation? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1434510673
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -198,15 +416,27 @@ protected boolean skipNonceCheck(HttpServletRequest
request) {
String requestedPath = getRequestedPath(request);
-if (!entryPoints.contains(requestedPath)) {
-return false;
+if (entryPoints.contains(requestedPath)) {
+if (log.isTraceEnabled()) {
+log.trace("Skipping CSRF nonce-check for GET request to entry
point " + requestedPath);
+}
+
+return true;
}
-if (log.isTraceEnabled()) {
-log.trace("Skipping CSRF nonce-check for GET request to entry
point " + requestedPath);
+if (null != noNoncePredicates && !noNoncePredicates.isEmpty()) {
+for (Predicate p : noNoncePredicates) {
+if (p.test(requestedPath)) {
+if (log.isTraceEnabled()) {
+log.trace("Skipping CSRF nonce-check for GET request
to no-nonce path " + requestedPath);
Review Comment:
> No `messages.properties`?
This class does not currently use localized exception messages. I'm happy to
do that work in a separate PR. I'm trying not to re-write every line of the
source file, and I'm trying to keep things consistent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
michael-o commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1433225531
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -198,15 +416,27 @@ protected boolean skipNonceCheck(HttpServletRequest
request) {
String requestedPath = getRequestedPath(request);
-if (!entryPoints.contains(requestedPath)) {
-return false;
+if (entryPoints.contains(requestedPath)) {
+if (log.isTraceEnabled()) {
+log.trace("Skipping CSRF nonce-check for GET request to entry
point " + requestedPath);
+}
+
+return true;
}
-if (log.isTraceEnabled()) {
-log.trace("Skipping CSRF nonce-check for GET request to entry
point " + requestedPath);
+if (null != noNoncePredicates && !noNoncePredicates.isEmpty()) {
+for (Predicate p : noNoncePredicates) {
+if (p.test(requestedPath)) {
+if (log.isTraceEnabled()) {
+log.trace("Skipping CSRF nonce-check for GET request
to no-nonce path " + requestedPath);
Review Comment:
No `messages.properties`?
##
webapps/docs/config/filter.xml:
##
@@ -319,6 +326,34 @@
of java.security.SecureRandom will be used.
+
+A list of URL patterns that will not have CSRF nonces added
+to them. You may not want to add nonces to certain URLs to avoid
+creating unique URLs which may defeat resource caching, etc.
+
+There are 3 types of patterns supported:
Review Comment:
three
##
webapps/docs/config/filter.xml:
##
@@ -291,6 +291,13 @@
request. The default value is 403.
+
+A flag to enable or disable enforcement. When enforcement is
+disabled, the CsrfPreventionFilter will allow all requests and
+log CSRF failures as DEBUG messages. The default is true,
+enabling the enforcement of CSRF protections.
+
Review Comment:
I don't understand the purpose. I mean, why not then drop the filter from
the `web.xml`? We don't have this for other filter, do we?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on PR #681:
URL: https://github.com/apache/tomcat/pull/681#issuecomment-1864951885
> Re 4: I think that if one is wise enough to write a RegExp, they could use
"|" to combine several patterns, and do not really need splitting by comma. Or
do you envision a use case, where different types of patterns are used
together, and one of them is a regular expression?
>
> I mean: do a .startsWith("/") && .endsWith("/") test before calling
String.split(). Skip splitting.
>
> ```diff
> - if (null == patterns || 0 == patterns.trim().length()) {
> + if (null == patterns || 0 == (patterns = patterns.trim()).length()) {
> ...
> - String values[] = patterns.split(",");
> + String values[] = patterns.startsWith("/") && patterns.endsWith("/") ?
new String[]{ patterns } : patterns.split(",");
> ```
Yes, I was thinking that someone could specify a series of checks like
`*.css, /.*includes.*/, *.png`. I suppose if you are going to use a regular
expression, maybe the entire pattern should be used since regex is very
expensive already.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
kkolinko commented on PR #681: URL: https://github.com/apache/tomcat/pull/681#issuecomment-1864889547 Re 8: Whatever is easier. (Maybe it will be easier to extract some logic into an utility class and test that utility class. My concern is just that the logic is not trivial, is complicated by nuances like case-insensitivity, and is not tested). (Existing TestCsrfPreventionFilter class has several "simple" tests. TestRestCsrfPreventionFilter has more substantial tests, using mocks). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
kkolinko commented on PR #681:
URL: https://github.com/apache/tomcat/pull/681#issuecomment-1864873921
Re 4: I think that if one is wise enough to write a RegExp, they could use
"|" to combine several patterns, and do not really need splitting by comma. Or
do you envision a use case, where different types of patterns are used
together, and one of them is a regular expression?
I mean: do a .startsWith("/") && .endsWith("/") test before calling
String.split(). Skip splitting.
```diff
- if (null == patterns || 0 == patterns.trim().length()) {
+ if (null == patterns || 0 == (patterns = patterns.trim()).length()) {
...
- String values[] = patterns.split(",");
+ String values[] = patterns.startsWith("/") && patterns.endsWith("/") ? new
String[]{ patterns } : patterns.split(",");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on PR #681:
URL: https://github.com/apache/tomcat/pull/681#issuecomment-1864828084
> 1. There are case-insensitive file systems out there... I wonder whether
those default extensions should be treated case-insensitively. (If one is
serving a web site from an USB stick or a memory card formatted with FAT? From
a CD Drive? It is possible, but rare nowadays.)
Fair. Again, I was thinking of trying to minimize the amount of processing
required by default.
> 2. Add "*.mjs" to the list (see
https://bz.apache.org/bugzilla/show_bug.cgi?id=68378 )
Fair.
> 3. Documentation: The value in "The default is ..." does not match the
actual value of DEFAULT_NO_NONCE_URL_PATTERNS;
I will correct this.
> 4. Documentation: "Complete regular expression ... Note that patterns
cannot contain a comma"
>I think if the value starts and ends with a '/'. it would be better
to treat it whole as a single RegExp. Commas are useful in RegExes and
disallowing them in this case does not look like a benefit.
I suppose I could write a more fully-featured parser, but right now I'm
using `String.split(",")` to separate the patterns from each other. If we want
to parse `/anything/` including commas, we'll need to be able to recognize `/`
within `/.../`, escapes, etc.
I think I might like to save that for a separate PR since this one is
complicated enough. WDYT?
> 5. protected boolean skipNonceCheck(HttpServletRequest request) {
>It is hard-coded to look for GET. How about a HEAD request?
This check pre-dates this PR. I think it should be addressed separately.
> 6. protected boolean skipNonceCheck(HttpServletRequest request) {
>Further in that method. "if (!entryPoints.contains(requestedPath))
{ return false; }" - note that unless it is an entry point, processing will end
here and subsequent lines will not run. I think it was intended to be the
opposite.
I will review.
> 7. private boolean shouldAddNonce(String url) { ... }
>I think that it would make sense to skip adding nonces to the
entryPoints.
>(As a use case: the front page of Manager web application).
I think it does not matter much.
> 8. It would be good to have some test cases.
Okay. Would you prefer very targeted unit tests against e.g. the predicates
and calls to `HttpServletResponse.encodeURL` or something that includes the
whole HTTP request/response, page-generation, etc.?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
kkolinko commented on PR #681:
URL: https://github.com/apache/tomcat/pull/681#issuecomment-1864808958
1. There are case-insensitive file systems out there... I wonder whether
those default extensions should be treated case-insensitively. (If one is
serving a web site from an USB stick or a memory card formatted with FAT? From
a CD Drive? It is possible, but rare nowadays.)
2. Add "*.mjs" to the list (see
https://bz.apache.org/bugzilla/show_bug.cgi?id=68378 )
3. Documentation: The value in "The default is ..." does not match the
actual value of DEFAULT_NO_NONCE_URL_PATTERNS;
4. Documentation: "Complete regular expression ... Note that patterns
cannot contain a comma"
I think if the value starts and ends with a '/'. it would be better to treat
it whole as a single RegExp. Commas are useful in RegExes and disallowing them
in this case does not look like a benefit.
5. protected boolean skipNonceCheck(HttpServletRequest request) {
It is hard-coded to look for GET. How about a HEAD request?
6. protected boolean skipNonceCheck(HttpServletRequest request) {
Further in that method. "if (!entryPoints.contains(requestedPath)) { return
false; }" - note that unless it is an entry point, processing will end here and
subsequent lines will not run. I think it was intended to be the opposite.
7. private boolean shouldAddNonce(String url) { ... }
I think that it would make sense to skip adding nonces to the entryPoints.
(As a use case: the front page of Manager web application).
8. It would be good to have some test cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
michael-o commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1432909931
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+static {
+ArrayList> defaultNoNonceURLPatterns = new
ArrayList<>();
+
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".css"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".js"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".gif"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".png"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".jpg"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".svg"));
+
+DEFAULT_NO_NONCE_URL_PATTERNS =
Collections.unmodifiableList(defaultNoNonceURLPatterns);
Review Comment:
Agree
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1432880460
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+static {
+ArrayList> defaultNoNonceURLPatterns = new
ArrayList<>();
+
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".css"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".js"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".gif"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".png"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".jpg"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".svg"));
+
+DEFAULT_NO_NONCE_URL_PATTERNS =
Collections.unmodifiableList(defaultNoNonceURLPatterns);
Review Comment:
I think this has been resolved in ef54a1e44fe32aa0ec0fd0559726b214275046aa
and d0433b10c98b90cdca573a14c4eb64dff8bdc980.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1432856054
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+static {
+ArrayList> defaultNoNonceURLPatterns = new
ArrayList<>();
+
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".css"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".js"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".gif"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".png"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".jpg"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".svg"));
+
+DEFAULT_NO_NONCE_URL_PATTERNS =
Collections.unmodifiableList(defaultNoNonceURLPatterns);
Review Comment:
@michael-o All of this stuff can be configured-around by the user. We are
just talking about the defaults, here. If you want to serve static content from
`/static/*.jpg` but dynamic files from `/dynamic/*.jpg` which need protection,
then you can set up a regular-expression-based check.
An out-of-the-box default should work for "a great many environments" not
"every single conceivable environment".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1432853641
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+static {
+ArrayList> defaultNoNonceURLPatterns = new
ArrayList<>();
+
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".css"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".js"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".gif"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".png"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".jpg"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".svg"));
+
+DEFAULT_NO_NONCE_URL_PATTERNS =
Collections.unmodifiableList(defaultNoNonceURLPatterns);
Review Comment:
@markt-asf Sure, this could work. I could use a new type of matcher such as
`mime:[whatever]` and then use the match based upon MIME type instead of
filename. Then you can mix-and-match.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
michael-o commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1432402971
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+static {
+ArrayList> defaultNoNonceURLPatterns = new
ArrayList<>();
+
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".css"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".js"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".gif"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".png"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".jpg"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".svg"));
+
+DEFAULT_NO_NONCE_URL_PATTERNS =
Collections.unmodifiableList(defaultNoNonceURLPatterns);
Review Comment:
Is is possible that an image is created on the fly containing some kind of
secret (e.g., QR code, OTP) which should be protected?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
markt-asf commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1431736198
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+static {
+ArrayList> defaultNoNonceURLPatterns = new
ArrayList<>();
+
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".css"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".js"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".gif"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".png"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".jpg"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".svg"));
+
+DEFAULT_NO_NONCE_URL_PATTERNS =
Collections.unmodifiableList(defaultNoNonceURLPatterns);
Review Comment:
Use `ServletContext.getMimeType(String)`? My thinking was a filter for
`image/*` was a lot more compact and somewhat future proof. You could also
exclude `*font*` and so on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1431641166
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+static {
+ArrayList> defaultNoNonceURLPatterns = new
ArrayList<>();
+
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".css"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".js"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".gif"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".png"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".jpg"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".svg"));
+
+DEFAULT_NO_NONCE_URL_PATTERNS =
Collections.unmodifiableList(defaultNoNonceURLPatterns);
Review Comment:
I've added `.ico` but I want to ensure I understand @markt-asf's comment
about the content-type before I "resolve" this review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1431639832
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+static {
+ArrayList> defaultNoNonceURLPatterns = new
ArrayList<>();
+
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".css"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".js"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".gif"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".png"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".jpg"));
Review Comment:
I've added `.jpeg`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1431618064
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+static {
+ArrayList> defaultNoNonceURLPatterns = new
ArrayList<>();
+
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".css"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".js"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".gif"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".png"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".jpg"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".svg"));
+
+DEFAULT_NO_NONCE_URL_PATTERNS =
Collections.unmodifiableList(defaultNoNonceURLPatterns);
Review Comment:
> Might this be better handled by looking at content type rather than file
extension?
All we have to go on is the path of the file, so I don't think we can check
the content-type. Remember, this is primarily being used inside of
`CsrfResponseWrapper.encodeURL(String)` where we know nothing about the file
other than the path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1431615348
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+static {
+ArrayList> defaultNoNonceURLPatterns = new
ArrayList<>();
+
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".css"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".js"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".gif"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".png"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".jpg"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".svg"));
+
+DEFAULT_NO_NONCE_URL_PATTERNS =
Collections.unmodifiableList(defaultNoNonceURLPatterns);
Review Comment:
I just assumed everyone used file formats other than `.ico` for their
"Favicon". It's not "favicon" that's dead... it's `.ico`. But I can add it to
the default. Remember: these are just the _defaults_. Anyone can change them vi
configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1431612056
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+static {
+ArrayList> defaultNoNonceURLPatterns = new
ArrayList<>();
+
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".css"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".js"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".gif"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".png"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".jpg"));
Review Comment:
I've used UNIX-like systems for decades and I rarely see `.jpeg`, but I'm
happy to add it. I was trying to keep the out-of-the-box processing to a
minimum.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1431610887
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
Review Comment:
Sure, I can re-arrange these.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
michael-o commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1430461949
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+static {
+ArrayList> defaultNoNonceURLPatterns = new
ArrayList<>();
+
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".css"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".js"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".gif"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".png"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".jpg"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".svg"));
+
+DEFAULT_NO_NONCE_URL_PATTERNS =
Collections.unmodifiableList(defaultNoNonceURLPatterns);
Review Comment:
Is this really dead https://en.wikipedia.org/wiki/Favicon#How_to_use?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
michael-o commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1430459964
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+static {
+ArrayList> defaultNoNonceURLPatterns = new
ArrayList<>();
+
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".css"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".js"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".gif"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".png"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".jpg"));
Review Comment:
The original extension is `.jpeg`. `.jpg` only exists because IBM's Disk
Operating System as too stupid to handle more than three chars for file
extensions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
markt-asf commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1430459910
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+static {
+ArrayList> defaultNoNonceURLPatterns = new
ArrayList<>();
+
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".css"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".js"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".gif"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".png"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".jpg"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".svg"));
+
+DEFAULT_NO_NONCE_URL_PATTERNS =
Collections.unmodifiableList(defaultNoNonceURLPatterns);
Review Comment:
Might this be better handled by looking at content type rather than file
extension?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
isapir commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1430457497
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
Review Comment:
I had to read these two lines a couple of times. Can you move line 65 to be
before line 63? It will make the code easier to read IMO, in the sense that
first you declare a field and only afterwards you use it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1430451465
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+static {
+ArrayList> defaultNoNonceURLPatterns = new
ArrayList<>();
+
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".css"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".js"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".gif"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".png"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".jpg"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".svg"));
+
+DEFAULT_NO_NONCE_URL_PATTERNS =
Collections.unmodifiableList(defaultNoNonceURLPatterns);
Review Comment:
Sure. I'm happy to add some obvious things. I thought `.ico` was dead,
actually.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1430450663
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+static {
+ArrayList> defaultNoNonceURLPatterns = new
ArrayList<>();
+
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".css"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".js"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".gif"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".png"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".jpg"));
Review Comment:
I don't follow. How is allowing requests for .jpg and .jpeg (not mentioned
here) a DOS problem? Feel free to message me privately if something should be
kept quiet.
Oh... did you mean Disk Operating System or Denial of Service?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
michael-o commented on code in PR #681:
URL: https://github.com/apache/tomcat/pull/681#discussion_r1428761992
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+static {
+ArrayList> defaultNoNonceURLPatterns = new
ArrayList<>();
+
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".css"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".js"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".gif"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".png"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".jpg"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".svg"));
+
+DEFAULT_NO_NONCE_URL_PATTERNS =
Collections.unmodifiableList(defaultNoNonceURLPatterns);
Review Comment:
What about `.ico`?
##
java/org/apache/catalina/filters/CsrfPreventionFilter.java:
##
@@ -53,6 +58,25 @@ public class CsrfPreventionFilter extends
CsrfPreventionFilterBase {
private String nonceRequestParameterName =
Constants.CSRF_NONCE_REQUEST_PARAM;
+private boolean enforce = true;
+
+private Collection> noNoncePatterns =
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+private static final Collection>
DEFAULT_NO_NONCE_URL_PATTERNS;
+
+static {
+ArrayList> defaultNoNonceURLPatterns = new
ArrayList<>();
+
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".css"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".js"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".gif"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".png"));
+defaultNoNonceURLPatterns.add(new SuffixPredicate(".jpg"));
Review Comment:
`jpeg`, `jpg` is a DOS problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on PR #681: URL: https://github.com/apache/tomcat/pull/681#issuecomment-1858319793 Commit [e2f78ec](https://github.com/apache/tomcat/pull/681/commits/e2f78eca0c7626303e5e50f1f033770b466f1755) adds nonce-check skipping to the URLs that won't get nonces added to them. This is necessary if the `` for the `` is set to `/*` as recommended in the documentation and you want to be able to load static resources from your own application :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Csrf filter improvements [tomcat]
ChristopherSchultz commented on PR #681: URL: https://github.com/apache/tomcat/pull/681#issuecomment-1858296301 My initial testing indicates that caching is working as expected with these changes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[PR] Csrf filter improvements [tomcat]
ChristopherSchultz opened a new pull request, #681: URL: https://github.com/apache/tomcat/pull/681 Please see https://lists.apache.org/thread/47syblyghh3tromyf6bkvl8q14w70f3x for the initial conversation. I see some potential improvements for the CSRF prevention filter that will be worthwhile. Specifically: 1. Non-enforcement mode(s?) to help locate problems without breaking an application 2. URL patterns to ignore when adding CSRF tokens (e.g. static resources) to avoid breaking caching -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
