Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added a UriCompliance.Violation.USER_INFO to deprecate user info in HttpURI #12012

Merged
merged 3 commits into from
Jul 9, 2024
Merged

Added a UriCompliance.Violation.USER_INFO to deprecate user info in HttpURI #12012

merged 3 commits into from
Jul 9, 2024

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Jul 8, 2024

As per RFC9110 user info is deprecated in server implementations. The new violation for USER_DATA is included by default in 12.0.x, but will be removed in 12.1.x

@gregw
Added a UriCompliance.Violation to deprecate user info in HttpURI
a825525 
As per [RFC9110](https://datatracker.ietf.org/doc/html/rfc9110#name-deprecation-of-userinfo-in-) user info is deprecated in server implementations.
The new violation for USER_DATA is included by default in 12.0.x, but will be removed in 12.1.x
@gregw gregw requested review from joakime and sbordet July 8, 2024 03:55
joakime
joakime requested changes Jul 8, 2024
View reviewed changes
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RequestTest could use a minor improvement.

jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java Outdated
Comment on lines 107 to 112
Arguments.of(UriCompliance.DEFAULT, "https://local/", 200, "local"),
Arguments.of(UriCompliance.DEFAULT, "https://other/", 400, "Authority!=Host"),
Arguments.of(UriCompliance.DEFAULT, "https://user@local/", 400, "Deprecated User Info"),
Arguments.of(UriCompliance.LEGACY, "https://user@local/", 200, "local"),
Arguments.of(UriCompliance.DEFAULT, "/%2F/", 400, "Ambiguous URI path separator"),
Arguments.of(UriCompliance.UNSAFE, "/%2F/", 200, "local")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we split the good and bad tests apart.

Also, can we use a hostname in the URI that is different than the one used in the Host: local header to make sure understand which host it is using?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we split the good and bad tests apart.

I did that, but it only saves a couple of lines and requires 40 lines to be duplicated, so a net loss. It also makes it hard to see the difference between the 200 and 400 tests, which is good to see exactly what we are testing. So I prefer it as a single test.

Also, can we use a hostname in the URI that is different than the one used in the Host: local header to make sure understand which host it is using?

I added a bunch more tests, including ones that allow HttpCompliance with different authorities.

@gregw gregw requested a review from joakime July 8, 2024 13:05
joakime
joakime approved these changes Jul 8, 2024
View reviewed changes
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Will be good to have it bake for a while too.

@gregw gregw merged commit c880d93 into jetty-12.0.x Jul 9, 2024
10 checks passed
@gregw gregw deleted the experiment/12.0.x/deprecateHttpUriUserInfo branch July 9, 2024 23:03
@d2a-pnagel
Copy link

d2a-pnagel commented Sep 11, 2024 •
edited
Loading

This change causes unencoded [ and ] to be rejected too (400 Illegal Path Character). Not sure if that is intended?

@joakime
Copy link
Contributor

joakime commented Sep 11, 2024

@d2a-pnagel what does the raw (on the network) HTTP request look like that triggers this issue for you?

@d2a-pnagel
Copy link

d2a-pnagel commented Sep 11, 2024 •
edited
Loading

Not sure about "on the network", but this is output from curl. Is that sufficient?

Jetty 12.0.11 returns a 404 Not Found for the same request.

> GET /[] HTTP/1.1
> Host: api:8080
> User-Agent: curl/7.64.0
> Accept: */*
>
< HTTP/1.1 400 Bad Request
< Server: Jetty(12.0.12)
< Cache-Control: must-revalidate,no-cache,no-store
< Content-Type: text/html;charset=iso-8859-1
< Content-Length: 437
< Connection: close
<
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=ISO-8859-1"/>
<title>Error 400 Illegal Path Character</title>
</head>
<body>
<h2>HTTP ERROR 400 Illegal Path Character</h2>
<table>
<tr><th>URI:</th><td>/badURI</td></tr>
<tr><th>STATUS:</th><td>400</td></tr>
<tr><th>MESSAGE:</th><td>Illegal Path Character</td></tr>

@joakime
Copy link
Contributor

joakime commented Sep 11, 2024

Not sure about "on the network", but this is output from curl. Is that sufficient?

Yes, that is sufficient.

The [ and ] are considered reserved characters in the gen-delims ABNF in the URI spec.
https://datatracker.ietf.org/doc/html/rfc3986#section-2.2

Those two characters are reserved for IPv6 or IPvLiteral authority sections on the URI.

The change from parsing the whole URI to just parsing the pathQuery is tripping up the gen-delims vs sub-delims nuance of the path parsing.

I'll open a new Issue about this.

@joakime joakime mentioned this pull request Sep 11, 2024
Open
@joakime
Copy link
Contributor

joakime commented Sep 11, 2024

Opened Issue #12259

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joakime joakime joakime approved these changes

@sbordet sbordet sbordet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
No open projects
🧊 Jetty 12.0.12 - FROZEN
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gregw @d2a-pnagel @joakime @sbordet