You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by "arturobernalg (via GitHub)" <gi...@apache.org> on 2023/08/26 16:56:00 UTC
[GitHub] [httpcomponents-core] arturobernalg opened a new pull request, #423: HTTPCORE-756 - Enforced the requirement that "https" URIs must not have an empty host identifier
arturobernalg opened a new pull request, #423:
URL: https://github.com/apache/httpcomponents-core/pull/423
This PR brings our `URIBuilder` in line with the standards set by RFC 9110, specifically Section 4.2.2 regarding HTTPS URIs.
Changes:
Implemented a check to ensure `https` URIs do not have an empty host identifier.
Modified `URIBuilder's toString()` method to return `invalid://` for URIs that don't adhere to the standards, as the [RFC](https://www.rfc-editor.org/rfc/rfc9110#name-https-uri-scheme) mandates such URIs be rejected as invalid.
--
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@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] arturobernalg commented on a diff in pull request #423: HTTPCORE-756 - Enforced the requirement that "https" URIs must not have an empty host identifier
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #423:
URL: https://github.com/apache/httpcomponents-core/pull/423#discussion_r1312881881
##########
httpcore5/src/test/java/org/apache/hc/core5/concurrent/TestBasicFuture.java:
##########
@@ -260,41 +260,41 @@ public void cancelled() {
@Test
void testGetWithTimeout() {
+ final AtomicBoolean isFutureCompleted = new AtomicBoolean(false);
Review Comment:
@ok2c. fair enough. Changed.
--
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@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] ok2c commented on pull request #423: HTTPCORE-756 - Enforced the requirement that "https" URIs must not have an empty host identifier
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #423:
URL: https://github.com/apache/httpcomponents-core/pull/423#issuecomment-1702595456
@arturobernalg What you can do here is move the checks to the `#build` method and keep `#buildString` as is. That would also enable to remove that ugly try-catch from the `#toString`
--
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@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] arturobernalg commented on a diff in pull request #423: HTTPCORE-756 - Enforced the requirement that "https" URIs must not have an empty host identifier
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #423:
URL: https://github.com/apache/httpcomponents-core/pull/423#discussion_r1311941788
##########
httpcore5/src/main/java/org/apache/hc/core5/net/URIBuilder.java:
##########
@@ -313,11 +314,14 @@ public URI build() throws URISyntaxException {
return new URI(buildString());
}
- private String buildString() {
+ private String buildString() throws URISyntaxException {
final StringBuilder sb = new StringBuilder();
if (this.scheme != null) {
sb.append(this.scheme).append(':');
}
+ if (URIScheme.HTTPS.same(scheme) && (host == null || host.isEmpty())) {
Review Comment:
@ok2c you're right. Changed.
--
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@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] arturobernalg commented on a diff in pull request #423: HTTPCORE-756 - Enforced the requirement that "https" URIs must not have an empty host identifier
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #423:
URL: https://github.com/apache/httpcomponents-core/pull/423#discussion_r1312860300
##########
httpcore5/src/main/java/org/apache/hc/core5/net/URIBuilder.java:
##########
@@ -309,11 +310,14 @@ public URI build() throws URISyntaxException {
return new URI(buildString());
}
- private String buildString() {
+ private String buildString() throws URISyntaxException {
final StringBuilder sb = new StringBuilder();
if (this.scheme != null) {
sb.append(this.scheme).append(':');
}
+ if (URIScheme.HTTPS.same(scheme) && (TextUtils.isBlank(host))) {
Review Comment:
True. should be `HTTP` as well.
--
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@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] ok2c commented on pull request #423: HTTPCORE-756 - Enforced the requirement that "https" URIs must not have an empty host identifier
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #423:
URL: https://github.com/apache/httpcomponents-core/pull/423#issuecomment-1699594902
@arturobernalg Is this PR ready for 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-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] arturobernalg merged pull request #423: HTTPCORE-756 - Enforced the requirement that `https` and `http` URIs must not have an empty host identifier
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg merged PR #423:
URL: https://github.com/apache/httpcomponents-core/pull/423
--
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@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] arturobernalg commented on pull request #423: HTTPCORE-756 - Enforced the requirement that "https" URIs must not have an empty host identifier
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #423:
URL: https://github.com/apache/httpcomponents-core/pull/423#issuecomment-1699651791
> @arturobernalg Is this PR ready for review?
@ok2c yes. Or is there something I'm missing?
--
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@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] arturobernalg commented on pull request #423: HTTPCORE-756 - Enforced the requirement that "https" URIs must not have an empty host identifier
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #423:
URL: https://github.com/apache/httpcomponents-core/pull/423#issuecomment-1703046218
> @arturobernalg Please also do not forget to update the description as it will be used as a commit message
@ok2c
Done
--
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@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] reschke commented on a diff in pull request #423: HTTPCORE-756 - Enforced the requirement that "https" URIs must not have an empty host identifier
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on code in PR #423:
URL: https://github.com/apache/httpcomponents-core/pull/423#discussion_r1312857095
##########
httpcore5/src/main/java/org/apache/hc/core5/net/URIBuilder.java:
##########
@@ -309,11 +310,14 @@ public URI build() throws URISyntaxException {
return new URI(buildString());
}
- private String buildString() {
+ private String buildString() throws URISyntaxException {
final StringBuilder sb = new StringBuilder();
if (this.scheme != null) {
sb.append(this.scheme).append(':');
}
+ if (URIScheme.HTTPS.same(scheme) && (TextUtils.isBlank(host))) {
Review Comment:
Why is this specific to HTTPS?
--
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@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] ok2c commented on a diff in pull request #423: HTTPCORE-756 - Enforced the requirement that "https" URIs must not have an empty host identifier
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #423:
URL: https://github.com/apache/httpcomponents-core/pull/423#discussion_r1312747037
##########
httpcore5/src/test/java/org/apache/hc/core5/concurrent/TestBasicFuture.java:
##########
@@ -260,41 +260,41 @@ public void cancelled() {
@Test
void testGetWithTimeout() {
+ final AtomicBoolean isFutureCompleted = new AtomicBoolean(false);
Review Comment:
@arturobernalg Looks good, but there appears to be unrelated change in the change-set. Please move it into a separate commit or a separate PR.
--
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@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] reschke commented on pull request #423: HTTPCORE-756 - Enforced the requirement that "https" URIs must not have an empty host identifier
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on PR #423:
URL: https://github.com/apache/httpcomponents-core/pull/423#issuecomment-1702581449
I'm not sure that changing `toString()` is a good idea here.
--
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@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] ok2c commented on pull request #423: HTTPCORE-756 - Enforced the requirement that "https" URIs must not have an empty host identifier
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #423:
URL: https://github.com/apache/httpcomponents-core/pull/423#issuecomment-1702609085
@arturobernalg Please also do not forget to update the description as it will be used as a commit message
--
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@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] ok2c commented on a diff in pull request #423: HTTPCORE-756 - Enforced the requirement that "https" URIs must not have an empty host identifier
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #423:
URL: https://github.com/apache/httpcomponents-core/pull/423#discussion_r1311292928
##########
httpcore5/src/main/java/org/apache/hc/core5/net/URIBuilder.java:
##########
@@ -313,11 +314,14 @@ public URI build() throws URISyntaxException {
return new URI(buildString());
}
- private String buildString() {
+ private String buildString() throws URISyntaxException {
final StringBuilder sb = new StringBuilder();
if (this.scheme != null) {
sb.append(this.scheme).append(':');
}
+ if (URIScheme.HTTPS.same(scheme) && (host == null || host.isEmpty())) {
Review Comment:
@arturobernalg Could you please use `TextUtils#isBlank` here for the host check? Looks good otherwise.
--
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@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org