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
