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

SetQueryParam strips leading slash in file URI with empty hostname #751

Open
markmccaigue opened this issue May 4, 2023 · 4 comments
Open
Labels

Comments

@markmccaigue
Copy link

Hi,

I have written the below test to illustrate what I suspect is a problem with the handling of slashes with file URIs.

[Test]
public void FlurlUrl_PreservesLeadingSlash()
{
    var urlString = "file:///document";

    var flurl = new Url(urlString);

    Assert.AreEqual(urlString, flurl.ToString());

    var queryKey = "queryKey";
    var queryValue = "queryValue";
    flurl = flurl.SetQueryParam(queryKey, queryValue);

    Assert.AreEqual($"{urlString}?{queryKey}={queryValue}", flurl.ToString());
}

I would expect this test to pass, but instead it fails with the following message:

String lengths are both 36. Strings differ at index 7.
Expected: "file:///document?queryKey=queryValue"
But was:  "file://document/?queryKey=queryValue"

In summary, I believe Flurl is removing that third slash and inadvertently generating an invalid URI.

Many thanks for your help.

@tmenier
Copy link
Owner

tmenier commented May 23, 2023

Interesting. I'll be honest, I didn't know the triple-slash notation was a thing when I wrote the normalization logic. But it looks legitimate and I'll make sure it's not tampered with when specified.

In summary, I believe Flurl is removing that third slash and inadvertently generating an invalid URI.

Technically not correct to say it's an invalid URI, but it's certainly the URI you expected, so point taken. 😉

@tmenier
Copy link
Owner

tmenier commented May 23, 2023

note to self: https://superuser.com/a/352134

@markmccaigue
Copy link
Author

Interesting. I'll be honest, I didn't know the triple-slash notation was a thing when I wrote the normalization logic. But it looks legitimate and I'll make sure it's not tampered with when specified.

In summary, I believe Flurl is removing that third slash and inadvertently generating an invalid URI.

Technically not correct to say it's an invalid URI, but it's certainly the URI you expected, so point taken. 😉

Haha fair enough 😁 Thanks a million!

@EamonHetherton
Copy link

EamonHetherton commented Jan 31, 2025

Just reporting that I am experiencing this issue too.

It's actually worse than the issue suggests as it will mutate the URL and remove the extra slash just by checking if a query param exists, even if you do not intend to mutate the URL in any way.

[Fact]
public void FlurlUrl_PreservesLeadingSlash()
{
    var urlString = "file:///document";

    var flurl = new Url(urlString);

    Assert.Equal(urlString, flurl.ToString());

    var queryKey = "queryKey";
    flurl.QueryParams.Contains(queryKey);

    Assert.Equal(urlString, flurl.ToString());
}

As a result, I was seeing a reload loop in my code. Using chromium (via CefSharp) would add the third slash and generate an "OnAddressChanged" event, then my logic to ensure a query parameter existed would remove the third slash as a result of this issue even when the Url was otherwise correct triggering a new page load as the URL was now "different":
-> load "file://" url
-> chromium adds slash
-> OnAddressChanged
-> flurl removes slash by checking for query param
-> load "file://" url
-> ad infinitum.

For now, I will workaround in the client code by adding the extra '/' back to a "file" scheme URL if it was there to start with but may attempt to fix it with a pull request if I get time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Planned
Development

No branches or pull requests

3 participants