Treat userinfo as basic auth if present in URL

[jswanner]

Alternative to #540, which treats userinfo in the URL as auth: {:basic, userinfo} option.

[wojtekmach]

Thank you for the PR. I agree with the spirit of this and this is something I wanted myself. Somewhat relatedly, it's pretty standard to have postgres://userinfo@host/database, so yeah, it's super ergonomic.

That being said, there's RFC 9110 § 4.2.4: Deprecation of userinfo in http(s) URIs and my reading is that we shouldn't do that. Maybe I'm not fully understanding the intention but at the moment I'd stay on the safer side and go with #540.

WDYT?

cc @sabiwara

[sabiwara]

I think that part of the rationale esp. makes sense:

even though such usage might expose a user identifier or password.

But maybe we could just strip it from the URI struct after we put it as basic auth, so we don't accidentally log the uri with the password down the lane.

[jswanner]

TIL of the deprecation of userinfo in URLs. It certainly makes sense in end user facing applications (browsers, email clients, etc) that userinfo can cause problems esp. around phishing (https://google:com@notgoogle.com). That said, I don't see developers giving up on the convenience of things like DATABASE_URL anytime soon.

If we go with #540 then I think the warning message should include info about the deprecation as justification for the warning.

But maybe we could just strip it from the URI struct after we put it as basic auth, so we don't accidentally log the uri with the password down the lane.

I did copy that part of your implementation, I don't have an explicit test for it yet but happy to add it if we decide to go with this PR.

[jswanner]

I've copied @sabiwara's inspect assertions to ensure credentials are not leaked when inspecting the Req.Request struct.

@wojtekmach I'm okay with whichever PR we go with, this is not actually something I had ever noticed before #540 was created.

[sabiwara]

Also leaning towards this PR, I don't think there's any real risk compared to using :basic, and I think it makes sense for a high-level client like Req focused on ergonomics to support it, like curl or python's requests do.

The deprecation might be more relevant for browser contexts? I don't see how it could be a vector of attack here.

[wojtekmach]

Thank you both. I think converting userinfo as soon as possible, so that it's never logged etc, should alleviate the concerns. I'll do some more research and get back to you!