Fixes: #1679. Remove unsafe-eval directive from our CSP policy. #1682

miketaylr · 2017-07-27T20:48:29Z

This is a pretty big change to how we do templates. But it's also 1000% safer than what we currently allow.

~~Please don't merge yet, but feel free to comment.~~ ready for review/merge

It also probably needs some docs updates to mention that if you ever change a template file, you'll need to re-run grunt:jst (or grunt). edit: done

grunt-tasks/jst.js

+          // strip the opening and closing <script> tags...
+          // otherwise, the template functions will just inject script elements
+          // that won't render.
+          src = src.split(/\<script type=['"]text\/template['"]>/)[1];


miketaylr · 2017-07-27T20:57:05Z

Better than expected!

15/77 tests failed (3 skipped)

miketaylr · 2017-07-27T22:01:29Z

Ah yeah, forgot about a tiny inline template in the labels code :))

Error: call to Function() blocked by CSP

Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src http://localhost:5000 https://www.google-analytics.com https://api.github.com”). Source: call to eval() or related function blocked by CSP.

miketaylr · 2017-07-27T22:24:13Z

Cool, that fixed it.

webcompat/templates/issue/issue-labels-sub.jst

+    <%= label.name %>
+  </span>
+<% }); %>
+<script type="text/template">


webcompat/templates/issue/issue-labels.jst

  <header class="wc-Issue-labelEditor">
    <div class="wc-Issue-labelEditor-title">
      Labels
    </div>
-    {% if session.user_id and session.avatar_url %}
+    <% if ($("body").data("username")) { %>


webcompat/templates/web_modules/dropdown.html

@@ -1,3 +0,0 @@
-{% macro dropdown(config) %}


miketaylr · 2017-07-27T22:47:11Z

OK, tests pass and I get no new csp errors locally when messing around.

r? @zoepage
r? @magsout

zoepage · 2017-07-28T16:25:27Z

@miketaylr that is really impressing. I went through the files and everything looks great on first glance including build & tests. But I'd like to take more time, to understand everything. Hope, this is ok.

miketaylr · 2017-07-28T18:27:23Z

(oops, forgot to push docs commit)

webcompat/templates/home-page/browse-issues.html

-      <% } %>
-      </script>
-      </div>
+      {% include "web_modules/issue-list.jst" %}


miketaylr · 2017-07-28T18:54:34Z

Just so it's not lost, here's an explanation of what this PR is trying to achieve:

#1682 (comment)

webcompat/static/js/lib/labels.js

-    ].join("")
-  ),
+  // relavant parts in issue/issue-labels.jst
+  subTemplate: wcTmpl["issue/issue-lables-sub.jst"],


zoepage · 2017-07-31T12:47:53Z

@miketaylr The performance improvement is really impressive! :)

I found a few small issues:

CSP issue:

Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src http://localhost:5000 https://www.google-analytics.com https://api.github.com”). Source: onfocusin attribute on DIV element.

Labels:

The placeholder for label is shown. When a label is added, it's not shown until a reload is fired manually. (See screenshots)

Anchor

http://localhost:5000/#contribute anchor directs not to the correct place as the issues are loaded later (from the all issues list on the index).

miketaylr · 2017-07-31T18:47:18Z

@zoepage thanks for digging around and finding those issues! That onfocusin one I'm 99% sure is just your adblocker or lastpass or similar. If you try it in a new profile that error won't be there. Will take a closer look at the rest.

miketaylr · 2017-07-31T19:29:19Z

OK, fixed the label editor issue (which also uncovered another small bug) and filed a follow up for the #contribute link (because that also exists in production): #1688

Just need to fix this package.json conflict.

…hem accordingly.

…ndant files as a result. Unfortunately the pre-compiled template approach won't let us rely on the jinja2 parser to do interesting things with JS templates.

…tions of my issues should use the same issue template.

…ath for convenience.

…licy.

…after modifying .jst templates.

zoepage · 2017-07-31T19:40:53Z

Sweet! Thank you :)

miketaylr · 2017-07-31T20:46:24Z

I just deployed this branch to staging if anyone wants to poke at it some more. Otherwise, will probably merge tomorrow.

zoepage · 2017-08-01T09:45:00Z

On stage (after posting an issue). Not just google analytics gets blocked but also the github api?

Things work, but just wanted to make you aware of this, IF this is an issue. :)

Also the function for the flash message ("Thank you for reporting, wanna share?") gets blocked, if I saw that right?

zoepage

I think, after fixing the flash messaged things should be good. :)

miketaylr · 2017-08-01T15:52:05Z

Things work, but just wanted to make you aware of this, IF this is an issue. :)

Investigating...

miketaylr · 2017-08-01T16:09:31Z

Investigating...

Yeah oops. Forgot about one last inline template. Fixed now!

miketaylr · 2017-08-01T16:49:35Z

💣s away... thanks for review @karlcow @zoepage!

miketaylr added status: do not merge in progress labels Jul 27, 2017

miketaylr commented Jul 27, 2017

View reviewed changes

webcompat/templates/issue/issue-labels-sub.jst Outdated

<%= label.name %>

</span>

<% }); %>

<script type="text/template">

This comment was marked as abuse.

Sign in to view

miketaylr commented Jul 27, 2017

View reviewed changes

webcompat/templates/issue/issue-labels.jst

<header class="wc-Issue-labelEditor">

<div class="wc-Issue-labelEditor-title">

Labels

</div>

{% if session.user_id and session.avatar_url %}

<% if ($("body").data("username")) { %>

This comment was marked as abuse.

Sign in to view

miketaylr commented Jul 27, 2017

View reviewed changes

webcompat/templates/web_modules/dropdown.html

@@ -1,3 +0,0 @@

{% macro dropdown(config) %}

This comment was marked as abuse.

Sign in to view

miketaylr requested review from zoepage and magsout July 27, 2017 22:46

miketaylr removed the status: do not merge label Jul 27, 2017

miketaylr changed the title ~~Fixes: #1679. Remove unsafe-eval directive from our CSP policy. [DO NOT MERGE]~~ Fixes: #1679. Remove unsafe-eval directive from our CSP policy. Jul 27, 2017

miketaylr force-pushed the issues/1679/1 branch from 36e636d to bac9ef4 Compare July 28, 2017 18:27

miketaylr commented Jul 28, 2017

View reviewed changes

webcompat/templates/home-page/browse-issues.html

<% } %>

</script>

</div>

{% include "web_modules/issue-list.jst" %}

This comment was marked as abuse.

Sign in to view

miketaylr force-pushed the issues/1679/1 branch from bac9ef4 to 98a5ba4 Compare July 28, 2017 18:53

miketaylr commented Jul 28, 2017

View reviewed changes

webcompat/static/js/lib/labels.js Outdated

].join("")

),

// relavant parts in issue/issue-labels.jst

subTemplate: wcTmpl["issue/issue-lables-sub.jst"],

This comment was marked as abuse.

Sign in to view

miketaylr force-pushed the issues/1679/1 branch from 98a5ba4 to b54168a Compare July 28, 2017 18:57

miketaylr force-pushed the issues/1679/1 branch from b54168a to aa6e98d Compare July 31, 2017 19:26

Mike Taylor added 4 commits July 31, 2017 14:33

Issue #1679. Add grunt-contrib-jst dependency to package.json

260c05b

Issue #1679. Add jst grunt task and generate a templates.js as a result.

ed3f430

Issue #1679. Move inline templates into their own files, and import t…

0e510cf

…hem accordingly.

Issue #1679. Handle authenticated check via JS.

2aa9490

Mike Taylor added 6 commits July 31, 2017 14:33

Issue #1679. Inline html includes into jst templates, and remove redu…

5206012

…ndant files as a result. Unfortunately the pre-compiled template approach won't let us rely on the jinja2 parser to do interesting things with JS templates.

Issue #1679. DRY: the diagnosis section of the homepage, and both sec…

e0cf9c4

…tions of my issues should use the same issue template.

Issue #1679. Rename tmpl to jst (to match configs), and use shorter p…

727f56a

…ath for convenience.

Issue #1679. Remove 'unsafe-eval' from script-src directive of CSP po…

5f4df15

…licy.

Issue #1679. Move subTemplate inline template into its own jst file.

a28e01d

Issue #1679. Update dev-env-setup.md to mention the need to re-build …

fa64824

…after modifying .jst templates.

miketaylr force-pushed the issues/1679/1 branch from aa6e98d to fa64824 Compare July 31, 2017 19:33

zoepage mentioned this pull request Aug 1, 2017

https://webcompat.com/#contribute link doesn't work as expected #1688

Closed

zoepage approved these changes Aug 1, 2017

View reviewed changes

Issue #1679. Add external .jst template for showThanks flash message.

697d8b5

miketaylr merged commit 2d77c21 into master Aug 1, 2017

miketaylr deleted the issues/1679/1 branch August 1, 2017 16:49

miketaylr added status: add to changelog and removed in progress status: add to changelog labels Aug 1, 2017

snyk-bot mentioned this pull request Nov 18, 2020

[Snyk] Security upgrade yargs from 13.3.2 to 16.0.0 sirinartk/webcompat.com#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes: #1679. Remove unsafe-eval directive from our CSP policy. #1682

Fixes: #1679. Remove unsafe-eval directive from our CSP policy. #1682

miketaylr commented Jul 27, 2017 •

edited

Loading

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr commented Jul 27, 2017

miketaylr commented Jul 27, 2017

miketaylr commented Jul 27, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr commented Jul 27, 2017

zoepage commented Jul 28, 2017

miketaylr commented Jul 28, 2017

This comment was marked as abuse.

miketaylr commented Jul 28, 2017

This comment was marked as abuse.

zoepage commented Jul 31, 2017

miketaylr commented Jul 31, 2017

miketaylr commented Jul 31, 2017

zoepage commented Jul 31, 2017

miketaylr commented Jul 31, 2017

zoepage commented Aug 1, 2017 •

edited

Loading

zoepage left a comment •

edited

Loading

miketaylr commented Aug 1, 2017

miketaylr commented Aug 1, 2017

miketaylr commented Aug 1, 2017

Fixes: #1679. Remove unsafe-eval directive from our CSP policy. #1682

Fixes: #1679. Remove unsafe-eval directive from our CSP policy. #1682

Conversation

miketaylr commented Jul 27, 2017 • edited Loading

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr commented Jul 27, 2017

miketaylr commented Jul 27, 2017

miketaylr commented Jul 27, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr commented Jul 27, 2017

zoepage commented Jul 28, 2017

miketaylr commented Jul 28, 2017

This comment was marked as abuse.

miketaylr commented Jul 28, 2017

This comment was marked as abuse.

zoepage commented Jul 31, 2017

CSP issue:

Labels:

Anchor

miketaylr commented Jul 31, 2017

miketaylr commented Jul 31, 2017

zoepage commented Jul 31, 2017

miketaylr commented Jul 31, 2017

zoepage commented Aug 1, 2017 • edited Loading

zoepage left a comment • edited Loading

Choose a reason for hiding this comment

miketaylr commented Aug 1, 2017

miketaylr commented Aug 1, 2017

miketaylr commented Aug 1, 2017

miketaylr commented Jul 27, 2017 •

edited

Loading

zoepage commented Aug 1, 2017 •

edited

Loading

zoepage left a comment •

edited

Loading