Closed Bug 1571742 Opened 1 year ago Closed 1 year ago

Json data is displayed as raw text instead of using the built-in Json viewer

Categories

(DevTools :: JSON Viewer, defect, P2)

defect

Tracking

(firefox-esr60 unaffected, firefox-esr68 unaffected, firefox68 unaffected, firefox69 unaffected, firefox70+ verified)

VERIFIED FIXED
Firefox 70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- unaffected
firefox70 + verified

People

(Reporter: pascalc, Assigned: sstreich)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

Mozilla/5.0 (X11; Linux x86_64; rv:70.0) Gecko/20100101 Firefox/70.0 ID:20190805220030

1/ Go to https://product-details.mozilla.org/1.0/firefox_versions.json (or any json feed

Expected result:
Json is prettified with the built-in Firefox viewer for Json

Actual result:
Json is displayed as raw text, as if the file was sent with a text/plain mimetype (it's not)

This is a regression, it workd in 69.0b11. I'll see if I can find a regression window.

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: Untriaged → JSON Viewer
Product: Firefox → DevTools

16:26.08 INFO: Last good revision: 1795156aec4f053365bc5a7160b77e2b6e752e64
16:26.08 INFO: First bad revision: 0aa6dca8717afcf336ed340caa233841104701c5
16:26.08 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=1795156aec4f053365bc5a7160b77e2b6e752e64&tochange=0aa6dca8717afcf336ed340caa233841104701c5

Sebastian, it seems that your patch in bug 1428473 broke partly our json viewer, most of the Json files I am loading in Firefox are now displayed as text/plain.

For example:
https://product-details.mozilla.org/1.0/firefox_versions.json

Is that an intended behaviour change?

Flags: needinfo?(streich.mobile)
Regressed by: 1428473

Tracking for 70 because it feels broken now so it either needs a fix before shipping 70 or if extra HTTP headers have tpo be sent to allow our json viewer to kick in , we need to document that change in our developers release notes.

Thanks for the report and regression window!

Honza

Priority: -- → P2

Hey!
As it seems the patch is working as expected. On the page "X-Content-Type:nosniff" is set and we disable all sniffers. But it appears the JSON-Viewer relies on sniffing to be enabled. In [1] we have a json-view-sniffer that changes the mime-type from "application/json" to "application/vnd.mozilla.json.view". Without that set, its rendered as text/plain.

We could allow the json-view sniffer anyway even if nosniff is enabled, if that is okay?

[1] https://searchfox.org/mozilla-central/source/devtools/client/jsonview/converter-observer.js

Flags: needinfo?(streich.mobile) → needinfo?(dd.mozilla)
Assignee: nobody → streich.mobile
Status: NEW → ASSIGNED

(In reply to Sebastian Streich [:sstreich] from comment #5)

Hey!
As it seems the patch is working as expected. On the page "X-Content-Type:nosniff" is set and we disable all sniffers. But it appears the JSON-Viewer relies on sniffing to be enabled. In [1] we have a json-view-sniffer that changes the mime-type from "application/json" to "application/vnd.mozilla.json.view". Without that set, its rendered as text/plain.

We could allow the json-view sniffer anyway even if nosniff is enabled, if that is okay?

[1] https://searchfox.org/mozilla-central/source/devtools/client/jsonview/converter-observer.js

I would like to ask Christoph if this is ok. I think it is.

Flags: needinfo?(dd.mozilla)

(In reply to Dragana Damjanovic [:dragana] from comment #7)

I would like to ask Christoph if this is ok. I think it is.

It's corner casy and Sebastian and I already discussed before we flagged your for ni. Now that you are also agreeing that it is ok to add a carveout for json, I think we should just add that carveout :-)

Keywords: checkin-needed

This failed to apply:

Please manually rebase your commits and try again. applying /tmp/tmpQYbX1M netwerk/protocol/http/nsHttpChannel.cpp Hunk #1 succeeded at 1448 with fuzz 1 (offset 2 lines). netwerk/base/nsNetUtil.cpp Hunk #2 FAILED at 2893. 1 out of 2 hunks FAILED -- saving rejects to file netwerk/base/nsNetUtil.cpp.rej abort: patch command failed: exited with status 256

Flags: needinfo?(streich.mobile)
Keywords: checkin-needed
Pushed by rmaries@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b3ee5fed44ec
Move Nosniff-Check into Sniffers r=ckerschb,dragana
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
Flags: needinfo?(streich.mobile)
Flags: qe-verify+

Following the STR from the first comment, I managed to reproduce the issue on nightly 70.0a1 (2019-08-01).
I can confirm that the issue is fixed in 70.0b7.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
See Also: → 1583869
You need to log in before you can comment on or make changes to this bug.