You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by "koust6u (via GitHub)" <gi...@apache.org> on 2025/02/03 14:06:59 UTC
[PR] clean up org.apache.tomcat.util.http RequestUtil [tomcat]
koust6u opened a new pull request, #818:
URL: https://github.com/apache/tomcat/pull/818
(no comment)
--
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@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PR] clean up org.apache.tomcat.util.http RequestUtil [tomcat]
Posted by "koust6u (via GitHub)" <gi...@apache.org>.
koust6u commented on code in PR #818:
URL: https://github.com/apache/tomcat/pull/818#discussion_r1940452423
##########
java/org/apache/tomcat/util/http/RequestUtil.java:
##########
@@ -123,19 +123,12 @@ public static boolean isSameOrigin(HttpServletRequest request, String origin) {
// Build scheme://host:port from request
StringBuilder target = new StringBuilder();
String scheme = request.getScheme();
- if (scheme == null) {
- return false;
- } else {
- scheme = scheme.toLowerCase(Locale.ENGLISH);
- }
- target.append(scheme);
- target.append("://");
-
String host = request.getServerName();
- if (host == null) {
+ if (scheme == null || host == null) {
return false;
}
- target.append(host);
+ scheme = scheme.toLowerCase(Locale.ENGLISH);
+ target.append(scheme).append("://").append(host);
Review Comment:
i have a question i think that is or operator,
so even if only one of them is null, it seems to be early return,
which one feels less efficient?
--
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@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PR] clean up org.apache.tomcat.util.http RequestUtil [tomcat]
Posted by "markt-asf (via GitHub)" <gi...@apache.org>.
markt-asf commented on code in PR #818:
URL: https://github.com/apache/tomcat/pull/818#discussion_r1944967982
##########
java/org/apache/tomcat/util/http/RequestUtil.java:
##########
@@ -123,19 +123,12 @@ public static boolean isSameOrigin(HttpServletRequest request, String origin) {
// Build scheme://host:port from request
StringBuilder target = new StringBuilder();
String scheme = request.getScheme();
- if (scheme == null) {
- return false;
- } else {
- scheme = scheme.toLowerCase(Locale.ENGLISH);
- }
- target.append(scheme);
- target.append("://");
-
String host = request.getServerName();
- if (host == null) {
+ if (scheme == null || host == null) {
return false;
}
- target.append(host);
+ scheme = scheme.toLowerCase(Locale.ENGLISH);
+ target.append(scheme).append("://").append(host);
Review Comment:
It is marginally less efficient because it calls request.getServerName() even if the scheme is null. But it isn't a big deal.
--
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@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PR] clean up org.apache.tomcat.util.http RequestUtil [tomcat]
Posted by "koust6u (via GitHub)" <gi...@apache.org>.
koust6u commented on code in PR #818:
URL: https://github.com/apache/tomcat/pull/818#discussion_r1940452423
##########
java/org/apache/tomcat/util/http/RequestUtil.java:
##########
@@ -123,19 +123,12 @@ public static boolean isSameOrigin(HttpServletRequest request, String origin) {
// Build scheme://host:port from request
StringBuilder target = new StringBuilder();
String scheme = request.getScheme();
- if (scheme == null) {
- return false;
- } else {
- scheme = scheme.toLowerCase(Locale.ENGLISH);
- }
- target.append(scheme);
- target.append("://");
-
String host = request.getServerName();
- if (host == null) {
+ if (scheme == null || host == null) {
return false;
}
- target.append(host);
+ scheme = scheme.toLowerCase(Locale.ENGLISH);
+ target.append(scheme).append("://").append(host);
Review Comment:
i have a question i think that is or operator,
so even if only one of them is null, it seems to be an early return,
which one feels less efficient?
--
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@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PR] clean up org.apache.tomcat.util.http RequestUtil [tomcat]
Posted by "koust6u (via GitHub)" <gi...@apache.org>.
koust6u commented on code in PR #818:
URL: https://github.com/apache/tomcat/pull/818#discussion_r1939672492
##########
java/org/apache/tomcat/util/http/RequestUtil.java:
##########
@@ -145,7 +138,7 @@ public static boolean isSameOrigin(HttpServletRequest request, String origin) {
// ports. Therefore only append the port to the target if a
// non-default port is used.
if (("http".equals(scheme) || "ws".equals(scheme)) && port != 80 ||
- ("https".equals(scheme) || "wss".equals(scheme)) && port != 443) {
+ ("https".equals(scheme) || "wss".equals(scheme)) && port != 443) {
Review Comment:
thanks
--
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@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PR] clean up org.apache.tomcat.util.http RequestUtil [tomcat]
Posted by "markt-asf (via GitHub)" <gi...@apache.org>.
markt-asf merged PR #818:
URL: https://github.com/apache/tomcat/pull/818
--
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@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PR] clean up org.apache.tomcat.util.http RequestUtil [tomcat]
Posted by "koust6u (via GitHub)" <gi...@apache.org>.
koust6u commented on code in PR #818:
URL: https://github.com/apache/tomcat/pull/818#discussion_r1939607782
##########
test/org/apache/tomcat/util/http/TestRequestUtilSameOrigin.java:
##########
@@ -42,6 +42,8 @@ public static Collection<Object[]> parameters() {
TesterRequest request2 = new TesterRequest("ws", "example.com", 80);
TesterRequest request3 = new TesterRequest("http", "example.com", 443);
TesterRequest request4 = new TesterRequest("http", "example.com", 8080);
+ TesterRequest request5 = new TesterRequest(null, "exmaple.com", 80);
Review Comment:
thank you
--
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@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PR] clean up org.apache.tomcat.util.http RequestUtil [tomcat]
Posted by "markt-asf (via GitHub)" <gi...@apache.org>.
markt-asf commented on code in PR #818:
URL: https://github.com/apache/tomcat/pull/818#discussion_r1939680064
##########
java/org/apache/tomcat/util/http/RequestUtil.java:
##########
@@ -123,19 +123,12 @@ public static boolean isSameOrigin(HttpServletRequest request, String origin) {
// Build scheme://host:port from request
StringBuilder target = new StringBuilder();
String scheme = request.getScheme();
- if (scheme == null) {
- return false;
- } else {
- scheme = scheme.toLowerCase(Locale.ENGLISH);
- }
- target.append(scheme);
- target.append("://");
-
String host = request.getServerName();
- if (host == null) {
+ if (scheme == null || host == null) {
return false;
}
- target.append(host);
+ scheme = scheme.toLowerCase(Locale.ENGLISH);
+ target.append(scheme).append("://").append(host);
Review Comment:
This is ever so slightly less efficient if scheme is null but I do like that the code is a lot more compact. I don't think it is worth checking scheme for null before retrieving the host so +1 to the change as proposed.
--
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@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PR] clean up org.apache.tomcat.util.http RequestUtil [tomcat]
Posted by "markt-asf (via GitHub)" <gi...@apache.org>.
markt-asf commented on code in PR #818:
URL: https://github.com/apache/tomcat/pull/818#discussion_r1939662587
##########
java/org/apache/tomcat/util/http/RequestUtil.java:
##########
@@ -145,7 +138,7 @@ public static boolean isSameOrigin(HttpServletRequest request, String origin) {
// ports. Therefore only append the port to the target if a
// non-default port is used.
if (("http".equals(scheme) || "ws".equals(scheme)) && port != 80 ||
- ("https".equals(scheme) || "wss".equals(scheme)) && port != 443) {
+ ("https".equals(scheme) || "wss".equals(scheme)) && port != 443) {
Review Comment:
Incorrect indentation (the original was correct)
--
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@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PR] clean up org.apache.tomcat.util.http RequestUtil [tomcat]
Posted by "funkman (via GitHub)" <gi...@apache.org>.
funkman commented on code in PR #818:
URL: https://github.com/apache/tomcat/pull/818#discussion_r1939578798
##########
test/org/apache/tomcat/util/http/TestRequestUtilSameOrigin.java:
##########
@@ -42,6 +42,8 @@ public static Collection<Object[]> parameters() {
TesterRequest request2 = new TesterRequest("ws", "example.com", 80);
TesterRequest request3 = new TesterRequest("http", "example.com", 443);
TesterRequest request4 = new TesterRequest("http", "example.com", 8080);
+ TesterRequest request5 = new TesterRequest(null, "exmaple.com", 80);
Review Comment:
exmaple is mispelled
--
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@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org