Update viewer-only earthengine repo to master branch of main repo as of 2013-10-22

[rsargent]

This reverts commit b7c6bd0.

[enguyen]

More about this commit from pdille, explaining a number of the diffs: "Around the time of the the final stages of the Landsat release, Yen-Chia, in parallel to everything going on, had gone through and completely redid the UI we use for our standard timelapses. The file that controlled this was timelapseViewer.js and then became defaultUI.js He also refactored the Landsat UI file to go along with how the UI was now being loaded. The original file was customTimeline.js and then it became customUI.js. This also explains why you see so many images being removed and tons of css changes, since we had an image based UI before.

Anyway, since this was so close to release I didn't want you, Eric, to deal with changing js/css includes on your embeds or worrying about why there are new files. I thought that was the last thing you wanted with your sleepless nights. So I left it as is. customUI.js was just a refactor and didn't introduce new fixes or new features, so it's not like you guess missed out on anything."

[enguyen]

Test line comment: Should these .iml and .ipr files also be .gitignored and removed from the repository?

[pdille]

Yes they should. But more importantly, they actually get removed down
the road. So you'll see them disappear once we get things up to the HEAD
of master.

On 3/31/2014 1:18 PM, Eric Nguyen wrote:

In TimeMachineExplorer.iml:

@@ -0,0 +1,9 @@
+
+

+

Test line comment: Should these .iml and .ipr files also be
.gitignored and removed from the repository?

—
Reply to this email directly or view it on GitHub
https://github.com/CMU-CREATE-Lab/timemachine-viewer/pull/1/files#r11125955.

[max99x]

Is this waiting on my review?

[pdille]

No, I believe Eric is doing it all this round. At least that's what he said at our last meeting.

[enguyen]

Yes, I'm taking a first look at this. Probably I'll take this round and
then we will split up the rest of the changes leading up to the present
state of things, which will be a larger chunk of work.

~e

On Fri, Apr 4, 2014 at 8:36 AM, pdille [email protected] wrote:

No, I believe Eric is doing it all this round. At least that's what he
said at our last meeting.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-39578471
.

[enguyen]

Hi, CMU team. Could you add some more orienting notes to this pull request? For example:

• We have taken this version of the source and have a running instance for your inspection at http://xxxx. It has a number of bugs (x,y,z) that were fixed in later commits, so don't worry about these.
• We can safely ignore *.iml, *.ipr, *.pl, cgi scripts, *.rb scripts, and bash scripts. These are all internal tools and some are removed in later commits.
• The images require only a cursory glance. We moved from an image-based UI, which is why so many are removed.
• One major change is the move from timelapseViewer.js to defaultUI.js, which a whole new scheme for rendering the UI. The new file is almost entirely different for XYZ reasons, which is why git didn't recognize the move.
• Another major addition is all the editor logic, which you can see in x.js, y.js, etc.
• Etc/

[enguyen]

Oh and another question I had was how all the example *.html files are related. Tnx!

[rsargent]

(And I'd also recommend in the notes calling out the files that won't be
used in Brassy like tour editing so Eric can skim or skip those)

On Fri, Apr 4, 2014 at 10:30 PM, Eric Nguyen [email protected]:

Oh and another question I had was how all the example *.html files are
related. Tnx!

Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-39626766
.

[pdille]

Here are some more detailed notes to go along with this commit:

Specific notes:

You can find a version of the viewer at this merge point live here.

/images/* (We moved from an image based UI to a fully css based one. It's probably not worth looking at them, but feel free.)

The *.html files you see in the root directory are either template files or example embed files. The examples were all renamed or moved to /tests or /examples at a later date. The templates remain in the root directory to this day. The template files are: player_template.html, time_warp_composer.html and annotation_editor.html

customTimeline.css became customUI.css
customTimeline.js became customUI.js

defaultUI.css was created and was populated from files like player.css or timelapse.css
timelapseViewer.js became defaultUI.js

• The reason github did not properly recognize this rename is because the new defaultUI.js also pulled in UI specific code that was in timelapse.js. I assume at this point it became too different for github to cope. This also explains why you see big blocks removed from timelapse.js

All the tour editor logic (both creating tours and viewing them) has always been part of the repo. There is a flag when embedding the viewer that enables/disables the tour editor UI from appearing and we choose to disable it on brassy. The core tour related code lives in snaplapse.js and snaplapseViewer.js.

Files removed at a later date

.iml
*.ipr
ark-earthquakes-2.json
/cgi-bin/
tour.css
videoset_stats.css
favicon.ico
fracked-ar2.json
iframe.html
/images/thumbs
js/jquery/plugins/carousel
emptyClass.js
flashViewer.js
tours.js
videoset_stats.js
/utils/*
save_time_warp.php

Files not used in brassy

VERSION (for our internal use)
annotation_editor.html (template for the annotation feature of the viewer)
annotator.css (css file for the annotation feature of the viewer)
annotator.js (js file for the annotation feature of the viewer)
visualizer.css (css file for a feature of the tour editor)
visualizer.js (js file for a feature of the tour editor
js/kinetic/* (specific to a feature of the tour editor)
integrated-viewer.html (specific to us and renamed to view.html. So it's view.html that you can ignore at a later date.)
canvasLayerRenderer.js (js file for giving the viewer the ability to overlay vector data)
*.rb (scipts used to update our ajax include files. We need to run them any time we update our templates or if the actual json info (number of levels, pano size, etc) for a dataset changes. We use these includes because Chrome cannot readily load json on file URL and IE 9 does not support CORS)

[enguyen]

Hi, team. TLDR: Focusing this review on security. Max, PTAL.

After contemplating a longish list of changes that I realized were going to be annoying to merge forward, I've decided that it makes sense to focus this review on security. We'll get my and Max's eyes on the timemachine codebase and get some certainty that what's committed on our end is secure. The next step is that we can merge forward any small changes and then Max and I can split up a monolithic review of your current state relative to a secure baseline.

Sound good? Okay, on to security review.

Overall, I think we're in good shape. I include all my notes below to indicate to Max (and you guys) what I looked at and how I was thinking so that we can look for more potential issues together. As you might expect, I'm looking for places where input from one user can be fed to another user, allowing script to be executed on their behalf. I explored two major possibilities.

1) Threats coming via cross-domain calls (embedded iframe)

Time machine instances can be embedded in an iframe, and we expose certain APIs. A vulnerability could allow an attacker-controlled parent page to execute script within the embedded iframe, i.e. in the context of one of our domains and with the end user's cookies/credentials.

First, I looked at postmessage.js. We use this to do cross-frame, cross-domain communication. It's a a fairly well-used open source project. However, it's not really used at Google, and the authors (who are here) told me that it's never been security reviewed. It may be prudent to use raw window.postMessage instead, since I believe it is supported on all platforms that thetimemachine client supports anyway (http://caniuse.com/#search=postmessage).

Next, I looked at how we sanitize user input generally. Luckily, we did most of this work in util.js in the pre-launch review. I've copied the last email from that long thread at the bottom, here. Essentially, util.js and a few other classes funnel user input through the getUnsafeHashVars() method and others like timelapse.unsafeViewToView(). Notes on this are at the bottom of this message. However, here are a bunch of fixes to make this better:

• All the comments for the timelapse.getXFromHash() methods describe unsafeHashVars as a string. It's an object, so let's fix the docs (which confused me) and maybe rename the params to "unsafeHashObj".
• timelapse.getModisLockFromHash() should indicate the type of data it is taking in, and ensure that the data is of that type. Is it a boolean? Cast it. Something else? Document and add checks.
• timelapse.getTourFromHash() and getPresentationFromHash() extract safe JSON, since their constituent urlStringToJSON calls go to carefully-designed methods that use strict encoders (and naming conventions to mark strings not strictly sanitized) to defang the URL input. So we're good here, but we should add short comments the getTour/PresentationFromHash() methods indicating that the output is safe.

Next, we need to make sure all data coming through the postmessage handlers is being properly sanitized. I looked at crossdomain_api.js for this:

• All of the inputs coming into the pm bindings should be marked unsafe (set-view, set-share-view, and goto-presentation-slide). E.g. the input param should be named "unsafe_data" per our conventions in util.js.
• "timemachine-set-view": Sanitize the view param using timelapse.unsafeViewToView(), as you do in the next binding. The doWarp and doPlay params can be sanitized as e.g. !!unsafe_data.doWarp. Note that doPlay doesn't seem to be supported in timelapse.setNewView(), though.
• "timemachine-set-share-view": We should parseFloat(unsafe_data.t). Also, l85 could be clarified to:

 if (unsafe_data.v) { 
   var view = timelapse.unsafeViewToView(unsafe_data.v.split(","));
   timelapse.setNewView(view, doWarp);
 }

• "timemachine-goto-presentation-slide": Using classes is kind of gunky ($.data() might be better?) and we should name the input param unsafe_slideTitle to remind future developers that this is dangerous input, but I don't see any attack here.

2) Threats coming in on load, via the URL itself

For both embedded and non-embedded instances of the timemachine player, attackers can also simply add data directly to the URL that the player will use on load. Most of this flows through the paths described in the section above, but I also took a look at all occurrences of window.location, window.top.location, and document.referrer. I think we're good here. The timelapse.getShareView() method and custom variants (e.g. viewer_animation_test.html) pull data from window.location, but only ever do so to populate a share URL dialog. These URLs will all necessarily be parsed by the sanitized util.js if another user opens them, so we should be safe here.

3) Anything else?

Other attacks I'm not considering?

Note that we'll be in much better shape once each of these classes of concerns can be expressed as a set of tests, but I'll reserve that for the full review, later.

---

Earlier Security review notes, for reference...

Eric Nguyen
To: Paul Dille
Cc: Randy Sargent

Hey Paul, thanks for your fixes on the security stuff. I still found myself without a fully clear path to trace security-review-wise, so I added some notes in commit 5ba090ed2dba1963ad376f57040a6a8f2c341e18. Here's how it looks now:

• The reviewer starts at getHashVars. The comment was nice, but what you need is for this information to travel wherever the method is called, so I renamed it getUnsafeHashVars.
  • This is called by timelapse.handleHashChange. I moved the getUnsafeHashVars call out so it only needs to execute once, and pass in a clearly marked unsafeHashVars param to each of the get*FromHash methods. So it now fans out to three more places, but the clarity for the reviewer is that you intend the buck to stop here with the naming convention. Those unsafeHashVars are passed into a couple methods, but out come safe vars.
  • getTimeFromHash is straightforward. There's that parseFloat right there :)
  • getViewFromHash requires going down another level since all those values are parsed. I made a bunch of var name changes to make this clear where the buck stops here as well, as per Earthengine pull request 2014 06: update earthengine-head to Time Machine head 2014-06-04, commit 694299ffbbfb2471fbc2ec55113314f017ede27e (plus changes TBD from review) #2.
  • getTourFromHash is complicated, but we handled that security situation earlier by clearly delineating where in that system dangerous data is likely to exist (i.e. unsafe_string_*)

Hope that makes sense. Again, just being detailed so you understand why all these changes are being made and so that you can model this thinking next time.

~e

[max99x]

I see no obvious security issues (ignoring the files not used by Brassy and/or removed later).

Also, Eric, I don't see the parts you discuss in this particular pull request (e.g. postMessage is not called anywhere that I can see).

[enguyen]

Max: I should have clarified. There were earlier changes that I felt
deserved scrutiny, now that the source here is about to be more public. So
my comments apply to the whole client codebase -- the most recent changes
are just a subset. If you can take another look with that lens, I'd
appreciate it.

On Sat, Apr 19, 2014 at 7:00 PM, Max Shawabkeh [email protected]:

I see no obvious security issues (ignoring the files not used by Brassy
and/or removed later).

Also, Eric, I don't see the parts you discuss in this particular pull
request (e.g. postMessage is not called anywhere that I can see).

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1#issuecomment-40885741
.

[pdille]

FYI, I pushed changes to master based on your security comments, Eric. I wasn't in disagreement on any points.

You'll see a link to the commit right above this post.

[enguyen]

Added comments to the commit, Paul. Mostly minor changes and some confusion around how unsafeViewToView works. Also, I forget where we left the discussion around using raw window.postMessage instead of the un-reviewed library. I don't see any reference to that in this commit.

[enguyen]

Just discussed offline: Paul is still working on a small wrapper for raw postMessage.

[pdille]

Here's the commit that replaces the postmessage library we were using with our own wrapper on the postMessage protocol. No hacks/fallbacks for older browsers (since they can't use our viewer anyway) and should thus be easier to review. It should be a drop in replacement and keeps the same API that the library was using. e92347e

[pdille]

The next commit above should cover the rest of the comments you made in regards unsafeViewToView and other minor things.

[enguyen]

Okay, I only had the one comment on e92347. Otherwise, the postMessage stuff looks good. To test the specific case we will have in the wild with existing installs of Timelapse, we should run an instance with the old version of postmessage.js on the parent page and the new timelapse client (with this new version of postmessage.js) in the iframe.

The other minor commits around unsafeViewToView, etc. also all look good! So once you address that one comment mentioned above, let's call this a wrap!

[pdille]

Sent email about a test page for testing backwards compatibility of embeds with postmessage. A login is required, and though not a loss if it gets outs, I just don't want it on a public page like this.