Developer Tools Network tab improperly decodes URL-encoded params

VERIFIED FIXED in Firefox 54

Status

defect
P3
normal
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: gonchuki, Assigned: rickychien)

Tracking

({regression})

53 Branch
Firefox 55
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53- wontfix, firefox54+ verified, firefox55+ verified)

Details

Attachments

(3 attachments)

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"
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"
Component: Untriaged → Developer Tools: Netmonitor
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
Blocks: 1308507
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Ever confirmed: true
Flags: needinfo?(rchien)
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Version: 54 Branch → 53 Branch
Ah, good catch! Thanks for reporting this.
Flags: needinfo?(rchien)
Priority: -- → P3
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?
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Flags: needinfo?(rchien)
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!
Posted image 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
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?
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko)
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 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
Attachment #8868546 - Flags: review?(odvarko) → review+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/748bee7648bc
Do not decode url before parsing query string r=Honza
https://hg.mozilla.org/mozilla-central/rev/748bee7648bc
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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.
Flags: needinfo?(rchien)
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
Flags: needinfo?(rchien)
Attachment #8868546 - Flags: approval-mozilla-beta?
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.
Attachment #8868546 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.