search

intercom / intercom-rails

The easiest way to install Intercom in a Rails app.
https://developers.intercom.io/reference
MIT License
280 stars 106 forks source link

Issues with ShutdownHelper #249

Open rap1ds opened 7 years ago

rap1ds commented 7 years ago

Hi there! Thanks for the intercom-rails gem. It was a great starting point for me to implement server-side shutdown. 👍

However, I had a couple of issues while implementing the server side shutdown using the ShutdownHelper. Please note that I'm not actually using the whole intercom-rails gem, just the ShutdownHelper module.

Version info

1. Issue: Domain missing from the cookie removal

As described in the issue #194, the cookie removal seems to be broken, because the domain is missing. The Intercom JS client sets the domain for the cookie, so according to Rails documentation and this Stack Overflow answer, the domain needs to be included when the cookie is removed.

From Rails docs:

Please note that if you specify a :domain when setting a cookie, you must also specify the domain when deleting the cookie

As @abrambailey suggested in #194, the quick fix is to add domain to the cookie removal. @Skaelv suggested that optional domain argument could be added to Shutdown Helper. However, that's not good enough. I'm using Intercom in an environment, where I don't know exactly what is the current domain. Users of my application can use their own domain (similar to hosted wordpress.com, where people can run their blog from the own domain). So I can't add the static domain there.

Because of this, I have to dynamically define the domain for the cookie from the request URL in my Rails code. And that needs to be done exactly in the same way as the Intercom JS client is doing it or otherwise the cookie domain doesn't match and the cookie is not properly deleted. Because of this, I needed to do some reverse engineering. I copied the code from the Intercom JS library, pretty printed the minified code and found out this piece of code, which seems to be responsible for writing the session cookie:

    function(e, t) {
        "use strict";
        var n = /[^.]*\.([^.]*|..\...|...\...)$/,
            o = /^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$/;
        e.exports = {
            finddomain: function(e) {
                var t = e.match(o);
                if (!t && (t = e.match(n))) {
                    var r = t[0];
                    return r = r.split(":")[0], "." + r
                }
            },
            read: function(e, t) {
                t = t || document.cookie;
                var n = "(?:(?:^|.*;)\\s*" + encodeuricomponent(e).replace(/[\-\.\+\*]/g, "\\$&") + "\\s*\\=\\s*([^;]*).*$)|^.*$";
                return decodeuricomponent(t.replace(new regexp(n), "$1")) || void 0
            },
            write: function(e, t, n, o, r) {
                var i = e + "=" + t;
                return "all" === n && (n = this.finddomain(location.host)), n && (i += "; domain=" + n), o && (i += "; path=" + o), r && (i += "; expires=" + r.toutcstring()), document.cookie = i, i
            },
            clear: function(e, t, n) {
                this.write(e, "", t, n, new date(0))
            }
        }
    },

The findDomain function seems to be the one that defines the cookie domain based on location.host. So I ended up porting that piece of code to Ruby and using that to define the cookie domain for cookie removal. The code is here if you're interested: https://github.com/sharetribe/sharetribe/pull/2924

So to recap the problem:

  1. I'm pretty sure there's a bug in the ShutdownHelper. It doesn't remove the cookie without domain.
  2. Adding static domain to the cookie removal is not enough, neither is adding optional domain argument to the ShutdownHelper
  3. Proper fix needs reverse engineering of the JS code, which is of course not optimal.

Suggestion:

To properly fix this, the functionaly for finding the right cookie domain should be included in the intercom-rails and it should be kept in sync with the JS library.

2. Question: Are you sure cookie removal doesn't work on redirect?

In the README, it says:

Be aware that if you call this method before a 'redirect_to' (quite common on logout) it will have no impact as it is impossible to update cookies when you use a redirection. But you can use the same logic as the devise implementation above.

Is this really the case? Is this documented somewhere (in Rails docs or in HTTP spec?). I did a quick Google search and I couldn't come up with any Rails documentation or any mention in HTTP spec saying that one couldn't set cookies on redirect.

I tried this on my local dev machine. I removed the cookie before calling redirect_to and it worked just fine.

I did found some information from 2009, saying that some browsers don't set cookie on redirect. However, the article was old and apparently this was a browser bug that has been fixed since then.

So to conclude: I believe that it's actually possible delete the cookie before redirect_to. Maybe it wasn't working for you because the domain was missing (just a wild guess)? It would be a lot cleaner to delete the cookie in the SessionsController (or what ever the controller is that handles the logout) and not to spread the log out logic to other controllers.

Thanks!

(PS. If you'd rather have these issues as separate issues, I'm happy to close this issue and split it to two if that's preferred!)

(PPS. Sorry for not using the Issue template. I hope I provided enough information in this Issue, but if not, I'm happy to provide more.)

bradcrawford commented 7 years ago

It would be awesome if you could configure the cookie domain the JS api.

Context, I use Pow and have my dev domains setup to mirror my production domains with .dev appended. So my production domain is www.<domain.com> and my dev domain is www.<domain.com>.dev. Unfortunately, this results in the cookie being written to the .com.dev domain rather than <domain.com>.dev during development.

For anyone that finds it useful, the following should delete the Intercom session cookie and log the user out. Replace ENV['INTERCOM_APP_ID'] with your preferred mechanism for storing config vars.

I also do not experience the issue with a redirect issue that is described in the docs.

  def find_intercom_domain(host)
    o = /^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$/
    n = /[^.]*\.([^.]*|..\...|...\...)$/

    t = host.match(o)
    if !t && (t = host.match(n))
      r = t[0]
      return r = r.split(':')[0], '.' + r
    end
  end

  def clear_intercom_cookie
    cookies.delete "intercom-session-#{ENV['INTERCOM_APP_ID']}", domain: find_intercom_domain(request.host)
  end
ideasasylum commented 7 years ago

Just got bitten by this bug too and I think it needs to be addressed. I needed to pass the domain when deleting the cookie to make Intercom shutdown

timherby commented 6 years ago

Just used this solution too. We have multiple domains and need to dynamically determine the domain. The gem should really integrate this fix in order to align with what the client does.

Also, no problems with setting cookies on redirect, so that whole process in the gem can be eliminated.

zeeshangulzar commented 6 years ago

I have upgraded to 0.4.0 version and passed the domain parameter as mentioned in docs, but it did''t work for multiple domains. I also tried above mentioned solution, but no success yet.

It would be great if anyone can guide me in right direction.

eliotsykes commented 6 years ago

It'd be good to understand for certain if the don't clear cookies on redirect guidance is ever needed.

Can Intercom's developers confirm what their current guidance is for customers?

In case its of help, the advice was added in this PR: https://github.com/intercom/intercom-rails/pull/174