ucla-oarc-mobile / mwf

UCLA Mobile Web Framework
http://mwf.ucla.edu
Other
86 stars 25 forks source link

Device Telemetry Stack on Separate Domain #6

Closed ebollens closed 12 years ago

ebollens commented 12 years ago

When the MWF does it's refresh to pass telemetry from client to server on the first page load, the cookie definition is occurring against the content provider host, not the service provider host.

This bug doesn't manifest for service providers who use the front splash page, because the write goes against the service provider and then stays set throug other pages. However, if you're running this in an environment where the framework is completely separate from even the front page, the cookie is set to the wrong place.

ghost commented 12 years ago

I presume all service provider and content provider will have the same domain (i.e. ucla.edu) and the cookie will be set at the domain level with path = '/'. We're setting that limitation for js and css loading and images compression anyways, right?

ebollens commented 12 years ago

Ike, in terms of our "federation" principle, that assumption isn't necessarily true - or hasn't been documented as a requirement if we want to require that. Given that we're headed towards a SaaS model, I don't think forcing this is a reasonable choice. Therefore, I'm going to look into getting this resolved by our next milestone.

ebollens commented 12 years ago

It appears there are two options for delivering the cookie to the Service Provider domain:

  1. Setting a third-party cookie for the service provider using the domain cookie attribute.
  2. A double redirect through the service provider and back to the content provider.

Given the implications of redirects, minimizing these is good, so (1) takes priority. However, need to figure out a way to determine if (1) is failing, and in that case try to take advantage of (2) instead. The reason for this is that many browsers allow you to prevent third-party cookies and, given the increasing fears of being "tracked" on the internet, I think that this setting will become more prevalent, not less.

ebollens commented 12 years ago

To resolve this issue, it will be necessary to rework '''mwf.override''' and '''mwf.server'''.

The new proposed process is outlined as follows: http://erb1.ats.ucla.edu/upl/DTS-v2.pdf

Essentially, it will do the following:

  1. Service Provider will define in vars.php if it already has the necessary cookies.
  2. If it does not, script running on content provider will determine cookie support.
  3. If no cookie support, then script concludes without a redirect.
  4. If on the first page load and likely to support third party cookies, will try to write and reload.
  5. If not on the first page load or not able to support third party cookies, will redirect to the service provider.
  6. On redirect to service provider, cookies will be set in domain and then user redirected back.

In the linked diagram, there's also a path for override.

There are a few caveats here:

As should be noted, this keeps current behavior the same while supporting the separate domain case, which is what this bug report is about. Please let me know if anything seems inconsistent.

ghost commented 12 years ago

The new flow looks good. Just throwing another idea out thereŠ Any concerns about using window.top.name?

http://www.thomasfrank.se/sessionvars.html

Any script on the page can read and write the data and the data persists as long as the window/tab is open. But since we're not storing any sensitive information -- I'm not too concerned about it. It could be a good fall back to cookie. And if the data in window.name is not we expected (meaning other script is using it), we just do the redirect.

On 10/31/11 9:39 AM, "Eric Bollens" reply@reply.github.com wrote:

To resolve this issue, it will be necessary to rework '''mwf.override''' and '''mwf.server'''.

The new proposed process is outlined as follows: http://erb1.ats.ucla.edu/upl/DTS-v2.pdf

Essentially, it will do the following:

  1. Service Provider will define in vars.php if it already has the necessary cookies.
  2. If it does not, script running on content provider will determine cookie support.
  3. If no cookie support, then script concludes without a redirect.
  4. If on the first page load and likely to support third party cookies, will try to write and reload.
  5. If not on the first page load or not able to support third party cookies, will redirect to the service provider.
  6. On redirect to service provider, cookies will be set in domain and then user redirected back.

In the linked diagram, there's also a path for override.

There are a few caveats here:

  • The '''mwf.capability.cookie()''' check needs to be 100% accurate, or at least 100% averse to false positives. If it claims that cookies are supported when in fact they are not, this will cause an infinite redirect. I have yet to determine a way to ensure that this sort of a redirect does not occur consistently. One option, that would at least alleviate the infinite-ness of it, would be to pass a GET parameter back into the content provider that signals its not the first page load. This would cause every page load to require two page loads, so still a bad use case, but better than unlimited.
  • It's important we don't try to set a third party cookie when we cannot. This setting requires a redirect, and if it isn't successful, then we still have to do two more redirects. However, we also want to minimize the case where we don't try third party cookies when they are viable, as the non-third-party method incurs an additional HTTP request in comparison to the third-party method.
  • This implementation inherently supports the same-domain case, as it's simply writing a first-party cookie instead of a third-party cookie, and in this case, it should simply use a single redirect. This is the case supported by the current implementation of DTS, and keeping the control path as such keeps its behavior consistent with what is already in DTS.

As should be noted, this keeps current behavior the same while supporting the separate domain case, which is what this bug report is about. Please let me know if anything seems inconsistent.

Reply to this email directly or view it on GitHub: https://github.com/ucla/mwf/issues/6#issuecomment-2579395

ebollens commented 12 years ago

Ike, this is an incredible find. I'd rather rely on cookies than not, but I think we can incorporate it in two ways:

  1. Help us prevent the infinite loop problem if mwf.capability.cookie() is false positive.
  2. As you mentioned, a fallback for cookies.

Very cool hack.

Trott commented 12 years ago

The window.name hack is very cool, but I'm not entirely clear on something:

Isn't the point of the cookies to make it so that the server side stuff has access to the telemetry data? How is the server side going to see the window.name values? I guess JavaScript could write them into a bunch of GET params and some custom PHP on the server side could write that to SESSION variables or something like that, all AJAX-style. Except that SESSION probably requires a cookie.

But I'd add that if we're going to go that route, then we might consider doing the same thing (perhaps with HTML5 Local Storage and/or Session Storage for iPhones and Android instead of the window.name hack) no matter what and dispense with cookies altogether. Or maybe not. Perhaps I'm just confusing the issue. But it might be nice to not have to send around all those cookies with every HTTP request. Of course, PHP SESSION stuff probably works via cookies. But it's probably one very small cookie compared to three.

ebollens commented 12 years ago

Rich, the point of this storage is to let us know if we've already loaded the page - to prevent a false positive infinite loop on a browser that doesn't support cookies but is identified as though it does. I'm going to be implementing a "strict" mode for cookie checking that is a lot more stringent, though a bit more costly in terms of performance, in order to minimize the false positive, but because of the outcome of the false positive, any method to make sure we don't infinite loop is important.

Trott commented 12 years ago

Ah, that totally makes sense now, although depending on how the implementation plays out, a URI parameter might (or might not) fit the bill a little more elegantly than the window.name hack.

ebollens commented 12 years ago

I have committed an initial overhaul of mwf.server: 0e6c25ce31d069595b910da14cbd96c6523e43dc

This tries to use the third-party method, and falls back to redirecting through a script that then passes the visitor back to the originating page. This has a cost of 1 HTTP request if same-origin as framework or third-party write works, 2 HTTP requests if no third-party support is guessed correctly and pass through is used, and 3 HTTP requests if third-party write is thought to work but fails and then it has to do a passthru.

Based on my experimentation, a couple questions:

  1. It does not seem that many desktop browsers support third-party cookies. Mobile Safari for iOS does not either. Do we know if Android does? If it doesn't, it might make sense to strip out the third-party cookie portion, as a bad guess is an expensive failure, requiring an additional load.
  2. I'm really not sure what to do with caching anymore. The js.php file isn't cacheable on first load anymore, because js/core/vars.php dynamically defines a set of variables. However, not caching it at all long-term sucks, though if the cookies expire on the server, we need to know. So what's the right approach here? One thing I was thinking is potentially a dynamic write of another script tag that loads everything except the core stack in a separate js file that is cacheable. However, again, it isn't actually fully cacheable because it accepts parameters that change its contents. Thoughts?

The next steps are as follows:

Please let me know your thoughts. This is the beginning of a pretty large shift in the paradigm.

As a note, it does follow the flowchart posted earlier in this topic.

Trott commented 12 years ago

I recommend drinking the TDD Kool-Aid (it's delicious!) and writing unit tests for the new JavaScript methods.

Trott commented 12 years ago

I'm on it for the unit tests for the JS stuff you've written thus far so don't worry about that stuff.

Trott commented 12 years ago

Might it be better to send 0s and 1s for the settings in the classification cookie rather than the entire "true" and "false" strings that we're sending? These cookies get sent on every HTTP request, so one could make the argument that every byte counts. (We also might want to abbreviate things like we do in the other cookies so f for full and s for standard?)

ghost commented 12 years ago

Android doesn't support third-party cookies either. I'm in favor of getting rid of that portion.

As far as caching is concerned, here's another blue sky idea that is similar to what you are proposing:

mwf.js does the detection and reloads if first time visiting. If not the first time visiting (no cookie or no window name), write another script tag to load js.php with the appropriate classification params. Since the params don¹t change, js.php will be cached.

If javascript is turned off, then just load the basic css.

I realize this is a dramatic departure from our current approach. But I'm just throwing out ideas here perhaps for future reference.

On 11/2/11 9:50 AM, "Eric Bollens" reply@reply.github.com wrote:

I have committed an initial overhaul of mwf.server: 0e6c25ce31d069595b910da14cbd96c6523e43dc

This tries to use the third-party method, and falls back to redirecting through a script that then passes the visitor back to the originating page. This has a cost of 1 HTTP request if same-origin as framework or third-party write works, 2 HTTP requests if no third-party support is guessed correctly and pass through is used, and 3 HTTP requests if third-party write is thought to work but fails and then it has to do a passthru.

Based on my experimentation, a couple questions:

  1. It does not seem that many desktop browsers support third-party cookies. Mobile Safari for iOS does not either. Do we know if Android does? If it doesn't, it might make sense to strip out the third-party cookie portion, as a bad guess is an expensive failure, requiring an additional load.
  2. I'm really not sure what to do with caching anymore. The js.php file isn't cacheable on first load anymore, because js/core/vars.php dynamically defines a set of variables. However, not caching it at all long-term sucks, though if the cookies expire on the server, we need to know. So what's the right approach here? One thing I was thinking is potentially a dynamic write of another script tag that loads everything except the core stack in a separate js file that is cacheable. However, again, it isn't actually fully cacheable because it accepts parameters that change its contents. Thoughts?

The next steps are as follows:

  • (Others) Provide feedback on the above commit and questions.
  • (Me) Develop the override support for this functionality.

Please let me know your thoughts. This is the beginning of a pretty large shift in the paradigm.

As a note, it does follow the flowchart posted earlier in this topic.

Reply to this email directly or view it on GitHub: https://github.com/ucla/mwf/issues/6#issuecomment-2605978

ebollens commented 12 years ago

Rich:

Ike:

All, as a note, this will be a discussion during our meeting on Friday.

Trott commented 12 years ago

This is a side issue and totally not the place to have a lengthy conversation about it, plus since it's arguing with Eric I expect to get thoroughly spanked, but just for the record: I believe that the programmer writing the code should ideally write the unit tests. See anything ever written on TDD or, for an abbreviated take, http://stackoverflow.com/questions/5154371/who-should-write-tests. Functional tests? Yeah, someone else. Unit tests: Totally the programmer writing the code, word is bond.

But back to the much more pressing and important issue of what to do with this cross-domain telemetry stuff: Yikes. I said, yikes. I'll try to think about this some more tonight, but my head hurts.

ebollens commented 12 years ago

On testing, Rich wins ;)

As for development, I think we all win (at least preliminarily):

Some pretty hefty stuff in there to accommodate the cross-domain functionality, but honestly, rewriting mwf.server and mwf.override has actually had the effect that I think they're a bit more clear than they used to be. Could probably still be improved in clarity, mostly by way of a state diagram - pretty tricky keeping track of where each thing is given the way that classification, override and server cookies are all linked to each other.

Please check this out and let me know how its working for you all, as well as provide feedback.

We'll discuss it during tomorrow's meeting as well.

Trott commented 12 years ago

I wrote a few more unit tests, but in commit c19e220c69063d3d43a731f8e3ba3f725206269b, I put in 7 unit test stubs that need writing. They are all marked with @todo. Any chance you could bust them out?

ebollens commented 12 years ago

Over capacity with work through the weekend I'm afraid - but will work on them early next week if needed - at least in time for the upcoming release.

Trott commented 12 years ago

I hear if you get within 30 meters of a hookah bar, that you get super-human coding powers.

ebollens commented 12 years ago

Hmmmm... I'm thinking one may be in need after the developer's meeting!

Trott commented 12 years ago

Oh, wait, it's not super-human coding powers. You just lose your ability to fly. Sorry, I got confused.

Trott commented 12 years ago

Any idea when you intend to merge feature/dts into develop-1.2 so that people can start integration testing in earnest ahead of the Wednesday release? Any reason not to go ahead and do that now-ish?

ebollens commented 12 years ago

I had hoped to get more weigh in from people before merge on Crucible.

Unfortunately, this hasn't been the case.

Therefore, yeah, merge to develop-1.2 is now done in cc581ab.

Trott commented 12 years ago

Sorry you didn't get the level of feedback you were hoping for. I'll see about giving this some good testing now.

Trott commented 12 years ago

Can we clarify what we mean by separate domain?

The comment for the cookie_domain setting says "The domain where the framework resides - this is REQUIRED for any framework service provider that has content providers leveraging it from different domains."

The term "domain" is vague in this context. Would m.ucsf.edu be a different domain from www.ucsf.edu? Or is it all good for everyone on ucsf.edu domains and I only need to set the setting if I want content providers from ucsfmedicalcenter.org to use the framework instance installed on m.ucsf.edu?

ebollens commented 12 years ago

Cookie domain is the domain that the cookie should define as "same-origin".

The cookie is written through document.cookie on the framework server, so your origin is the domain of that cookie.

Thoughts on how to document this correctly?

Trott commented 12 years ago

Examples would probably be the most helpful. Two things in particular:

1) When to use. Which of these are valid use cases for this?

A) If I want www.example.com and m.example.com to both be able to read the MWF cookies served by m.example.com, do I need to set this value?

B) How about example.com and m.example.com?

C) Will it work for example.org or frobozz.biz and m.example.com?

2) What to set it as:

A) "example.com" B) ".example.com" C) "m.example.com"

ebollens commented 12 years ago

Rich, if the framework runs on m.ucsf.edu, then valid domains are:

m.ucsf.edu ucsf.edu

Generally, we'd advise the "tighter" one and not the looser one, as it limits the poisoning surface.

I'm thinking then that we want to say something along the lines of:

"cookie_domain" - A valid domain for the cookies that the framework sets. This must be the exact subdomain, or any higher-level domain, that the framework resides under. For example, if the framework resides at m.example.com, it can be set to m.example.com or example.com.

That said, though, this documentation does bring up another point: in reality, the cookie isn't explicitly defining it's domain based on this value. I'm going to push a future update so that it does, such that it will conform to the above statement - right now, I think that it requires the most descriptive subdomain.

ebollens commented 12 years ago

Rich, I've resolved the issue with the config comment.

That leaves us one final thing to deal with: that the cookie needs to define domain based on value. Working on this now.

ebollens commented 12 years ago

This and the type error are both resolved in 96a401d in feature/dts and merged to develop and develop-1.2.

ebollens commented 12 years ago

Please let me know if there are any remaining issues that you encounter with DTS.

Trott commented 12 years ago

Clean checkout of develop-1.2, set site_url, set site_assets_url, do not set cookie_domain...

No cookies set....

Trott commented 12 years ago

Clean checkout of develop-1.2 on a Mac, set site_url to "http://localhost", site_assets_url to "http://localhost/assets", set cookie_domain to "localhost", open http://localhost/ in Chrome, infinite looping. (Remove your cookies if you don't see this happening.)

Trott commented 12 years ago

Ooof.... Clean checkout...configure site_url, configure site_assets_url, configure cookie_domain to be somewhat permissive (e.g., ucsf.edu instead of m.ucsf.edu) of your site_url, clear your cookies in iOS simulator, load in iOS simulator, infinite redirect loop....

ebollens commented 12 years ago

On your looping issue, I was able to get it to enter an infinite loop in Chrome and Opera, but not Safari, iOS Safari or Firefox. I'm currently looking at the issue to figure out why the loop shows up on some browsers but not others. I'm afraid it may again have something to do with when in the Javascript event stack it's actually safe to do the reload.

ebollens commented 12 years ago

Rich, interestingly, this is the line causing the loop in Chrome:

if(mwf.site.cookie.domain)
                cookieSuffix += ";domain="+mwf.site.cookie.domain;

According to RFC 2109, it should actually be:

if(mwf.site.cookie.domain)
                cookieSuffix += ";domain=."+mwf.site.cookie.domain;

It's safe to just impose the dot because I strip any first dot in the PHP before setting mwf.site.cookie.domain in vars.php.

However, this doesn't resolve the problem in Chrome, while simply removing it does.

I therefore propose the following changes:

I'll have a commit up in a few minutes that accomplishes this.

Trott commented 12 years ago

On getting it to loop in some browsers and not others, is it possible you skipped the clear-out-the-cookies step? That made the difference for me in iOS Simulator. Well, whatever, looks like we might have a fix, thanks, I'll get testing.

ebollens commented 12 years ago

Patched this in develop-1.2 with commit 6c0bd00.

This commit no longer provides the config option for domain, and removes domain specificity.

Instead, it assumes that one must be under the SP's tightest domain in order to not have to redirect.

As an example, assume the SP is at http://framework.m.example.com/1.2/assets. In this case, any path under framework.m.example.com is valid, such as subdomains like apps.framework.m.example.com and paths like framework.m.example.com/1.3. However, less tight subdomains like m.example.com and example.com are not valid, nor are sibling-level subdomains like other.m.example.com. This might sound like a "rigid" rule, but in reality, it simply means that if you're outside of the same origin as the framework, then you definitely redirect through passthru.php. Removes a lot of user-related configuration risk.

Let me know what you think.

ebollens commented 12 years ago

As an aside, Opera is still not behaving nicely. Seems to think we're doing CSRF.

Trott commented 12 years ago

This seems to have fixed the infinite redirect loop although a warning for UCSD: Using an offline appcache will result in infinite redirect loops for browsers that don't already have the cookies set. I guess you can get around it by making sure you don't cache the wrong JS, but I'm not sure if that will completely defeat the purpose of offline caching. More testing required, I guess.

ebollens commented 12 years ago

Rich, I'm open to thoughts to make this work with app cache. However, if that's going to be a target, I'd like to slate it for a future revision release so that we can test this thing hard today and get it packaged in a 1.2.06 release tomorrow on schedule.

Trott commented 12 years ago

(I guess it doesn't completely defeat the purpose, as you still get a performance boost on loading images etc.)

ebollens commented 12 years ago

Yeah, but it's still a loss, so let's slate it for future investigation.

Trott commented 12 years ago

On my Mac web server, vars.php generates a piece of JS that looks like this:

this.classification="{\"mobile\":false,\"basic\":true,\"standard\":true,\"full\":true}"

Cool.

But on CentOS, it doesn't backslash escape the quotes and the JS blows up. If you know why I'm seeing this and can save me the trouble of figuring it out, awesome.

ebollens commented 12 years ago

Must be with the way that PHP versions or deeper OS differences handle $_COOKIE.

See vars.php line 35:

$classification_cookie_var = '"'.$_COOKIE[$prefix.'classification'].'"';
ebollens commented 12 years ago

Since we know the form, maybe the solution is to do: addslashes(stripslashes($_COOKIE[...

Trott commented 12 years ago

It's the php_ini setting magic_quotes_gpc. We need to use ini_get('magic_quotes_gpc') and add the slashes ourselves or not based on what the server setting is. Or at least that's my take based on five minutes of research. Fun!

ebollens commented 12 years ago

I'm glad they've deprecated it in PHP 5.3, but do you want to go ahead and commit the fix? I'm about to run to class, so won't have time to dev/test for a couple hours.

Trott commented 12 years ago

On it.