1363553 - Developer Tools Network tab improperly decodes URL-encoded params

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[gonchuki]

Attachment: firefox54_params_bug.jpg

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170418004027

Steps to reproduce:

Using Firefox 54, both 32-bit Beta and 64-bit Dev Edition
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0

I'm using the Firefox developer tools to inspect several requests that include URL-encoded param values and I started getting worried that the values I was sending were broken. However upon inspection through Fiddler and comparng with Chrome it seems Firefox is decoding the entire URL before displaying it on the network panel.

Minimal example (can be accessed for live testing at https://jsfiddle.net/zbp5L0dp/1/): 

var oReq = new XMLHttpRequest();
oReq.open("GET", "?a=b&c=d%20e%20%3D%20f%20%26%20g%20%3D%20r");
oReq.send();

I want to emphasize that this is a devtools issue, the actual request is sending the params correctly in their raw form, the decoding only happens in the inspector UI.



Actual results:

Network panel shows these as the params:

a: "b"
c: "d e "
g: " r"



Expected results:

The actual params on the request are a and c, so:

a: "b"
c: "d%20e%20%3D%20f%20%26%20g%20%3D%20r"

Comment 1

[gonchuki]

Just wanted to note that the expected result could also be the value passed through decodeURIComponent, which for this example would be "d e = f & g = r"

Comment 2

[Loic]

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=afa4e843b8e99d7b64cc23a0d42b95638147fc2b&tochange=3478d56c55a26cf90445b5c0c077a4b20d617141

Ricky Chien — Bug 1308507 - Remove all usages of nsIURL and NetworkHelper r=jsnajdr

Comment 3

[Ricky Chien [:rickychien] (inactive)]

Ah, good catch! Thanks for reporting this.

Comment 4

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Too late to fix for 53, but we could still take a patch in 54. 
Ricky, can you take this bug or help find someone to work on it?

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1363553 - Do not decode url before parsing query string

Review commit: https://reviewboard.mozilla.org/r/140158/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140158/

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Honza,

I believe this is a proper fix for not doing decodeUnicodeUrl for development purpose. Some of necko team members also told me the url on header panel is not that useful since it has been decoded.

This simple fix will affect to header panel and request-utils.js. Please check it kindly. Thanks!

Comment 7

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8868546 [details]
Bug 1363553 - Do not decode url before parsing query string

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140158/diff/1-2/

Comment 8

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

Attachment: view-url-encoded.png

(In reply to Ricky Chien [:rickychien] from comment #6)
> Honza,
> 
> I believe this is a proper fix for not doing decodeUnicodeUrl for
> development purpose. Some of necko team members also told me the url on
> header panel is not that useful since it has been decoded.
> 
> This simple fix will affect to header panel and request-utils.js. Please
> check it kindly. Thanks!
I was just testing this patch but, is this really what we want?
I understand that displaying non-encoded url is useful but,
shouldn't we have a switch that allows see both? I think that
displaying encoded url might be useful for others.

I am attaching a screenshot showing how it's done in Chrome.

Honza

Comment 9

[Ricky Chien [:rickychien] (inactive)]

Good catch! I just played with Chrome's network panel. As your screenshot says, Chrome provides a "view URL encoded" and "view decoded" on the right side for toggling encoded URL / decoded URL.

Chrome is prefer to render decoded URL by default, but user still gets a chance to view encoded URL by option. Before we provide the similar "view URL encoded" and "view decoded" options for user, what our default value would be?

Honza, should we display encoded url or decoded url by default? what's your opinion for this?

Comment 10

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

(In reply to Ricky Chien [:rickychien] from comment #9)
> Good catch! I just played with Chrome's network panel. As your screenshot
> says, Chrome provides a "view URL encoded" and "view decoded" on the right
> side for toggling encoded URL / decoded URL.
> 
> Chrome is prefer to render decoded URL by default, but user still gets a
> chance to view encoded URL by option. Before we provide the similar "view
> URL encoded" and "view decoded" options for user, what our default value
> would be?
> 
> Honza, should we display encoded url or decoded url by default? what's your
> opinion for this?
I would render decoded URL by default.

Honza

Comment 11

[gonchuki]

Honza & Ricky, 

Please remember the bug is actually about how parameters are displayed on the right panel as (it seems) the decode is happening before the parse, so by the time the right panel is to display the params it goes as if the URL that is now displayed (with all params decoded and re-appended in raw form) was the original URL, interpreting the incorrect set of params.

Defining how the full URL is displayed on the left might be more suitable for a separate effort, as all it should happen for this bug is to have the right panel parse over the raw original URL. I do agree however it would be a great addition to be able to switch between encoded and decoded views.

-- G

Comment 12

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8868546 [details]
Bug 1363553 - Do not decode url before parsing query string

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140158/diff/2-3/

Comment 13

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8868546 [details]
Bug 1363553 - Do not decode url before parsing query string

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140158/diff/3-4/

Comment 14

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8868546 [details]
Bug 1363553 - Do not decode url before parsing query string

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140158/diff/4-5/

Comment 15

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8868546 [details]
Bug 1363553 - Do not decode url before parsing query string

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140158/diff/5-6/

Comment 16

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

Comment on attachment 8868546 [details]
Bug 1363553 - Do not decode url before parsing query string

https://reviewboard.mozilla.org/r/140158/#review145518

Ok, the list of parameters is now properly rendered.

Ricky, should we introduce the option raw/decoded URL params
in a follow up?

Honza

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/748bee7648bc
Do not decode url before parsing query string r=Honza

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/748bee7648bc

Comment 19

[Nathan Froyd [:froydnj]]

Please request uplift on this when you get a chance.  This is just a polish issue, but it sounds fairly low risk, so it'd be nice to have in 54.

Comment 20

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8868546 [details]
Bug 1363553 - Do not decode url before parsing query string

Approval Request Comment
[Feature/Bug causing the regression]: bug 1308507
[User impact if declined]: minor
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: yes, please see Description
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: polish issue
[String changes made/needed]: none

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8868546 [details]
Bug 1363553 - Do not decode url before parsing query string

Polish devtools issue. Beta54+. Should be in 54 beta 11.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cd589e5aabd4

Comment 23

[Emil Ghitta, QA [:emilghitta]]

I have reproduced the issue mentioned in comment 0 using Firefox 55.0a1 (Build Id:20170509132952).

This issue is verified fixed on Firefox 55.0a1 (Build Id:20170606030207) and Firefox 54.0 (Build Id:20170605134926) on Windows 10 64bit, macOS 10.11.6 and Ubuntu 14.04 32bit