Open in Lighthouse Viewer button #1179

ebidel · 2016-12-19T20:59:28Z

R: @paulirish @patrickhulce @brendankenny

This adds a button to the nav. When clicked, the local report opens the viewer ( http://googlechrome.github.io/lighthouse/viewer/) in a new tab and postMessage's the report json to it. The online viewer has a message listener that responds and constructs the page. This functionality will encourage folks to use the online viewer (where we have save as gist, shareable links, export functionality, etc.) It may also be a solution for the DevtTools integration.

On http://googlechrome.github.io/lighthouse/viewer/, the "view online" button will be disabled:

On html reports generated by the CLI or the extension, the button will be enabled:

Security concerns were addressed in #1188 by removing {{{}}} usage of user-provided content.

brendankenny

this makes me nervous :) especially since we can't really control the origin of the report. An XSS could conceivably let someone create gists in your account if you've already given permission, though that would probably rely on a handlebars exploit or one of our formatter helpers being stupid.

I don't really want to rely on us never writing stupid code, though. Should we write a lighthouse results object validator that strips out everything that doesn't match it?

brendankenny · 2016-12-20T20:08:48Z

lighthouse-core/test/report/report-test.js

-              'lighthouse-report.js was not inlined');
-    assert.ok(html.includes('.report-body {'), 'report.css was not inlined');
-    assert.ok(!html.includes('&quot;lighthouseVersion'), 'lhresults were not escaped');
+    assert.ok(html.includes('window.lhresults = '), 'report results were inlined');


do you want to test more than this? (though you do test for lighthouseVersion, below?) In the past we had a failure for a while where window.lhresults = undefined; was passing the test

I'll check that there's an object on the page, but I don't think we should get in the business of validating the object as it's embedded on the page.

brendankenny · 2016-12-20T20:19:54Z

lighthouse-viewer/app/src/lighthouse-report-viewer.js

+   */
+  listenForMessages() {
+    window.addEventListener('message', e => {
+      if (e.data.lhresults) {


not a huge increase in security, but can we check the message origin/source against the opener?

brendankenny · 2016-12-20T20:20:38Z

lighthouse-viewer/app/src/lighthouse-report-viewer.js

+    });
+
+    // If the page was opened as a popup, tell the opening window we're ready.
+    if (self.opener && !self.closed) {


self.closed? or checking the opener as closed?

opener. done.

brendankenny · 2016-12-20T20:25:00Z

lighthouse-viewer/app/src/lighthouse-report-viewer.js

+
+    // If the page was opened as a popup, tell the opening window we're ready.
+    if (self.opener && !self.closed) {
+      self.opener.postMessage({opened: true}, '*');


again, doesn't do much, but don't we know the origin here?

Unfortunately, you can't access opener.origin if the opener is cross domain.

ebidel · 2016-12-20T23:53:36Z

PTAL.

An XSS could conceivably let someone create gists in your account if you've already given permission, though that would probably rely on a handlebars exploit or one of our formatter helpers being stupid.

If you can exploit the viewer like this, it's already possible since we allow drag and drop of a user file into the app. This additional also gores through our ReportGenerator.generateHTML, which goes through Handlebars under the hood. In most places we use {{ }} in the HTML template to prevent script injection. That goes through http://handlebarsjs.com/reference.html#base-escapeExpression by default, so we should be good.

The places where we use{{{ }}} to render user content should be replaced by something safer. I think we should do that in a followup PR because that's tangential to these changes and a broader change we need to make. I'd also like to write tests one to handle both cases.

Update: filed #1188 to remove {{{ }}} usage.
Update: #1188 is fixed in #1226.

ebidel · 2016-12-21T19:56:13Z

@brendankenny rebased and passing.

ebidel · 2017-01-10T20:33:32Z

Pingy ping. This has been open for 22 days.

I think we should start with this before upstreaming viewer code into core (future PR). This PR is useful standalone.

paulirish

From a UX pov I have been thinking of something slightly different:

User runs lighthouse
They view the HTML report (locally)
They want to save or share this particular report
They click a "generate link" kind of button.
This will open a new tab with the production report-viewer
(User authenticates if its their first time)
After communicating progress, the URL of the page is updated to now updated to the new permalink
A "Copy link" link is available to automate adding to clipboard.

it's a bit of a change, so how does that UX sound to you?

paulirish · 2017-01-10T21:40:17Z

lighthouse-core/report/templates/report-template.html

+{{#if script }}
+<script>
+window.lhresults = {{{lhresults}}};
+


let's put the lhresults in a separate script tag from the {{{script}}}.

brendankenny · 2017-01-10T22:34:58Z

is there a reason to stick with the postMessage solution if we're going to pull in the save to gist mechanism from Viewer into the report? Then it just becomes save button -> upload -> update link to permalink

?

paulirish · 2017-01-10T22:38:24Z

@brendankenny i think there's two parts to that

that wont work on file://. Undetermined if that's a problem we care about
we need to validate the auth flow is possible & fine from a (arbitrary) localhost post

ebidel · 2017-01-10T22:55:51Z

@paulirish I think this will capture that entire workflow. The only thing that we'd have to change is:

4.) is "open in viewer" instead of "generate link"...but we could do a thing where if you click the share button inside a local report, it goes through your flow.
6.) this is manual right now but we could auto-detect 4. and automatically auth the user (if they're not already).

@brendankenny postMessage allows us to view json results from other locations like the devtools.

brendankenny · 2017-01-10T22:59:48Z

lighthouse-viewer/app/src/lighthouse-report-viewer.js

+   * Initializes of a `message` listener to respond to postMessage events.
+   */
+  listenForMessages() {
+    window.addEventListener('message', e => {


remove listener after a report loaded?

I can see cases where we'd want the same page to update itself again. For example, when the DT opens the tab, it could re-postMessage() to the existing open tab.

ebidel · 2017-01-11T02:17:30Z

Rebased

brendankenny

one more XSS thought, but except for better escaping this approach is looking good. It would be great to get another reviewer looking at safety before landing, though :)

brendankenny · 2017-01-12T00:54:13Z

lighthouse-core/report/scripts/lighthouse-report.js

@@ -19,9 +19,34 @@

 'use strict';

+function sendJSONReport() {


add a function description comment?

brendankenny · 2017-01-12T02:02:23Z

lighthouse-core/report/templates/report-template.html

+
+{{#if scripts }}
+<script>
+  window.lhresults = {{{lhresults}}};


would it be totally crazy to do even more defense in depth here by putting lhresults in an html element instead of in a <script>?

Really we should be doing more to escape our JS, but one end run around this is to do double stash {{lhresults}} in a (display: none) div, so we get HTML entity escaping, then get the actual results when needed by JSON.parse(lhresultsDiv.textContent) (see rule 1 and the summary section for more details)

Ah, so to be clear, these <script>s (and therefore the {{{}}}) never see user-provided input.

The flow is:
User clicks "open in viewer", the local page opens the online viewer (a page with different markup) and sends the window.lhresults JSON to it. That page's message listener does the replaceReportHTML. That method then goes through our normal Handlebars pipeline to generate the inner page content.

right, but lhresults does see user input (which is why we have to escape any </script> in it).

This suggestion is actually independent of the postMessage part of this PR, and just about (safely) embedding lhresults in the page again. Basically their advice is that if you're going to put user generated data in a page, an easy way to keep it safe is doing html escape (which the double handlebars stash does) inside an HTML element. If you want to embed the user data in a JS var literal instead there's a lot more work to do (and it doesn't appear that handlebars offers that escaping by default).

This would also let us drop our own </script> escaping and it would leave us in a lot safer spot wrt exploits of the postMessage system, however unlikely those are, and the change doesn't seem semantically that bad.

Really we should be doing more to escape our JS, but one end run around this is to do double stash {{lhresults}} in a (display: none) div, so we get HTML entity escaping, then get the actual results when needed by JSON.parse(lhresultsDiv.textContent) (see rule 1 and the summary section for more details)

I'm not sure this does anything different other than adding an additional parse. Handlebars, #1188, and #996 already force entity escaping everywhere that's needed.

The lhresults object is also constructed and sanitized by us. It's not pure user-input. Parts of the object come from user-space but we only allow markdown (two tags at that). If the user provides HTML tags, they'll end up encoded.

This suggestion is actually independent of the postMessage part of this PR, and just about (safely) embedding lhresults in the page again.

The thing we're trying to prevent is a malicious actor creating gists in a user's account. That all happens online. Locally, I'm not sure what there is to worry about? A malicious actor would have to create a page such that running LH over it properly escapes the embedded JSON (which we prevent), alters lhresults to their advantage, the user clicks "share in viewer", the rev'd lhresults is able to escape the online viewer's {{}}, passes additional JSON.parse() and DOM construction calls, then has code to automatically call createGist() (we don't provide a way to call this; creating a gist is behind user gesture atm). The also needs to already have an oauth token.

If something were to ahppen, our oauth tokens are limited scoped. They can only create gists in the users account. They're also secret by default (for extra feel goods) and can be revoked by the user at any time.

The theme of this PR is to encourage users to share reports. Given the current low volume of sharers, let's land this so we can grow some us LH users!

The thing we're trying to prevent is a malicious actor creating gists in a user's account

Ideally we should prevent them from doing anything, of course. It's better that way but it also helps us down the line as we add new features, things get more complicated, etc. lhresults is still used in the extension's report, and while it's stripped out in replaceReportHTML() right now, it's possible we'll want to use it at some point in the future.

I'm not sure this does anything different other than adding an additional parse.

It is an additional parse, but it means the only cost is an additional parse. Meanwhile everything is escaped and no matter the user generated content it won't be able to break out. If we assign the object in js we're doing an eval with an optional innerHTML if it can close the element, with all the danger that entails. JS escaping is fairly complicated; this seems like an easy way to make sure we have it right (at least as a defense-in-depth thing with the rest of our efforts).

Handlebars, #1188, and #996 already force entity escaping everywhere that's needed.

The various other template entries are protected, but {{{lhresults}}} is raw and only protected by escaping the closing </script> tag from #996. Unless we're very careful it's possible someone will be able to do the double escape trick to get those back (or try one of the various other tricks in that doc) with our current system or in the future. Handlebars and the markdown change don't touch lhresults since it's using the triple stash, and we're adding new user content all the time (e.g. the new CSS usage audit, which adds verbatim quotes from stylesheets in its extendedInfo).

Given the current low volume of sharers, let's land this so we can grow some us LH users!

Totally agree. The main thing is that this is a fairly trivial change with no real overhead (semantically or performance-wise). The escaping that needs to happen is small in scope and already handled by Handlebars. It keeps us to the guarantee that the only things in {{{}}} is content completely generated by LH itself.

It's really just

<div id="lhresults">{{lhresults}}</div> {{#if scripts }} <script> // Use escaped results object to protect against XSS from result details. window.lhresults = JSON.parse(document.querySelector('#lhresults').textContent); </script> {{/if}}

(with display: none in the css) and everything else acts as it does now with no other changes.

Really not trying to be argumentative, this just seems like an easy way to secure us for the future :)

I also prefer putting the lhresults in a non-script element. How about <template>?
TBH when i was first trying out the lhresults, it created some html elements from various descriptions. I think those things are addressed now, but i feel safer not throwing fairly arbitrary text into a script tag.

I kept the _escapeScriptTags. Defense in depth-n-stuff.

brendankenny

Nice!
📜
🔽
✉️ ➡️ ☁️

ebidel added the viewer label Dec 19, 2016

brendankenny requested changes Dec 20, 2016

View reviewed changes

ebidel force-pushed the postmessage branch from 6624b9a to 7825e51 Compare December 21, 2016 19:35

ebidel force-pushed the postmessage branch 3 times, most recently from 27920af to 352149d Compare January 9, 2017 23:17

paulirish reviewed Jan 10, 2017

View reviewed changes

ebidel added 4 commits January 10, 2017 14:33

Open in Lighthouse Viewer button

5e092e1

no longer need the var

45f3b6d

Feedback

8ec57d0

Remove unstable test

145a8ff

ebidel force-pushed the postmessage branch from 352149d to 145a8ff Compare January 10, 2017 22:33

separate scripts

8254973

brendankenny reviewed Jan 10, 2017

View reviewed changes

ebidel added 2 commits January 10, 2017 17:52

Merge branch 'master' into postmessage

2dbcaa1

Update failing test

22655b0

brendankenny requested changes Jan 12, 2017

View reviewed changes

ebidel added 3 commits January 11, 2017 19:15

feedback

8212609

Double down escape evade boom boom bad

f000bc4

better comment

86ccb72

brendankenny approved these changes Jan 13, 2017

View reviewed changes

brendankenny merged commit ec97c35 into master Jan 13, 2017

brendankenny deleted the postmessage branch January 13, 2017 05:13

andrewrota pushed a commit to andrewrota/lighthouse that referenced this pull request Jan 13, 2017

Report: open in Lighthouse Viewer button (GoogleChrome#1179)

0269b80

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Open in Lighthouse Viewer button #1179

Open in Lighthouse Viewer button #1179

ebidel commented Dec 19, 2016 •

edited

Loading

brendankenny left a comment

brendankenny Dec 20, 2016

ebidel Dec 20, 2016

ebidel Dec 20, 2016

brendankenny Dec 20, 2016

ebidel Dec 20, 2016

brendankenny Dec 20, 2016

ebidel Dec 20, 2016

brendankenny Dec 20, 2016

ebidel Dec 20, 2016

ebidel commented Dec 20, 2016 •

edited

Loading

ebidel commented Dec 21, 2016

ebidel commented Jan 10, 2017

paulirish left a comment

paulirish Jan 10, 2017

ebidel Jan 10, 2017

brendankenny commented Jan 10, 2017

paulirish commented Jan 10, 2017

ebidel commented Jan 10, 2017

brendankenny Jan 10, 2017

ebidel Jan 10, 2017

ebidel commented Jan 11, 2017

brendankenny left a comment

brendankenny Jan 12, 2017

brendankenny Jan 12, 2017

ebidel Jan 12, 2017

brendankenny Jan 12, 2017 •

edited

Loading

ebidel Jan 13, 2017

brendankenny Jan 13, 2017

brendankenny Jan 13, 2017

paulirish Jan 13, 2017

ebidel Jan 13, 2017

brendankenny left a comment

Open in Lighthouse Viewer button #1179

Open in Lighthouse Viewer button #1179

Conversation

ebidel commented Dec 19, 2016 • edited Loading

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel commented Dec 20, 2016 • edited Loading

ebidel commented Dec 21, 2016

ebidel commented Jan 10, 2017

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Jan 10, 2017

paulirish commented Jan 10, 2017

ebidel commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel commented Jan 11, 2017

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

ebidel commented Dec 19, 2016 •

edited

Loading

ebidel commented Dec 20, 2016 •

edited

Loading

brendankenny Jan 12, 2017 •

edited

Loading