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.

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

Remove set_user_info from URI #8738

Merged
merged 4 commits into from
Jan 15, 2024
Merged

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Jan 11, 2024

Pull Request Description

I have added this in #8591, but I have realised it may not be a good idea to have it, so I am removing that particular change.

Rationale for the changes

Reading RFC 3986 we can see that setting a password through user-info is deprecated, because it is deemed unsafe.

Moreover, basic testing suggests that our HTTP client did not in fact send this user info anyway (I'm not 100% sure of that, I'm however quite sure that it was not possible to read this data back in our HTTP server using the available APIs, after some digging).

It seems that the typical thing to do with user info specified in URI is to convert it into the Authorization: Basic HTTP header. At least that is what CURL does:

$ curl -v http://a:b@localhost:8080/get
*   Trying [::1]:8080...
* Connected to localhost (::1) port 8080
* Server auth using Basic with user 'a'
> GET /get HTTP/1.1
> Host: localhost:8080
> Authorization: Basic YTpi
> User-Agent: curl/8.4.0
> Accept: */*
>
< HTTP/1.1 200 OK
< Date: Thu, 11 Jan 2024 13:39:28 GMT
< Content-type: application/json
< Content-length: 191
<
{
  "headers": {
    "Accept": "*/*",
    "User-Agent": "curl/8.4.0",
    "Authorization": "Basic YTpi"
  },
  "origin": "0:0:0:0:0:0:0:1",
  "path": "/get",
  "method": "GET",
  "args": {}
}* Connection #0 to host localhost left intact

So, there is no 'native' support for user info in the URI. What we could do is automatically translate it into the Authorization: Basic header, like CURL does - but I'm not sure that this would be the right call. Instead, I think the user should refrain from using the user info at all for authorization, and just use our Header.authorization_basic instead.

Note that this does not prevent users from typing in user info directly, e.g. URI.from "http://user@pass:example.com/" will still be parsed and include the user info in the URI, handling it with the default behaviour (i.e. it will unfortunately be ignored later).

The only API change is that we remove set_user_info helper, which was encouraging users to use this deprecated feature.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

Copy link
Member

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

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

Minor fix

@radeusgd radeusgd self-assigned this Jan 11, 2024
@radeusgd radeusgd added the CI: No changelog needed Do not require a changelog entry for this PR. label Jan 11, 2024
@radeusgd radeusgd force-pushed the wip/radeusgd/deprecate-user-info branch from 677a703 to b62440e Compare January 15, 2024 10:49
Base automatically changed from wip/radeusgd/8556-cloud-secrets-followup to develop January 15, 2024 16:12
@radeusgd radeusgd force-pushed the wip/radeusgd/deprecate-user-info branch from b62440e to f238417 Compare January 15, 2024 16:26
@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Jan 15, 2024
@mergify mergify bot merged commit 5b70ff2 into develop Jan 15, 2024
27 checks passed
@mergify mergify bot deleted the wip/radeusgd/deprecate-user-info branch January 15, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants