1500915 - [Network Monitor] Hash in Address Cuts off Params

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[nikeee]

Attachment: Reproduction.mkv

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:62.0) Gecko/20100101 Firefox/62.0

Steps to reproduce:

See attached video or:
1. Go to https://pandoc.org/try/
2. Enter a markdown document, starting with "# foo" (or some other string containing "#")
3. Open the Network Monitor
4. Press "Convert" (submit GET request)



Actual results:

The address in the panel of the resulting request is:
https://pandoc.org/cgi-bin/trypandoc?from=markdown&to=html&text=

If the input is something like `foo # bar`, the URL is:
https://pandoc.org/cgi-bin/trypandoc?from=markdown&to=html&text=foo+

Everything after the '#' sign is is cut off.

Note that if the query parameter value contains a line break (\n), it is not displayed in the URL either. It is just omitted, instead of escaped. E. g. `foo\nbar` is displayed as:

`https://pandoc.org/cgi-bin/trypandoc?from=markdown&to=html&text=foobar`

The "Params" tab which can be used to inspect the query strnig of the URL lists `#+foo` as a value for the `text` parameter, which is correct. If one clicks on the element to copy its value, the + is not unescaped to a whitespace charater.

A similar thing happens to line breaks, as they just vanish.


Expected results:

The actual URL that is displayed in the address bar displays the '#' escaped as '%23', which I was expecting in the URL of the network request in the network monitor.

https://pandoc.org/try/?text=%23+foo&from=markdown&to=

In case the address contains a line break, I expected it to contain the escaped line break, just like in the address bar:
`https://pandoc.org/try/?text=foo%0Abar&from=markdown&to=`

In the "Params" tab, I expected + to be converted to a blank character when tryping to copy the value. I also expect line breaks not to vanish, at least when the value is copied.

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the report and detailed STR!

I can reproduce it on my machine.

Honza

Comment 2

[Mrigank Krishan]

I was able to reproduce this on my machine. Here are some ideas:

• Have a button to toggle URL encoding, or at least convert + to (space). Just like chromium.
• I am not sure what to do with the line breaks yet. I guess removing line-breaks in the UI is fine, but we should preserve it while copying.

PS: I would like to work on this bug, can someone assign this to me?

Comment 3

[Mrigank Krishan]

I think I found the problem:
In unicode-url.js#103, we are using decodeURIComponent to decode, this is the problem.

Let's say, our URL is https://google.com/?q=foo%23bar#alpha.

Because we are decoding complete URL, there is no way to distinguish between %23 and #.
Also, this # is used here to split the URL.

Here are some possible solutions:

• use decodeURI instead?
• In request-utils.js, parse the URL, keep the hash separately and decode rest of the URL.

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@mrigank: This sounds promising. Are you willing to provide a patch, so I can try it on my machine?

Should I assign the bug to you?

Honza

Comment 5

[Mrigank Krishan]

Attachment: Bug 1500915 - Properly decode urls. r=Honza

Used decodeURI insted of decodeURLComponent and replace + with (space). I've not tackled the line break issue yet.

Comment 6

[Mrigank Krishan]

Are you willing to provide a patch, so I can try it on my machine?

Patch uploaded.

Should I assign the bug to you?

Sure!

Comment 7

[Hengyeow (:tanhengyeow)]

Changes look good! Please change the comments of the function getUnicodeUrl since we are not using decodeURIComponent anymore.

Also, I am not able to reproduce the line break issue anymore. Please verify if it works for you as well.

Try results here for reference to ensure there's no regression:
https://hg.mozilla.org/try/rev/4e6d97d26a0d22b4babdf98a48cadf4756794925

In the meantime I think you can explore writing tests:

1. Refer to browser_net_header-ref-policy.js for inspiration to test that the summary URL doesn't cut off values if there is a # value.
2. Refer to browser_net_params_sorted.js for inspiration to test that the params value does not have a + sign but instead should show spaces.

Please add me in as reviewer as well so I can comment on Phabricator directly :)

Comment 8

[Hengyeow (:tanhengyeow)]

Actual try results link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a4112eef6f95b3ea85ddde84396d0123c22d206

Comment 9

[Mrigank Krishan]

@tanhengyeow I've added you as a reviewer.
Please take a look at the updated patch on phabricator.

Comment 10

[Patrick Brosset <:pbro>]

It came to my attention that this bug hasn't moved for the past month. It'd be great to get it moving again since some good work has already happened on phabricator.
It looks like Mrigank is waiting for a new review from Heng Yeow, so I'll needinfo them.

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Mrigank: please see my comment in Phab
Thanks!

Honza

Comment 12

[Hengyeow (:tanhengyeow)]

Changes look good as of the latest patch! Thanks for working on this :)

P.S Somehow I cannot accept the revision/comment on it even when assigned as reviewer :(

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/11ea9feda2a3
Properly decode urls. r=Honza

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Heng Yeow (:tanhengyeow) from comment #12)

Changes look good as of the latest patch! Thanks for working on this :)

P.S Somehow I cannot accept the revision/comment on it even when assigned as reviewer :(

Ah, it might require higher commit level, anyway thanks for looking at that and posting the comment!
I removed you as the reviewer and landed the patch.

Honza

Comment 15

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/11ea9feda2a3