Closed Bug 1500915 Opened Last year Closed 7 months ago

[Network Monitor] Hash in Address Cuts off Params

Categories

(DevTools :: Netmonitor, defect, P3)

62 Branch
defect

Tracking

(firefox68 fixed)

RESOLVED FIXED
Firefox 68
Tracking Status
firefox68 --- fixed

People

(Reporter: nikeee, Assigned: mrigankkrishan, NeedInfo)

Details

Attachments

(2 files)

Attached video 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.
Component: Untriaged → Netmonitor
Product: Firefox → DevTools

Thanks for the report and detailed STR!

I can reproduce it on my machine.

Honza

Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3

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?

Flags: needinfo?(odvarko)

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.

@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

Flags: needinfo?(odvarko) → needinfo?(mrigankkrishan)

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

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!

Flags: needinfo?(mrigankkrishan) → needinfo?(odvarko)
Assignee: nobody → mrigankkrishan
Status: NEW → ASSIGNED

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 :)

Flags: needinfo?(mrigankkrishan)

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

Flags: needinfo?(mrigankkrishan)

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.

Flags: needinfo?(E0032242)

@Mrigank: please see my comment in Phab
Thanks!

Honza

Flags: needinfo?(odvarko) → needinfo?(mrigankkrishan)

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 :(

Flags: needinfo?(E0032242) → needinfo?(odvarko)

(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

Flags: needinfo?(odvarko)
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
You need to log in before you can comment on or make changes to this bug.