Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Content Security Policy error in XML parsing in Chrome #13268

Closed
1 of 6 tasks
kimsey0 opened this issue Jan 18, 2021 · 8 comments · Fixed by #13382
Closed
1 of 6 tasks

Content Security Policy error in XML parsing in Chrome #13268

kimsey0 opened this issue Jan 18, 2021 · 8 comments · Fixed by #13382
Assignees
Labels
Azure.Core bug This issue requires a change to an existing behavior in the product in order to be resolved. Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-attention This issue needs attention from Azure service team or SDK team

Comments

@kimsey0
Copy link

kimsey0 commented Jan 18, 2021

Describe the bug
The Azure SDK for JavaScript includes a utility module for XML parsing. As part of initializing the module, it attempts to parse an invalid XML string at

errorNS = parser.parseFromString("INVALID", "text/xml").getElementsByTagName("parsererror")[0]

In most browsers, this returns a document containing a <parseerror> element describing the error. The error document contains inline styles which the browser interprets. In version 86.0 and newer of Chrome (https://bugs.chromium.org/p/chromium/issues/detail?id=1148221), the error document inherits the Content Security Policy from the owner document. Therefore, on pages with a Content Security Policy with a style-src directive not including unsafe-inline, Azure SDK for JavaScript causes a CSP error to be reported and shown in the console.

To Reproduce
Steps to reproduce the behavior:

  1. Set up a page with a Content Security Policy with a style-src directive not containing unsafe-inline, for example style-src: 'self'.
  2. Load the Azure SDK for JavaScript.
  3. Open page in Google Chrome version 86.0 or newer.
  4. Look for Content Security Policy error in console.

Expected behavior
No Content Security Policy error should be shown in the console.

Screenshots
Content Security Policy error shown in console

Additional context
I'm unsure if and when the issue in Chrome will be fixed. Until it is fixed, the XML parser module will necessarily cause a Content Security Policy error when parsing an invalid XML string on sites with the relevant policy. However, since XML parsing errors are likely not expected during normal usage of the library, it would still be beneficial to prevent the CSP error on load to save developer effort in debugging it.

Potential fixes could be to:

  1. Lazily testing for browser behavior when first parsing error is encountered, instead of at module load.
  2. Removing the check for browser behavior completely and always querying for the parsererror element, even in Internet Explorer.
@ghost ghost added needs-triage This is a new issue that needs to be triaged to the appropriate team. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Jan 18, 2021
@ramya-rao-a ramya-rao-a added Azure.Core Client This issue points to a problem in the data-plane of the library. labels Jan 19, 2021
@ghost ghost removed the needs-triage This is a new issue that needs to be triaged to the appropriate team. label Jan 19, 2021
@ghost ghost added the needs-team-attention This issue needs attention from Azure service team or SDK team label Jan 19, 2021
@jeremymeng
Copy link
Contributor

@kimsey0 Thank you for the report! We will looking into this shortly.

@jeremymeng
Copy link
Contributor

note: I was able to repro by adding the following to my html

<meta http-equiv="Content-Security-Policy" content="style-src 'self'">

@jeremymeng
Copy link
Contributor

When our module is loaded, we try to parse an invalid xml document then save the namespace of the element in the parsed result to errorNS. In Chrome and Firefox DomParser.parseFromString() returns a document with the expected element. In IE it throws an DOMException of SyntaxError but we ignore the error thus didn't set errorNS.

After every time we parse an xml document, we check whether it contains errors by using the saved error namespace errorNS to search for elements with same namespace and name of parsererror.

I agree with @kimsey0 that we could remove the browser check since in IE, an error would have been thrown from parsing in the error cases, we wouldn't even get to check whether errorNS is set

we probably still to search using the same namespace, because there's a possibility that element is part of a valid xml document.

@kimsey0
Copy link
Author

kimsey0 commented Jan 23, 2021

Unfortunately, Chrome and Firefox return different error documents. Firefox has the parsererror element as the root and in namespace http://www.mozilla.org/newlayout/xml/parsererror.xml, while Chrome has it wrapped inside html and body elements and with namespace http://www.w3.org/1999/xhtml.

I think every solution is going to be a workaround. One possibility could be to do a non-namespace-aware (getElementsByTagName) search for parserelement when parsing and then, if any are found, check each of their namespaces against a lazily computed (and potentially cached) error namespace.

@kimsey0
Copy link
Author

kimsey0 commented Jan 24, 2021

Basically, something like this:

let errorNS = "";
function throwIfError(dom: Document): void {
  const parserErrors = dom.getElementsByTagName("parsererror");
  if (parserErrors.length) {
    if (!errorNS) {
      try {
        errorNS = parser.parseFromString("INVALID", "text/xml").getElementsByTagName("parsererror")[0]
          .namespaceURI!;
      } catch (ignored) {
        // Most browsers will return a document containing <parsererror>, but IE will throw.
      }
    }

    for (let i = 0; i < parserErrors.length; i++) {
      if (parserErrors[i].namespaceURI === errorNS) {
        throw new Error(parserErrors[i].innerHTML);
      }
    }
  }
}

@jeremymeng
Copy link
Contributor

One possibility could be to do a non-namespace-aware (getElementsByTagName) search for parserelement when parsing and then, if any are found, check each of their namespaces against a lazily computed (and potentially cached) error namespace.

Yes, this is exactly the same as I planned to do. Also thanks for the code snippets @kimsey0!

jeremymeng added a commit to jeremymeng/azure-sdk-for-js that referenced this issue Jan 25, 2021
We eagerly save the namespace of "parsererror" element on module load. It's
unnecessary because in most scenarios we would not encounter a parse error.
Furthermore, it caused Content Security Policy error in Chrome version 86.0 or
newer because the returned DOM object for parser errors contains some CSS
styles.

This change fixes the issue by lazily loading the error namespace when the
parsed DOM document contains "parsererror" elements.

Fixes Azure#13268
jeremymeng added a commit to jeremymeng/azure-sdk-for-js that referenced this issue Jan 25, 2021
We eagerly save the namespace of "parsererror" element on module load. It's
unnecessary because in most scenarios we would not encounter a parse error.
Furthermore, it caused Content Security Policy error in Chrome version 86.0 or
newer because the returned DOM object for parser errors contains some CSS
styles.

This change fixes the issue by lazily loading the error namespace when the
parsed DOM document contains "parsererror" elements.

Fixes Azure#13268
jeremymeng added a commit to jeremymeng/azure-sdk-for-js that referenced this issue Jan 25, 2021
We eagerly save the namespace of "parsererror" element on module load. It's
unnecessary because in most scenarios we would not encounter a parse error.
Furthermore, it caused Content Security Policy error in Chrome version 86.0 or
newer because the returned DOM object for parser errors contains some CSS
styles.

This change fixes the issue by lazily loading the error namespace when the
parsed DOM document contains "parsererror" elements.

Fixes Azure#13268
@ramya-rao-a ramya-rao-a added bug This issue requires a change to an existing behavior in the product in order to be resolved. and removed needs-team-attention This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Feb 3, 2021
@ghost ghost added the needs-team-attention This issue needs attention from Azure service team or SDK team label Feb 3, 2021
@ramya-rao-a ramya-rao-a added this to the [2021] February milestone Feb 3, 2021
jeremymeng added a commit that referenced this issue Feb 3, 2021
We eagerly save the namespace of "parsererror" element on module load. It's
unnecessary because in most scenarios we would not encounter a parse error.
Furthermore, it caused Content Security Policy error in Chrome version 86.0 or
newer because the returned DOM object for parser errors contains some CSS
styles.

This change fixes the issue by lazily loading the error namespace when the
parsed DOM document contains "parsererror" elements.

Fixes #13268
@jeremymeng
Copy link
Contributor

This has been published in @azure/core-http v1.2.3

@kimsey0
Copy link
Author

kimsey0 commented Feb 8, 2021

Thank you! We upgraded earlier today (but have some unrelated CORS errors, #13658).

@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core bug This issue requires a change to an existing behavior in the product in order to be resolved. Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-attention This issue needs attention from Azure service team or SDK team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants