You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cxf.apache.org by "WhiteCat22 (via GitHub)" <gi...@apache.org> on 2023/10/20 17:02:35 UTC
[PR] [CXF-8947] - Avoid expensive regex operations in Rfc3986UriValidator [cxf]
WhiteCat22 opened a new pull request, #1483:
URL: https://github.com/apache/cxf/pull/1483
https://issues.apache.org/jira/browse/CXF-8947
--
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-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8947] - Avoid expensive regex operations in Rfc3986UriValidator [cxf]
Posted by "WhiteCat22 (via GitHub)" <gi...@apache.org>.
WhiteCat22 commented on code in PR #1483:
URL: https://github.com/apache/cxf/pull/1483#discussion_r1380784747
##########
rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/Rfc3986UriValidator.java:
##########
@@ -39,21 +39,28 @@ final class Rfc3986UriValidator {
private static final String LAST = "#(.*)";
- private static final Pattern HTTP_URL = Pattern.compile("^" + SCHEME
+ private static final Pattern HTTP_URL = Pattern.compile("^" + SCHEME
+ "(//(" + USERINFO + "@)?" + HOST + ")?" + PATH
+ "(\\?" + QUERY + ")?" + "(" + LAST + ")?");
private Rfc3986UriValidator() {
}
/**
- * Validate the HTTP URL according to https://datatracker.ietf.org/doc/html/rfc3986#appendix-B
+ * Validate the HTTP URL according to https://datatracker.ietf.org/doc/html/rfc3986#appendix-B
* @param uri HTTP schemed URI to validate
* @return "true" if URI matches RFC-3986 validation rules, "false" otherwise
*/
public static boolean validate(final URI uri) {
// Only validate the HTTP(s) URIs
- if (HttpUtils.isHttpScheme(uri.getScheme())) {
+ if (HttpUtils.isHttpScheme(uri.getScheme())) {
+ // If URI.getHost() returns a host name, validate it and
+ // skip the expensive regular expression logic.
+ final String uriHost = uri.getHost();
Review Comment:
That is a good point that we had not considered.
--
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-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8947] - Avoid expensive regex operations in Rfc3986UriValidator [cxf]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet merged PR #1483:
URL: https://github.com/apache/cxf/pull/1483
--
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-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8947] - Avoid expensive regex operations in Rfc3986UriValidator [cxf]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #1483:
URL: https://github.com/apache/cxf/pull/1483#issuecomment-4038692781
Hi @reta, I've pushed a commit addressing your review feedback:
- **Removed** the `URI.getHost()` shortcut in `Rfc3986UriValidator` — you were right that Java's `URI.getHost()` is not RFC-3986 compliant, so it can't be trusted here.
- **Simplified** `HttpUtils.isHttpScheme()` to use `equalsIgnoreCase()` with two string constants instead of a `HashSet`, as you suggested.
Also, while investigating the flaky `NettyServerTest.testGetWsdl` from #1517, we found the root cause: `ServiceWSDLBuilder` uses a `HashMap` for namespace-to-prefix mapping, causing non-deterministic WSDL output. That's been fixed separately in #2944.
--
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-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8947] - Avoid expensive regex operations in Rfc3986UriValidator [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1483:
URL: https://github.com/apache/cxf/pull/1483#discussion_r1369414837
##########
rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/impl/Rfc3986UriValidator.java:
##########
@@ -39,21 +39,28 @@ final class Rfc3986UriValidator {
private static final String LAST = "#(.*)";
- private static final Pattern HTTP_URL = Pattern.compile("^" + SCHEME
+ private static final Pattern HTTP_URL = Pattern.compile("^" + SCHEME
+ "(//(" + USERINFO + "@)?" + HOST + ")?" + PATH
+ "(\\?" + QUERY + ")?" + "(" + LAST + ")?");
private Rfc3986UriValidator() {
}
/**
- * Validate the HTTP URL according to https://datatracker.ietf.org/doc/html/rfc3986#appendix-B
+ * Validate the HTTP URL according to https://datatracker.ietf.org/doc/html/rfc3986#appendix-B
* @param uri HTTP schemed URI to validate
* @return "true" if URI matches RFC-3986 validation rules, "false" otherwise
*/
public static boolean validate(final URI uri) {
// Only validate the HTTP(s) URIs
- if (HttpUtils.isHttpScheme(uri.getScheme())) {
+ if (HttpUtils.isHttpScheme(uri.getScheme())) {
+ // If URI.getHost() returns a host name, validate it and
+ // skip the expensive regular expression logic.
+ final String uriHost = uri.getHost();
Review Comment:
@WhiteCat22 the reason for this validator to exists sadly is the fact that Java's URI is not RFC-3986 complaint. The `host` is not trustful source here hence we validate it against the pattern.
--
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-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8947] - Avoid expensive regex operations in Rfc3986UriValidator [cxf]
Posted by "WhiteCat22 (via GitHub)" <gi...@apache.org>.
WhiteCat22 commented on code in PR #1483:
URL: https://github.com/apache/cxf/pull/1483#discussion_r1380783800
##########
rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/utils/HttpUtils.java:
##########
@@ -93,13 +93,18 @@ public final class HttpUtils {
// there are more of such characters, ex, '*' but '*' is not affected by UrlEncode
private static final String PATH_RESERVED_CHARACTERS = "=@/:!$&\'(),;~";
private static final String QUERY_RESERVED_CHARACTERS = "?/,";
-
+
private static final Set<String> KNOWN_HTTP_VERBS_WITH_NO_REQUEST_CONTENT =
new HashSet<>(Arrays.asList(new String[]{"GET", "HEAD", "OPTIONS", "TRACE"}));
private static final Set<String> KNOWN_HTTP_VERBS_WITH_NO_RESPONSE_CONTENT =
new HashSet<>(Arrays.asList(new String[]{"HEAD", "OPTIONS"}));
-
- private static final Pattern HTTP_SCHEME_PATTERN = Pattern.compile("^(?i)(http|https)$");
Review Comment:
Hi @reta , Sorry for the delay. The reason we changed this was because `HashSet.contains(String)` uses "WAY" less CPU than `Pattern.matcher(String).matches()`
--
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-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8947] - Avoid expensive regex operations in Rfc3986UriValidator [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1483:
URL: https://github.com/apache/cxf/pull/1483#discussion_r1380901764
##########
rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/utils/HttpUtils.java:
##########
@@ -93,13 +93,18 @@ public final class HttpUtils {
// there are more of such characters, ex, '*' but '*' is not affected by UrlEncode
private static final String PATH_RESERVED_CHARACTERS = "=@/:!$&\'(),;~";
private static final String QUERY_RESERVED_CHARACTERS = "?/,";
-
+
private static final Set<String> KNOWN_HTTP_VERBS_WITH_NO_REQUEST_CONTENT =
new HashSet<>(Arrays.asList(new String[]{"GET", "HEAD", "OPTIONS", "TRACE"}));
private static final Set<String> KNOWN_HTTP_VERBS_WITH_NO_RESPONSE_CONTENT =
new HashSet<>(Arrays.asList(new String[]{"HEAD", "OPTIONS"}));
-
- private static final Pattern HTTP_SCHEME_PATTERN = Pattern.compile("^(?i)(http|https)$");
Review Comment:
Fair enough, may be we could just have two constants instead?
--
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-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8947] - Avoid expensive regex operations in Rfc3986UriValidator [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1483:
URL: https://github.com/apache/cxf/pull/1483#discussion_r1369416097
##########
rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/utils/HttpUtils.java:
##########
@@ -93,13 +93,18 @@ public final class HttpUtils {
// there are more of such characters, ex, '*' but '*' is not affected by UrlEncode
private static final String PATH_RESERVED_CHARACTERS = "=@/:!$&\'(),;~";
private static final String QUERY_RESERVED_CHARACTERS = "?/,";
-
+
private static final Set<String> KNOWN_HTTP_VERBS_WITH_NO_REQUEST_CONTENT =
new HashSet<>(Arrays.asList(new String[]{"GET", "HEAD", "OPTIONS", "TRACE"}));
private static final Set<String> KNOWN_HTTP_VERBS_WITH_NO_RESPONSE_CONTENT =
new HashSet<>(Arrays.asList(new String[]{"HEAD", "OPTIONS"}));
-
- private static final Pattern HTTP_SCHEME_PATTERN = Pattern.compile("^(?i)(http|https)$");
Review Comment:
This pattern is super straightforward to look up, what is exactly the gains here (vs adding the set + comparator + lowecasing)?
##########
rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/utils/HttpUtils.java:
##########
@@ -93,13 +93,18 @@ public final class HttpUtils {
// there are more of such characters, ex, '*' but '*' is not affected by UrlEncode
private static final String PATH_RESERVED_CHARACTERS = "=@/:!$&\'(),;~";
private static final String QUERY_RESERVED_CHARACTERS = "?/,";
-
+
private static final Set<String> KNOWN_HTTP_VERBS_WITH_NO_REQUEST_CONTENT =
new HashSet<>(Arrays.asList(new String[]{"GET", "HEAD", "OPTIONS", "TRACE"}));
private static final Set<String> KNOWN_HTTP_VERBS_WITH_NO_RESPONSE_CONTENT =
new HashSet<>(Arrays.asList(new String[]{"HEAD", "OPTIONS"}));
-
- private static final Pattern HTTP_SCHEME_PATTERN = Pattern.compile("^(?i)(http|https)$");
Review Comment:
This pattern is super straightforward to look up, what are exactly the gains here (vs adding the set + comparator + lowecasing)?
--
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-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org