ramosbugs / openidconnect-rs

OpenID Connect Library for Rust
MIT License
367 stars 96 forks source link

Unintuitive `IssuerUrl` joins. #176

Open gibbz00 opened 3 weeks ago

gibbz00 commented 3 weeks ago

Might be related to: #277

#[test]
fn temp() {
    let base = "http://localhost:9025";
    let suffix = "/jwk";

    let expected_join = format!("{}{}", base, suffix);

    let url = Url::parse(base).unwrap();
    let actual_url_join = url.join(suffix).unwrap();
    assert_eq!(expected_join, actual_url_join.as_str());

    let issuer_url = IssuerUrl::new(base.to_string()).unwrap();
    let actual_issuer_url_join = issuer_url.join(suffix).unwrap();
    assert_eq!(expected_join, actual_issuer_url_join.as_str());
}

Last assertion fails:

assertion `left == right` failed
 left: "http://localhost:9025/jwk"
 right: "http://localhost:9025//jwk"

https://github.com/ramosbugs/openidconnect-rs/blob/02525323bfdcef1c0da17835fa0d9738db354765/src/types/mod.rs#L310

I'm currently able to work around this by calling trim_start_matches() so it not that big of an issue really. Mostly interested if/where I've missed the underlying motivation for this behavior :)

const INVALID_JOIN: &str = "unable to join issuer url server endpoint path";
let auth_url = AuthUrl::from_url(issuer_url.join(Routes::AUTHORIZE.trim_start_matches('/')).expect(INVALID_JOIN));
ramosbugs commented 2 weeks ago

I can see how this would be confusing, and I agree that the sensible result should be http://localhost:9025/jwk.

As context, I intentionally didn't use Url::join because (iiuc) it always replaces the last component of the issuer URL unless it has a trailing slash. Since an issuer URL should always be a prefix of the joined URL, we always want to append rather than replace.

The simplest fix is probably to check whether the suffix begins with a /, and not to insert another / in the middle if so (similar to the existing check on the issuer URL ending in a /). If the issuer URL ends in a / and the suffix starts with a /, we probably want to skip the duplicate leading slash in the suffix. I don't think any further canonicalization is necessary though (e.g., if there are multiple consecutive slashes in the issuer URL or the suffix).

Thoughts?