Skip to content

Don't decode url before encoding it again#179

Merged
jgm merged 3 commits intocommonmark:masterfrom
danielberndt:patch-1
Jan 9, 2020
Merged

Don't decode url before encoding it again#179
jgm merged 3 commits intocommonmark:masterfrom
danielberndt:patch-1

Conversation

@danielberndt
Copy link
Copy Markdown
Contributor

Decoding before encoding leads to invalid urls. Here's an example:

Let's assume this markdown:

[link](https://www.example.com/home/%25batty)

Note: %25 is the code for the % character

This results in:

var dest1 = decode("https://www.example.com/home/%25batty")
//dest1 = https://www.example.com/home/%batty

var dest2 = encode(dest1)
// dest2 = https://www.example.com/home/%batty 

since %ba is a potential percentage-encoded hex code, mdurl's encode doesn't change it. resulting in an invalid link.

As far as I'm aware, just doing encode(uri) is doing the desired thing. for all kinds of uris

@jgm
Copy link
Copy Markdown
Member

jgm commented Jan 8, 2020

Test failure is

Regression tests  ✓✓✓✓✓✓✓✓✓✓✓
250✘ Example 12
251=== markdown ===============
252[XSS](javascript:alert%28'XSS'%29)
253=== expected ===============
254<p><a␣href="javascript&amp;colon;alert('XSS')">XSS</a></p>
255=== got ====================
256<p><a␣href="javascript&amp;colon;alert%28'XSS'%29">XSS</a></p>

It seems to me that the change in output is unproblematic; do you agree?
If so, we can change the expected output for this test.

@colinodell
Copy link
Copy Markdown
Contributor

Could we please also add the broken example in this PR (https://www.example.com/home/%25batty) to the regression tests?

@danielberndt
Copy link
Copy Markdown
Contributor Author

Alright, I just added a fix to the existing regression test as well as adding a %25 based test

@danielberndt
Copy link
Copy Markdown
Contributor Author

I also just removed the decode import, so as a side effect this PR should make this library a little bit smaller and faster :)

(fyi: there seems to be no lint rule warning about unused variables)

@jgm jgm merged commit cd03322 into commonmark:master Jan 9, 2020
@jgm
Copy link
Copy Markdown
Member

jgm commented Jan 9, 2020

Thanks! And thanks for the lint suggestion. I added the rule and found another unused variable.

colinodell added a commit to thephpleague/commonmark that referenced this pull request Jan 9, 2020
We previously did this in commit 92ab7cb, but due to an upstream change
in commonmark/commonmark.js#179 we'll be
adopting that new approach instead.

Essentially, this change removes the decoding step prior to decoding.
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

Successfully merging this pull request may close these issues.

3 participants