search

Polymer / polymer-resin

XSS mitigation for Polymer webcomponents that uses safe html type contracts
BSD 3-Clause "New" or "Revised" License
18 stars 5 forks source link

Polymer resin fails with href #8

Closed paladox closed 5 years ago

paladox commented 5 years ago

Hi, since polymer resin 2.0, it does not work with gerrit :(

Fails with tests that insert href.


15:33:45 chrome 69                ��� elements/gr-app_test.html �� gr-app tests �� location change updates gwt footer
15:33:45 
15:33:45   expected 'about:invalid#zClosurez' to equal 'http://localhost:8081/?polygerrit=0#/test/path'
15:33:45     <unknown> at       flush at gr-app_test.html:88:0
15:33:45 
15:33:45 chrome 69                ��� elements/gr-app_test.html �� gr-app tests �� _handleLocationChange handles hashes
15:33:45

also fails with:

[BABEL] Note: The code generator has deoptimised the styling of undefined as it exceeds the max of 500KB.
safari 12.1              Beginning tests via http://localhost:8000/components/tmp.MZx3oRQo/generated-index.html?cli_browser_id=1
safari 12.1              Tests failed: Error thrown outside of test function: console.error: TypeError: undefined is not an object (evaluating 'config.auth.auth_type') (http://localhost:8000/components/tmp.MZx3oRQo/bower_components/web-component-tester/browser.js:1724)
chrome 74                Tests failed: Error thrown outside of test function: polymer-resin violation: Failed to sanitize attribute of <%s>: <%s %s="%O">["a","a","href","/admin/groups"]
                             reportHandler at /components/tmp.MZx3oRQo/test/common-test-setup.html:27
                                         b at /components/tmp.MZx3oRQo/bower_components/polymer-resin/standalone/polymer-resin.js:746
  HTMLElement.c [as _computeFinalAnnotationValue] at /components/tmp.MZx3oRQo/bower_components/polymer-resin/standalone/polymer-resin.js:772
             HTMLElement._applyEffectValue at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:2239
   HTMLElement._annotatedComputationEffect at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:1916
                HTMLElement._effectEffects at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:1698
               HTMLElement._propertySetter at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:1680
                 HTMLElement.__setProperty at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:1691
             HTMLElement._applyEffectValue at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:2250
             HTMLElement._annotationEffect at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:1856
Test run ended in failure: Error thrown outside of test function: console.error: TypeError: undefined is not an object (evaluating 'config.auth.auth_type') (http://localhost:8000/components/tmp.MZx3oRQo/bower_components/web-component-tester/browser.js:1724), Error thrown outside of test function: polymer-resin violation: Failed to sanitize attribute of <%s>: <%s %s="%O">["a","a","href","/admin/groups"]
                             reportHandler at /components/tmp.MZx3oRQo/test/common-test-setup.html:27
                                         b at /components/tmp.MZx3oRQo/bower_components/polymer-resin/standalone/polymer-resin.js:746
  HTMLElement.c [as _computeFinalAnnotationValue] at /components/tmp.MZx3oRQo/bower_components/polymer-resin/standalone/polymer-resin.js:772
             HTMLElement._applyEffectValue at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:2239
   HTMLElement._annotatedComputationEffect at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:1916
                HTMLElement._effectEffects at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:1698
               HTMLElement._propertySetter at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:1680
                 HTMLElement.__setProperty at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:1691
             HTMLElement._applyEffectValue at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:2250
             HTMLElement._annotationEffect at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:1856

chrome 74 (44/0/0 error)              
safari 12.1 (error)                   

Error: Error thrown outside of test function: console.error: TypeError: undefined is not an object (evaluating 'config.auth.auth_type') (http://localhost:8000/components/tmp.MZx3oRQo/bower_components/web-component-tester/browser.js:1724), Error thrown outside of test function: polymer-resin violation: Failed to sanitize attribute of <%s>: <%s %s="%O">["a","a","href","/admin/groups"]
                             reportHandler at /components/tmp.MZx3oRQo/test/common-test-setup.html:27
                                         b at /components/tmp.MZx3oRQo/bower_components/polymer-resin/standalone/polymer-resin.js:746
  HTMLElement.c [as _computeFinalAnnotationValue] at /components/tmp.MZx3oRQo/bower_components/polymer-resin/standalone/polymer-resin.js:772
             HTMLElement._applyEffectValue at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:2239
   HTMLElement._annotatedComputationEffect at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:1916
                HTMLElement._effectEffects at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:1698
               HTMLElement._propertySetter at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:1680
                 HTMLElement.__setProperty at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:1691
             HTMLElement._applyEffectValue at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:2250
             HTMLElement._annotationEffect at /components/tmp.MZx3oRQo/bower_components/polymer/polymer.html:1856
paladox commented 5 years ago

cc @mikesamuel. Since we really would like to have the fix for aria-*.

mikesamuel commented 5 years ago

@paladox Is Gerrit using https://www.npmjs.com/package/noclosure-resin-bridge or some equivalent bridge?

paladox commented 5 years ago

@mikesamuel it's using https://github.com/GerritCodeReview/gerrit/blob/master/polygerrit-ui/app/behaviors/safe-types-behavior/safe-types-behavior.html

paladox commented 5 years ago

version 2.0 fails with that where as version 1.2.8 works.

mikesamuel commented 5 years ago

I will look into it. Do you have a patch that I can apply to see what you're seeing? (It's been a while since I mucked around with Gerrit)

paladox commented 5 years ago

@mikesamuel you can cherry pick https://gerrit-review.googlesource.com/c/gerrit/+/224055 (on the master branch then run polygerrit-ui/app/run_test.sh)

mikesamuel commented 5 years ago

@paladox, I'm pushing a version with some recent changes to handle srcset and which include some tweaks to violation reporting so that what I'm debugging matches what I've worked on recently.

paladox commented 5 years ago

Thanks!

mikesamuel commented 5 years ago

Sorry for the delay.

AFAICT, the problem is that common-test-setup.html#22 does not specify a safeTypesBridge property.

I think it should be Gerrit.SafeTypes.safeTypesBridge to mirror gr-app.html#45.

The patch I ended up with is

diff --git a/WORKSPACE b/WORKSPACE
index f2c387bd5c..3607494a59 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -1289,8 +1289,8 @@ bower_archive(
 bower_archive(
     name = "polymer-resin",
     package = "polymer/polymer-resin",
-    sha1 = "5cb65081d461e710252a1ba1e671fe4c290356ef",
-    version = "1.2.8",
+    sha1 = "94c29926c20ea3a9b636f26b3e0d689ead8137e5",
+    version = "2.0.1",
 )

 bower_archive(
diff --git a/package.json b/package.json
index f096e2a84d..325449c294 100644
--- a/package.json
+++ b/package.json
@@ -9,7 +9,7 @@
     "eslint-plugin-html": "^5.0.5",
     "fried-twinkie": "^0.2.2",
     "typescript": "^2.x.x",
-    "web-component-tester": "^6.5.0"
+    "web-component-tester": "^6.9.2"
   },
   "scripts": {
     "test": "WCT_HEADLESS_MODE=1 WCT_ARGS='--verbose -l chrome' ./polygerrit-ui/app/run_test.sh",
diff --git a/polygerrit-ui/app/behaviors/safe-types-behavior/safe-types-behavior.html b/polygerrit-ui/app/behaviors/safe-types-behavior/safe-types-behavior.html
index 68000bce06..43022d9b42 100644
--- a/polygerrit-ui/app/behaviors/safe-types-behavior/safe-types-behavior.html
+++ b/polygerrit-ui/app/behaviors/safe-types-behavior/safe-types-behavior.html
@@ -23,7 +23,7 @@ limitations under the License.
   /** @polymerBehavior Gerrit.SafeTypes */
   Gerrit.SafeTypes = {};

-  const SAFE_URL_PATTERN = /^(https?:\/\/|mailto:|\/|#)/i;
+  const SAFE_URL_PATTERN = /^(https?:\/\/|mailto:|[^:/?#]*(?:[/?#]|$))/i;

   /**
    * Wraps a string to be used as a URL. An error is thrown if the string cannot
diff --git a/polygerrit-ui/app/test/common-test-setup.html b/polygerrit-ui/app/test/common-test-setup.html
index c5979fa74e..a549dd4c52 100644
--- a/polygerrit-ui/app/test/common-test-setup.html
+++ b/polygerrit-ui/app/test/common-test-setup.html
@@ -18,6 +18,7 @@ limitations under the License.

 <link rel="import"
     href="../bower_components/polymer-resin/standalone/polymer-resin.html" />
+<link rel="import" href="../behaviors/safe-types-behavior/safe-types-behavior.html">
 <script>
   security.polymer_resin.install({
     allowedIdentifierPrefixes: [''],
@@ -32,6 +33,7 @@ limitations under the License.
             + JSON.stringify(args));
       }
     },
+    safeTypesBridge: Gerrit.SafeTypes.safeTypesBridge,
   });
 </script>
 <script>

and that makes tests run clean except for

chrome 74                ✖ elements/diff/gr-comment-api/gr-comment-api_test.html » gr-comment-api tests » _changeComment methods » comment ranges and paths » computeAllThreads

  expected [ Array(11) ] to deeply equal [ Array(11) ]
    <unknown> at   Function.assert.deepEqual at /components/chai/chai.js:2083:0
    <unknown> at         Context.<anonymous> at gr-comment-api_test.html:641:0

which isn't obviously resin-related.

Note the change to the regexp in safe-types-behavior.html which prevents test failures on URLs like https//gerrit... (no colon) and f.oo. I haven't investigated why gerrit is generating URLs like https//....

paladox commented 5 years ago

Wow, thank you!! (yes the last one is not related to resin, i've been having that issue on the mac for a while).

mikesamuel commented 5 years ago

You're welcome. Loop me in if you decide to use the SAFE_URL_PATTERN change and someone wants a security engineer to sign off.

paladox commented 5 years ago

@mikesamuel i've done these changes (1. backports to 2.15 (support for safe-types-behavior), 2. does your suggested change to safe-types-behavior and 3. adjusts SAFE_URL_PATTERN per your suggestion so it supports polymer resin 2.x!)

  1. https://gerrit-review.googlesource.com/c/gerrit/+/225172/
  2. https://gerrit-review.googlesource.com/c/gerrit/+/225132/
  3. https://gerrit-review.googlesource.com/c/gerrit/+/225133/
paladox commented 5 years ago

Gerrit has updated successfully to 2.0.1 now using your feedback above :).

Thanks for your diff!