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

Removes uses of HTML injection in report #1226

Merged
merged 4 commits into from
Jan 3, 2017
Merged

Removes uses of HTML injection in report #1226

merged 4 commits into from
Jan 3, 2017

Conversation

ebidel
Copy link
Contributor

@ebidel ebidel commented Dec 21, 2016

R: @brendankenny @patrickhulce all

Fixes #1188.

This PR:

  • removes the dangerous uses of {{{}}}, but keeps the ones where we control the input ({{{css}}} and {{{script}}}).
  • Adds marked as a dependency. This module is lightweight (<30kb) and produces the necessary sanitization we need to escape unwanted HTML that gets loaded in the viewer/report.
  • Moves the audits to use markdown snippets rather than HTML snippets. Makes helpText and descriptions shorter and easier to author.
  • An added benefit is that we can now use markdown all over the place (aggregation descriptions, audit descriptions, help text). Just need to use {{sanitize variableName}}.

@kaycebasques for the FYI as this changes audit help text from html to using markdown.

@@ -62,15 +63,6 @@ class ReportGenerator {
return name.toLowerCase().replace(/\s/, '-');
});

// Helper for either show an "✘" or "✔" booleans, or simply returning the
// value if it's of any other type.
Handlebars.registerHelper('getItemValue', value => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we were no longer using this.


// eslint-disable-next-line no-unused-vars
Handlebars.registerHelper('sanitize', function(str, opts) {
// const isViewer = opts.data.root.reportContext === 'viewer';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kept this to remember how to do one offs for different contexts (possible future stuff)

@patrickhulce
Copy link
Collaborator

nice this will be so much better! need to talk to youtube guy about the dependency?

@ebidel
Copy link
Contributor Author

ebidel commented Dec 29, 2016 via email

@ebidel
Copy link
Contributor Author

ebidel commented Dec 29, 2016

@patrickhulce want to approve if LGTU?

@ebidel ebidel added the p0 label Dec 30, 2016
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry!

@ebidel
Copy link
Contributor Author

ebidel commented Jan 3, 2017

Landing this so the next release has the security improvements and others can move forward with audit changes without running into rebase issues.

@ebidel ebidel merged commit 83c4915 into master Jan 3, 2017
@ebidel ebidel deleted the notriplebrackets branch January 3, 2017 18:47
@paulirish
Copy link
Member

marked appears to add just 27kb to the final bundle size. That seems good to me!

image

👍

@ebidel
Copy link
Contributor Author

ebidel commented Jan 3, 2017

Yep, very small. That could potentially come down further if we tree shake some unused features out.

@paulirish
Copy link
Member

Nice. Got an experimental branch up where we do minifying and and viewing this size report: https://github.com/GoogleChrome/lighthouse/compare/minified-sizereport

andrewrota pushed a commit to andrewrota/lighthouse that referenced this pull request Jan 13, 2017
* Beginning to use markdown in snippets

* Allow links and code snippets

* Update audits to use .md instead of html

* Add tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants