Netmonitor should not unescape URL-escaped parameters

RESOLVED FIXED in Firefox 68

Status

defect
P3
normal
RESOLVED FIXED
9 months ago
2 months ago

People

(Reporter: freddyb, Assigned: tanhengyeow, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 68

Firefox Tracking Flags

(firefox68 fixed)

Details

(Whiteboard: good-first-bug,)

Attachments

(5 attachments, 1 obsolete attachment)

Reporter

Description

9 months ago
Note: The test case for this is observable within a web-platform-test that I was looking at. But the actual outcome of that wpt is *independent* of this bug.

STR
- open netmonitor and go to <https://w3c-test.org/cors/redirect-preflight.htm>
- observe all the requests, many of them containing long URL parameters
- some of those requests contain a `location` parameter that points to a URL (with more parameters)

What happens:
It shows the URL parameter unescaped in the tabular view (left panel).
The tooltip for the "File" column shows a URL of 

<https://www1.w3c-test.org/cors/resources/cors-makeheader.py?preflight=200&headers=x-test&location=https://www1.w3c-test.org/cors/resources/cors-makeheader.py?headers=x-test&4&code=303&5>

What should happen:
The actual URL is
<https://www1.w3c-test.org/cors/resources/cors-makeheader.py?preflight=200&headers=x-test&location=https%3A%2F%2Fwww1.w3c-test.org%2Fcors%2Fresources%2Fcors-makeheader.py%3Fheaders%3Dx-test%264&code=303&5>

Note: The URL is correct in the params sidepanel and it is correct when you copy the URL to clipboard.


This bug is *slightly* related to bug 1370860. But not really :-)
Thanks for reporting this!

> Note: The URL is correct in the params sidepanel and it is correct when you copy the URL to clipboard.
I don't understand this comment. The URL is also rendered unescaped in the side panel for me.
Can you attach a screenshot?

--

Anyway, what if the tooltip displayed both escaped URL as well as the raw (unescaped) version?

Honza
Flags: needinfo?(fbraun)
Priority: -- → P3
Reporter

Comment 2

9 months ago
(In reply to Jan Honza Odvarko [:Honza] from comment #1)
> > Note: The URL is correct in the params sidepanel and it is correct when you copy the URL to clipboard.
> I don't understand this comment. The URL is also rendered unescaped in the
> side panel for me.

Let me rephrase that:
First: 
The "params" sidepanel shows the param unescaped. You're right. But here it's less confusing because it's already unpacked in a tree-view of key/value pairs, e.g.

> headers	x-test
> location:	https://www1.w3c-test.org/cors/resources/cors-makeheader.py?headers=x-test&2

Note that the parameters within the location are in the in the same line and therefore not as easily confused with the other parameters


Secondly: The tooltip in the tabular view, when I hover "File" is also unencoded, but it is more confusing hwew because it's all in one line, and technically it *shows the wrong URL*.

> Can you attach a screenshot?
> 

Funnily enough, I can not. My screenshot tool is currently broken and I don't understand why (maybe Wayland related?).



> 
> Anyway, what if the tooltip displayed both escaped URL as well as the raw
> (unescaped) version?
That makes no sense, does it?
For a tooltip, I think that's just too long. It should just show the actual correct URL (which proper encoding of the location parameter).
Flags: needinfo?(fbraun)
Alright. So, let's display the actual and correct URL with proper encoding in the list of requests (File column) as well as in the tooltip.

Bug 1492450 should introduce options menu for columns and we can use that menu to introduce special option "Show Encoded" (checked by default). I don't think this is a blocker and it can be done in separate bug report as soon as Bug 1492450 is fixed.

Honza
For anyone interested in this bug:

1) The file URL is rendered here:
https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/netmonitor/src/components/RequestListColumnFile.js#37

It uses `urlDetails` structure that has unescaped URL.

2) We need to have the original correct URL in the structure as well.

The structure is created here:
https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/netmonitor/src/utils/request-utils.js#252

So, getUrlDetails function needs to store it in as well.

Honza
Mentor: odvarko
Keywords: good-first-bug
Whiteboard: good-first-bug,
Hi, Can I work on this?
It's yours :-)

Honza
Assignee: nobody → soundharyabharatiya
Thank you!
Status: NEW → ASSIGNED
@Soundharya: Hi, any progress on this bug? Are you still working on this?

honza
Flags: needinfo?(soundharyabharatiya)
@Honza: Yes. I am working on it. Will get back with the PR soon.
Flags: needinfo?(soundharyabharatiya)
Excellent, thanks for the update!
Honza
Assignee

Comment 11

4 months ago

Hi :soundharya

Are you still working on this? Do you require any assistance? :)

Flags: needinfo?(soundharyabharatiya)
Assignee

Comment 12

4 months ago

Hi Frederik

Thanks for raising this issue. I've followed the conversation and would like to ask if it makes sense for the header panel's Request URL to have its value be URL-encoded as well.

Flags: needinfo?(fbraun)
Assignee

Comment 14

4 months ago

Render original URL to the appropriate places

Assignee

Comment 15

4 months ago

Code ready for review in Phabricator :)

Flags: needinfo?(odvarko)
Reporter

Comment 16

4 months ago

Not sure. I only found the bug and it's been quite a while. Honza should know this, he's already needinfo'd.

Flags: needinfo?(fbraun)
Assignee: soundharyabharatiya → E0032242

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

I've followed the conversation and would like to ask if it makes sense for the header panel's Request URL to have its value be URL-encoded as well.

Yes

Also, could you experiment a bit and display the encoded value in the Tooltip for the File column?
I.e. we would display:

  1. Original URL
  2. One empty line
  3. Escaped URL

Or any better idea how to help the user to read escaped URL somewhere?

Thanks,
Honza

Flags: needinfo?(odvarko)
Assignee

Comment 18

4 months ago
Posted image urlencode-tooltip-experiment.png (obsolete) —
Assignee

Updated

4 months ago
Attachment #9046315 - Attachment is obsolete: true

That was quick and looks good to me!

  1. We could perhaps call it: "Original" and "Encoded" (labels should be localized properly using %S). I don't think that the 'URL' suffix is necessary.
  2. If both is the same, the tooltip should work just like now (display the original URL).

Honza

Assignee

Comment 21

4 months ago

Hi Honza

How does the attached image looks? I think the other place to show the information would be the Headers panel and we can have something like:

Encoded Request URL: ....
Unencoded Request URL ....
...
...

But that might be too much information. I think your suggestion on Comment 3 sounds good. I think we can live with the tooltip solution for now, if you think the attached image looks ok.

Flags: needinfo?(odvarko)
Assignee

Comment 23

4 months ago

Updated the patch/attachment to reflect the latest changes :)

Flags: needinfo?(odvarko)
Assignee

Updated

4 months ago
Flags: needinfo?(odvarko)
Posted image image.png

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

But that might be too much information. I think your suggestion on Comment 3 sounds good. I think we can live with the tooltip solution for now, if you think the attached image looks ok.

Ah, thanks for pointing out the comment #3! Yes that's still valid.

And yes, the tooltip looks good and we can keep it. The only thing is that original and encoded seems to be switched. Look at my screenshot (and original should be always at the top).

Honza

Flags: needinfo?(odvarko)
Assignee

Comment 25

4 months ago

Hi Honza

Updated the patch! The behaviour now:

  1. If URL is not encoded, tooltip just shows the URL as it is.
  2. If URL is encoded, show original URL first followed by encoded URL.

Try results for reference:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba39d844d088bf8c29f333372f524bb2cea88d50

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

There are some test failures. It looks like related to the tooltip change.

Like e.g. devtools/client/netmonitor/test/browser_net_api-calls.js failure is nicely reproducible locally

Honza

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

Comment 27

3 months ago

Hi :Honza

Updated all the tests! Please refer to Phabricator.

Here's the try results for reference:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad5416bfe36ed3105ebb2696d3b7106e46f49730

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

Comment 28

3 months ago
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6fb6c2f36749
Netmonitor should not unescape URL-escaped parameters. r=Honza
Flags: needinfo?(odvarko)

Comment 29

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Depends on: 1537616
No longer depends on: 1537616
You need to log in before you can comment on or make changes to this bug.