Closed Bug 1491008 Opened 2 years ago Closed 1 year ago

Netmonitor should not unescape URL-escaped parameters

Categories

(DevTools :: Netmonitor, defect, P3)

defect

Tracking

(firefox68 fixed)

RESOLVED FIXED
Firefox 68
Tracking Status
firefox68 --- fixed

People

(Reporter: freddy, Assigned: tanhengyeow, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: good-first-bug,)

Attachments

(5 files, 1 obsolete file)

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

Hi :soundharya

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

Flags: needinfo?(soundharyabharatiya)

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)

Render original URL to the appropriate places

Code ready for review in Phabricator :)

Flags: needinfo?(odvarko)

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)
Attached image urlencode-tooltip-experiment.png (obsolete) —
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

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)

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

Flags: needinfo?(odvarko)
Flags: needinfo?(odvarko)
Attached 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)

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)

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)
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)
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
No longer depends on: 1537616
You need to log in before you can comment on or make changes to this bug.