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

Incorrect URL encoding #29

Closed
zipme opened this issue Jan 26, 2017 · 9 comments
Closed

Incorrect URL encoding #29

zipme opened this issue Jan 26, 2017 · 9 comments

Comments

@zipme
Copy link

zipme commented Jan 26, 2017

When init with url like this new Url('http://www.example.com/hello world here.jpg').toString()

returns: http://www.example.com/hello%2520world%2520here.jpg

expected result: 'http://www.example.com/hello%20world%20here.jpg'

Not sure why the space got encoded into %2520

When remove the protocol it works correctly:

new Url('www.example.com/hello world here.jpg').toString()
=> 'www.example.com/hello%20world%20here.jpg'

@sindilevich
Copy link

@zipme, it looks like the URI got encoded twice. The first time it became: ... hello%20world%20here .... And then the second pass has encoded the % character and it became: ... hello%2520world%2520here .... %25 is the encoded version of the %character.

@zipme
Copy link
Author

zipme commented Jan 26, 2017

@sindilevich indeed, do you know if there is a quick fix for this?

@sindilevich
Copy link

@zipme, do you use this library with node.js?

@zipme
Copy link
Author

zipme commented Jan 26, 2017

@sindilevich I use it in browser

@sindilevich
Copy link

@zipme, I built an HTML page as follows, and used the latest url.js file as of today. It worked as expected for me: ... hello%20world%20here ....

The HTML page:

<html>

<head>
    <title>DOM URI test page</title>
    <script src="url.js"></script>
</head>

<body>
    <label>
        Enter an URI to parse:
        <input type="url" value="http://www.example.com/hello world here.jpg" class="uri-input" style="width: 100%;" />
    </label>
    <br />
    <br />
    <br />
    <label>
        The parsed URI is:
        <textarea class="uri-output" cols="80" rows="2" style="width: 100%;"></textarea>
    </label>
    <br />
    <br />
    <br />
    <button class="act-button">Act on URI</button>
    <script>
        const actButton = document.querySelector(".act-button");

        actButton.addEventListener("click", () => { 
            const uriInput = document.querySelector(".uri-input");
            const uriOutput = document.querySelector(".uri-output");
            const uri = uriInput.value;

            if (uri !== "") {
                uriOutput.value = (new URL(uri)).toString();
            }
        }, false);
    </script>
</body>

</html>

@zipme
Copy link
Author

zipme commented Jan 27, 2017

@sindilevich Indeed, I tried this in fiddler it does work correctly, I am using the library in a react project and here is the code snippet:

import Url from 'domurl'
new Url('http://www.example.com/hello world here.jpg').toString()
=> 'http://www.example.com/hello%2520world%2520here.jpg'

@step307
Copy link

step307 commented Jul 13, 2017

I've debugged a bit and found the function Url.prototype.paths a bit strange.

It seems to work as a setter for this.path by given array or as getter returning an array if nothing passed.

It looks like this function expects unescaped values.

More over the only call of this function seems to me useless:
self.paths((self.path.charAt(0) === '/' ? self.path.slice(1) : self.path).split('/') );

Basically we set this.path by array parsed from this.path. And in this case the function gets already escaped values which is not correct.

May be it should be something like:
self.paths(self.paths());
Which makes even less sense on my opinion

Whether this is required on some browsers/engines for some magic or it can be skipped.

If I delete this function or its call, all tests are passed in chrome and ff
May be IE is exactly the one who needs that.

Does anyone have any idea ?

@step307
Copy link

step307 commented Jul 13, 2017

Ok, not its clear IE needs this function to add starting "/"

@step307
Copy link

step307 commented Jul 13, 2017

Pull request: #32

@Mikhus Mikhus closed this as completed in b0b2826 Apr 23, 2018
Mikhus added a commit that referenced this issue Apr 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants