Closed
Bug 1363553
Opened 5 years ago
Closed 5 years ago
Developer Tools Network tab improperly decodes URL-encoded params
Categories
(DevTools :: Netmonitor, defect, P3)
Tracking
(firefox-esr52 unaffected, firefox53- wontfix, firefox54+ verified, firefox55+ verified)
VERIFIED
FIXED
Firefox 55
People
(Reporter: gonchuki, Assigned: rickychien)
References
Details
(Keywords: regression)
Attachments
(3 files)
56.34 KB,
image/jpeg
|
Details | |
59 bytes,
text/x-review-board-request
|
Honza
:
review+
gchang
:
approval-mozilla-beta+
|
Details |
24.60 KB,
image/png
|
Details |
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"
Updated•5 years ago
|
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
status-firefox53:
--- → affected
status-firefox54:
--- → affected
status-firefox55:
--- → affected
tracking-firefox53:
--- → ?
tracking-firefox54:
--- → ?
tracking-firefox55:
--- → ?
Ever confirmed: true
Flags: needinfo?(rchien)
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Version: 54 Branch → 53 Branch
Assignee | ||
Comment 3•5 years ago
|
||
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?
Flags: needinfo?(rchien)
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Flags: needinfo?(rchien)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•5 years ago
|
||
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 hidden (mozreview-request) |
Comment 8•5 years ago
|
||
(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
Assignee | ||
Comment 9•5 years ago
|
||
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)
Comment 10•5 years ago
|
||
(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)
Reporter | ||
Comment 11•5 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•5 years ago
|
||
mozreview-review |
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+
Comment 17•5 years ago
|
||
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/748bee7648bc Do not decode url before parsing query string r=Honza
Comment 18•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/748bee7648bc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
![]() |
||
Comment 19•5 years ago
|
||
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)
Assignee | ||
Comment 20•5 years ago
|
||
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?
Updated•5 years ago
|
status-firefox-esr52:
--- → unaffected
Comment 21•5 years ago
|
||
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+
Comment 22•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/cd589e5aabd4
Updated•5 years ago
|
Flags: qe-verify+
Comment 23•5 years ago
|
||
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+
Updated•4 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•