1635835 - Network response JSON is not parsed if leading XSSI-prevention characters like )]}'

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[bradw2k]

Attachment: Screen Shot 2020-05-06 at 9.49.59 AM.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:75.0) Gecko/20100101 Firefox/75.0

Steps to reproduce:

1. Open dev > Network > Response
2. Get a JSON response with these prepended (security) characters:
   )]}',

Actual results:

The JSON is not parsed and pretty printed as should be, instead the raw text is shown with error message "SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data"

Expected results:

Should ignore the prepended security characters, and parse and pretty-print the JSON response.
The prepended security characters are a best-practice: https://security.stackexchange.com/questions/110539/how-does-including-a-magic-prefix-to-a-json-response-work-to-prevent-xssi-attack

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 2

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

Thanks for the report!

Yes, agree, we should improve the way we parse JSON responses.

Honza

Comment 3

[:Harald Kirschner :digitarald]

v0 could add workaround for the most common XSSI prevents, like )]}'. Alternatively, we could trim JSON-type responses from the beginning to the first [/{ by default; which might work for more cases.

Could this be a good first bug, Bomsy?

Comment 4

[Hubert Boma Manilla (:bomsy)]

Yeah i agree, we could start of with a simple utility function which strips out known sequences like )]}, for(;;),while(1); etc
Add the utility here
https://searchfox.org/mozilla-central/rev/8827278483c337667cdfb238112eb1be397dd102/devtools/client/netmonitor/src/utils/request-utils.js
And here is where the text to sanitize is
https://searchfox.org/mozilla-central/rev/8827278483c337667cdfb238112eb1be397dd102/devtools/client/netmonitor/src/components/request-details/ResponsePanel.js#290

Comment 5

[Dev Singh]

Hi all,
I'm sorry if this is not standard practice, I'm new so I'm not quite sure what that would be!
I'm looking to work on fixing this, is there somewhere where I need to make this known?

Thanks

Comment 6

[Hubert Boma Manilla (:bomsy)]

Thanks for your interest in working on this.
I'll assign to you.

Let us know if you need any help.

Comment 7

[Dev Singh]

Attachment: testFile.js

Hi, 
I edited the isJSON function to start parsing only from the first JSON-starting character (`[` or `{`). When run outside of the browser, the isJSON function returns the correct JSON without the XSSI-prevention characters. However, the browser still throws the same error. Here's my test file that I used with the edited function attached. The render function seems to take the output from this function as the output to render, but it's still using the raw webserver output.

Comment 8

[Dev Singh]

Hi,
I edited the isJSON function to start parsing only from the first JSON-starting character ([ or {). When run outside of the browser, the isJSON function returns the correct JSON without the XSSI-prevention characters. However, the browser still throws the same error. Here's my test file that I used with the edited function attached. The render function seems to take the output from this function as the output to render, but it's still using the raw webserver output. (bugzilla won't seem to let me upload a file, so I have pasted it in)

function isBase64(payload) {
    try {
      return btoa(atob(payload)) == payload;
    } catch (err) {
      return false;
    }
}

function isJSON(payloadUnclean) {
  let json, error;
  // Start at the first likely JSON character, so that magic XSSI characters can be avoided.
  const firstSquare = payloadUnclean.indexOf("[");
  const firstCurly = payloadUnclean.indexOf("{");
  const first = Math.max(firstCurly, firstSquare);
  let payload = "";
  if (first !== -1) {
    try {
      payload = payloadUnclean.substring(first);
    } catch (err) {
      error = err
    }
  }
  try {
    json = JSON.parse(payload);
  } catch (err) {
    if (isBase64(payload)) {
      try {
        json = JSON.parse(atob(payload));
      } catch (err64) {
        error = err;
      }
    } else {
      error = err;
    }
  }

  // Do not present JSON primitives (e.g. boolean, strings in quotes, numbers)
  // as JSON expandable tree.
  if (!error) {
    if (typeof json !== "object") {
      return {};
    }
  }

  return {
    json,
    error,
  };
}
let { json, error } = isJSON('while(;;))}]{"name": "John","age": 30,"car": null}')
console.log(json)
console.log(error)

{ name: 'John', age: 30, car: null }
undefined

The browser GUI still returns the original string sent from the webserver.

Comment 9

[Dev Singh]

Sorry, forgot to needinfo. Still doesn't make sense to me, even after looking at the code in the frontend.

Comment 10

[Dev Singh]

Doy! I realized that the main page JSON parsing fails because it does not use the same isJSON function. The devtools parsing has been fixed, but the first page of where the browser shows the JSON on load is still broken since it's a seperate component. I will file a seperate bug for that.

Sorry for the spam, all!

Comment 11

[Dev Singh]

Attachment: Fixes Bug 1635835 r=Honza

Comment 13

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

I filled a foolow up bug 1643696 for renaming isJSON to parseJSON

Honza

Comment 14

[Dev Singh]

Attachment: Bug 1635835 - Fix request parsing with XSSI-escapes in requests and responses r=Honza,bomsy

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4cee9ec9155e
Fix request parsing with XSSI-escapes in requests and responses r=Honza,bomsy

Comment 17

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/4cee9ec9155e