delete window.intercomSettings on shutdown

[ekilah]

this library adds a new piece of data to window: window.intercomSettings. this piece of data contains data about the intercom user, like their name, email, and even the user_hash, which is a security token used to guarantee the user chatting with you is who they say they are.

This data should be cleared whenever logging out of your application, otherwise that user_hash and other user information is sitting around in the tab after the user has logged out.

shutdown is the API you are supposed to call when logging out, so this function should do this cleanup, too.

hardShutdown does this already, but it also deletes window.Intercom. That's problematic because if someone logs back in in the same session/tab, you don't want window.Intercom to be missing (nothing adds it back)

[devrnt]

Hi, thanks for creating this PR, really appreciate it.

Some remarks: at the moment the shutdown, follows the suggested shutdown flow, see https://www.intercom.com/help/en/articles/16845-how-do-i-end-a-session. I can't find any docs/fora where it's recommended to delete the window.intercomSettings.
If I can't trace back where the cleanup is recommended I'll keep the recommended "shutdown flow".

[ekilah]

I am under the impression that the Intercom library does not read window.intercomSettings directly, though I don't have proof of that one way or another.

Their "Basic Javascript" docs do tell you to set window.intercomSettings, but then the code blob that it tells you to paste right under that just uses that in the update call it makes. I imagine that for non-SPAs the window variable there is just used to have a global you can edit later if needed.

Right below those docs are the separate Single Page Application docs and those instructions never tell you to set window.intercomSettings, and you can see instead the docs just tell you to call window.Intercom('boot')(and update, etc) with the raw settings object as needed. This library's implementation follows the SPA docs by calling window.Intercom('boot'), 'update', etc, but for some reason it also sets window.intercomSettings.

So, it looks like this library is doing a bit of both. I imagine you could just stop setting intercomSettings completely in this library without affecting React SPAs, but I didn't aim to make that large of a change. It would be easy to test - either Intercom works, or it doesn't. I would be shocked if Intercom's JS bundle required window.intercomSettings to be set while also requiring an object in the boot call, but I've seen weirder.

The goal of this PR is just to delete the data about the previously-logged-in user after logging out, which is what the Intercom('shutdown') API already does (by deleting their own cookies). This change is in the spirit of their shutdown docs, though I admit their docs don't mention clearing this directly. I will email their support team, because I think it should be included there, too.

---

Without this PR's change, if someone logs out of a website using Intercom and leaves the tab open, an attacker could read window.intercomSettings, copy the user_hash stored there, log in to the application as a different user, set their own user_hash to the stolen one, and pretend to be the first user. This is just as bad as not clearing the cookies, which shutdown already does.

[devrnt]

Thanks for taking your time and explaining this, really appreciate it! I can totally follow your need to remove window.intercomSettings, but as you mentioned I'll wait for the response to your mail. If you receive no response I'll probably merge this anyway, but let's make sure that we don't break any current behaviour

[ekilah]

Sounds good! I sent Intercom a message, we'll see what they say. I'll update here when I receive a response.

[ekilah]

Got a reply yesterday. Their response confirms that they don't expect an SPA integration like this one to be setting window.intercomSettings at all:

Thanks for the report and your patience!

The basic javascript section of the installation guide assumes that the website will be "...a web app with multiple pages where each one triggers a new page refresh..." which means the previous window.intercomSettings object should be destroyed on page navigation (i.e. logout -> homepage). Whereas the single page installation section assumes the opposite, so examples in that section don't set the window.intercomSettings object directly – showing instead to pass the information directly into the messenger's JS API calls.

[...] there isn't a security vulnerability here but we could update our documentation on how to end sessions so it's more explicit about this. I'll follow up with the team to have that updated appropriately – hope that's helpful.

Thanks,
Josh

[devrnt]

Thanks for the update! so TLDR, window.intercomSettings isn't relevant when Intercom is used in an SPA? The Intercom API expects you to pass in all options/settings through the arguments of the according method?

[ekilah]

Correct...! My PR here doesn't remove it completely, but we/you/I could do that instead, or separately.

[hjoelh]

Hey guys, sounds good. Any news here?

[ekilah]

Hi there @hjoelh - I am mostly waiting for @devrnt to reply. I think this PR is mergeable now, and I'd be happy to follow up with a second PR to remove window.intercomSettings completely if that was desired also.

We could combine them into one PR as well if that is easier on @devrnt , I don't have much of a preference, it's just a slightly larger change I think.

My concern was mostly about making sure this library cleaned up after itself, which this PR does already!

[trevorwhealy]

Noticing the same behavior here. Authenticated user sessions are still persisting even after shutdown is called which is exposing the user-protected Intercom articles and chat messages to a logged out user

[ekilah]

@trevorwhealy feel free to delete window.intercomSettings yourself after you call shutdown until this merges, I tested it on my own application and still have that running in production waiting for this to be released 🤓

[ekilah]

🥳

[devrnt]

v2.0.0

[ekilah]

Thanks for the release @devrnt !

[ekilah]

this library adds a new piece of data to window: window.intercomSettings. this piece of data contains data about the intercom user, like their name, email, and even the user_hash, which is a security token used to guarantee the user chatting with you is who they say they are.

This data should be cleared whenever logging out of your application, otherwise that user_hash and other user information is sitting around in the tab after the user has logged out.

shutdown is the API you are supposed to call when logging out, so this function should do this cleanup, too.

hardShutdown does this already, but it also deletes window.Intercom. That's problematic because if someone logs back in in the same session/tab, you don't want window.Intercom to be missing (nothing adds it back)

[devrnt]

Hi, thanks for creating this PR, really appreciate it.

Some remarks: at the moment the shutdown, follows the suggested shutdown flow, see https://www.intercom.com/help/en/articles/16845-how-do-i-end-a-session. I can't find any docs/fora where it's recommended to delete the window.intercomSettings.
If I can't trace back where the cleanup is recommended I'll keep the recommended "shutdown flow".

[ekilah]

I am under the impression that the Intercom library does not read window.intercomSettings directly, though I don't have proof of that one way or another.

Their "Basic Javascript" docs do tell you to set window.intercomSettings, but then the code blob that it tells you to paste right under that just uses that in the update call it makes. I imagine that for non-SPAs the window variable there is just used to have a global you can edit later if needed.

Right below those docs are the separate Single Page Application docs and those instructions never tell you to set window.intercomSettings, and you can see instead the docs just tell you to call window.Intercom('boot')(and update, etc) with the raw settings object as needed. This library's implementation follows the SPA docs by calling window.Intercom('boot'), 'update', etc, but for some reason it also sets window.intercomSettings.

So, it looks like this library is doing a bit of both. I imagine you could just stop setting intercomSettings completely in this library without affecting React SPAs, but I didn't aim to make that large of a change. It would be easy to test - either Intercom works, or it doesn't. I would be shocked if Intercom's JS bundle required window.intercomSettings to be set while also requiring an object in the boot call, but I've seen weirder.

The goal of this PR is just to delete the data about the previously-logged-in user after logging out, which is what the Intercom('shutdown') API already does (by deleting their own cookies). This change is in the spirit of their shutdown docs, though I admit their docs don't mention clearing this directly. I will email their support team, because I think it should be included there, too.

---

Without this PR's change, if someone logs out of a website using Intercom and leaves the tab open, an attacker could read window.intercomSettings, copy the user_hash stored there, log in to the application as a different user, set their own user_hash to the stolen one, and pretend to be the first user. This is just as bad as not clearing the cookies, which shutdown already does.

[devrnt]

Thanks for taking your time and explaining this, really appreciate it! I can totally follow your need to remove window.intercomSettings, but as you mentioned I'll wait for the response to your mail. If you receive no response I'll probably merge this anyway, but let's make sure that we don't break any current behaviour

[ekilah]

Sounds good! I sent Intercom a message, we'll see what they say. I'll update here when I receive a response.

[ekilah]

Got a reply yesterday. Their response confirms that they don't expect an SPA integration like this one to be setting window.intercomSettings at all:

Thanks for the report and your patience!

The basic javascript section of the installation guide assumes that the website will be "...a web app with multiple pages where each one triggers a new page refresh..." which means the previous window.intercomSettings object should be destroyed on page navigation (i.e. logout -> homepage). Whereas the single page installation section assumes the opposite, so examples in that section don't set the window.intercomSettings object directly – showing instead to pass the information directly into the messenger's JS API calls.

[...] there isn't a security vulnerability here but we could update our documentation on how to end sessions so it's more explicit about this. I'll follow up with the team to have that updated appropriately – hope that's helpful.

Thanks,
Josh

[devrnt]

Thanks for the update! so TLDR, window.intercomSettings isn't relevant when Intercom is used in an SPA? The Intercom API expects you to pass in all options/settings through the arguments of the according method?

[ekilah]

Correct...! My PR here doesn't remove it completely, but we/you/I could do that instead, or separately.

[hjoelh]

Hey guys, sounds good. Any news here?

[ekilah]

Hi there @hjoelh - I am mostly waiting for @devrnt to reply. I think this PR is mergeable now, and I'd be happy to follow up with a second PR to remove window.intercomSettings completely if that was desired also.

We could combine them into one PR as well if that is easier on @devrnt , I don't have much of a preference, it's just a slightly larger change I think.

My concern was mostly about making sure this library cleaned up after itself, which this PR does already!

[trevorwhealy]

Noticing the same behavior here. Authenticated user sessions are still persisting even after shutdown is called which is exposing the user-protected Intercom articles and chat messages to a logged out user

[ekilah]

@trevorwhealy feel free to delete window.intercomSettings yourself after you call shutdown until this merges, I tested it on my own application and still have that running in production waiting for this to be released 🤓

[ekilah]

🥳

[devrnt]

v2.0.0

[ekilah]

Thanks for the release @devrnt !

[ekilah]

this library adds a new piece of data to window: window.intercomSettings. this piece of data contains data about the intercom user, like their name, email, and even the user_hash, which is a security token used to guarantee the user chatting with you is who they say they are.

This data should be cleared whenever logging out of your application, otherwise that user_hash and other user information is sitting around in the tab after the user has logged out.

shutdown is the API you are supposed to call when logging out, so this function should do this cleanup, too.

hardShutdown does this already, but it also deletes window.Intercom. That's problematic because if someone logs back in in the same session/tab, you don't want window.Intercom to be missing (nothing adds it back)

[devrnt]

Hi, thanks for creating this PR, really appreciate it.

Some remarks: at the moment the shutdown, follows the suggested shutdown flow, see https://www.intercom.com/help/en/articles/16845-how-do-i-end-a-session. I can't find any docs/fora where it's recommended to delete the window.intercomSettings.
If I can't trace back where the cleanup is recommended I'll keep the recommended "shutdown flow".

[ekilah]

I am under the impression that the Intercom library does not read window.intercomSettings directly, though I don't have proof of that one way or another.

Their "Basic Javascript" docs do tell you to set window.intercomSettings, but then the code blob that it tells you to paste right under that just uses that in the update call it makes. I imagine that for non-SPAs the window variable there is just used to have a global you can edit later if needed.

Right below those docs are the separate Single Page Application docs and those instructions never tell you to set window.intercomSettings, and you can see instead the docs just tell you to call window.Intercom('boot')(and update, etc) with the raw settings object as needed. This library's implementation follows the SPA docs by calling window.Intercom('boot'), 'update', etc, but for some reason it also sets window.intercomSettings.

So, it looks like this library is doing a bit of both. I imagine you could just stop setting intercomSettings completely in this library without affecting React SPAs, but I didn't aim to make that large of a change. It would be easy to test - either Intercom works, or it doesn't. I would be shocked if Intercom's JS bundle required window.intercomSettings to be set while also requiring an object in the boot call, but I've seen weirder.

The goal of this PR is just to delete the data about the previously-logged-in user after logging out, which is what the Intercom('shutdown') API already does (by deleting their own cookies). This change is in the spirit of their shutdown docs, though I admit their docs don't mention clearing this directly. I will email their support team, because I think it should be included there, too.

---

Without this PR's change, if someone logs out of a website using Intercom and leaves the tab open, an attacker could read window.intercomSettings, copy the user_hash stored there, log in to the application as a different user, set their own user_hash to the stolen one, and pretend to be the first user. This is just as bad as not clearing the cookies, which shutdown already does.

[devrnt]

Thanks for taking your time and explaining this, really appreciate it! I can totally follow your need to remove window.intercomSettings, but as you mentioned I'll wait for the response to your mail. If you receive no response I'll probably merge this anyway, but let's make sure that we don't break any current behaviour

[ekilah]

Sounds good! I sent Intercom a message, we'll see what they say. I'll update here when I receive a response.

[ekilah]

Got a reply yesterday. Their response confirms that they don't expect an SPA integration like this one to be setting window.intercomSettings at all:

Thanks for the report and your patience!

The basic javascript section of the installation guide assumes that the website will be "...a web app with multiple pages where each one triggers a new page refresh..." which means the previous window.intercomSettings object should be destroyed on page navigation (i.e. logout -> homepage). Whereas the single page installation section assumes the opposite, so examples in that section don't set the window.intercomSettings object directly – showing instead to pass the information directly into the messenger's JS API calls.

[...] there isn't a security vulnerability here but we could update our documentation on how to end sessions so it's more explicit about this. I'll follow up with the team to have that updated appropriately – hope that's helpful.

Thanks,
Josh

[devrnt]

Thanks for the update! so TLDR, window.intercomSettings isn't relevant when Intercom is used in an SPA? The Intercom API expects you to pass in all options/settings through the arguments of the according method?

[ekilah]

Correct...! My PR here doesn't remove it completely, but we/you/I could do that instead, or separately.

[hjoelh]

Hey guys, sounds good. Any news here?

[ekilah]

Hi there @hjoelh - I am mostly waiting for @devrnt to reply. I think this PR is mergeable now, and I'd be happy to follow up with a second PR to remove window.intercomSettings completely if that was desired also.

We could combine them into one PR as well if that is easier on @devrnt , I don't have much of a preference, it's just a slightly larger change I think.

My concern was mostly about making sure this library cleaned up after itself, which this PR does already!

[trevorwhealy]

Noticing the same behavior here. Authenticated user sessions are still persisting even after shutdown is called which is exposing the user-protected Intercom articles and chat messages to a logged out user

[ekilah]

@trevorwhealy feel free to delete window.intercomSettings yourself after you call shutdown until this merges, I tested it on my own application and still have that running in production waiting for this to be released 🤓

[ekilah]

🥳

[devrnt]

v2.0.0

[ekilah]

Thanks for the release @devrnt !

[ekilah]

this library adds a new piece of data to window: window.intercomSettings. this piece of data contains data about the intercom user, like their name, email, and even the user_hash, which is a security token used to guarantee the user chatting with you is who they say they are.

This data should be cleared whenever logging out of your application, otherwise that user_hash and other user information is sitting around in the tab after the user has logged out.

shutdown is the API you are supposed to call when logging out, so this function should do this cleanup, too.

hardShutdown does this already, but it also deletes window.Intercom. That's problematic because if someone logs back in in the same session/tab, you don't want window.Intercom to be missing (nothing adds it back)

[devrnt]

Hi, thanks for creating this PR, really appreciate it.

Some remarks: at the moment the shutdown, follows the suggested shutdown flow, see https://www.intercom.com/help/en/articles/16845-how-do-i-end-a-session. I can't find any docs/fora where it's recommended to delete the window.intercomSettings.
If I can't trace back where the cleanup is recommended I'll keep the recommended "shutdown flow".

[ekilah]

I am under the impression that the Intercom library does not read window.intercomSettings directly, though I don't have proof of that one way or another.

Their "Basic Javascript" docs do tell you to set window.intercomSettings, but then the code blob that it tells you to paste right under that just uses that in the update call it makes. I imagine that for non-SPAs the window variable there is just used to have a global you can edit later if needed.

Right below those docs are the separate Single Page Application docs and those instructions never tell you to set window.intercomSettings, and you can see instead the docs just tell you to call window.Intercom('boot')(and update, etc) with the raw settings object as needed. This library's implementation follows the SPA docs by calling window.Intercom('boot'), 'update', etc, but for some reason it also sets window.intercomSettings.

So, it looks like this library is doing a bit of both. I imagine you could just stop setting intercomSettings completely in this library without affecting React SPAs, but I didn't aim to make that large of a change. It would be easy to test - either Intercom works, or it doesn't. I would be shocked if Intercom's JS bundle required window.intercomSettings to be set while also requiring an object in the boot call, but I've seen weirder.

The goal of this PR is just to delete the data about the previously-logged-in user after logging out, which is what the Intercom('shutdown') API already does (by deleting their own cookies). This change is in the spirit of their shutdown docs, though I admit their docs don't mention clearing this directly. I will email their support team, because I think it should be included there, too.

---

Without this PR's change, if someone logs out of a website using Intercom and leaves the tab open, an attacker could read window.intercomSettings, copy the user_hash stored there, log in to the application as a different user, set their own user_hash to the stolen one, and pretend to be the first user. This is just as bad as not clearing the cookies, which shutdown already does.

[devrnt]

Thanks for taking your time and explaining this, really appreciate it! I can totally follow your need to remove window.intercomSettings, but as you mentioned I'll wait for the response to your mail. If you receive no response I'll probably merge this anyway, but let's make sure that we don't break any current behaviour

[ekilah]

Sounds good! I sent Intercom a message, we'll see what they say. I'll update here when I receive a response.

[ekilah]

Got a reply yesterday. Their response confirms that they don't expect an SPA integration like this one to be setting window.intercomSettings at all:

Thanks for the report and your patience!

The basic javascript section of the installation guide assumes that the website will be "...a web app with multiple pages where each one triggers a new page refresh..." which means the previous window.intercomSettings object should be destroyed on page navigation (i.e. logout -> homepage). Whereas the single page installation section assumes the opposite, so examples in that section don't set the window.intercomSettings object directly – showing instead to pass the information directly into the messenger's JS API calls.

[...] there isn't a security vulnerability here but we could update our documentation on how to end sessions so it's more explicit about this. I'll follow up with the team to have that updated appropriately – hope that's helpful.

Thanks,
Josh

[devrnt]

Thanks for the update! so TLDR, window.intercomSettings isn't relevant when Intercom is used in an SPA? The Intercom API expects you to pass in all options/settings through the arguments of the according method?

[ekilah]

Correct...! My PR here doesn't remove it completely, but we/you/I could do that instead, or separately.

[hjoelh]

Hey guys, sounds good. Any news here?

[ekilah]

Hi there @hjoelh - I am mostly waiting for @devrnt to reply. I think this PR is mergeable now, and I'd be happy to follow up with a second PR to remove window.intercomSettings completely if that was desired also.

We could combine them into one PR as well if that is easier on @devrnt , I don't have much of a preference, it's just a slightly larger change I think.

My concern was mostly about making sure this library cleaned up after itself, which this PR does already!

[trevorwhealy]

Noticing the same behavior here. Authenticated user sessions are still persisting even after shutdown is called which is exposing the user-protected Intercom articles and chat messages to a logged out user

[ekilah]

@trevorwhealy feel free to delete window.intercomSettings yourself after you call shutdown until this merges, I tested it on my own application and still have that running in production waiting for this to be released 🤓

[ekilah]

🥳

[devrnt]

v2.0.0

[ekilah]

Thanks for the release @devrnt !

[ekilah]

this library adds a new piece of data to window: window.intercomSettings. this piece of data contains data about the intercom user, like their name, email, and even the user_hash, which is a security token used to guarantee the user chatting with you is who they say they are.

This data should be cleared whenever logging out of your application, otherwise that user_hash and other user information is sitting around in the tab after the user has logged out.

shutdown is the API you are supposed to call when logging out, so this function should do this cleanup, too.

hardShutdown does this already, but it also deletes window.Intercom. That's problematic because if someone logs back in in the same session/tab, you don't want window.Intercom to be missing (nothing adds it back)

[devrnt]

Hi, thanks for creating this PR, really appreciate it.

Some remarks: at the moment the shutdown, follows the suggested shutdown flow, see https://www.intercom.com/help/en/articles/16845-how-do-i-end-a-session. I can't find any docs/fora where it's recommended to delete the window.intercomSettings.
If I can't trace back where the cleanup is recommended I'll keep the recommended "shutdown flow".

[ekilah]

I am under the impression that the Intercom library does not read window.intercomSettings directly, though I don't have proof of that one way or another.

Their "Basic Javascript" docs do tell you to set window.intercomSettings, but then the code blob that it tells you to paste right under that just uses that in the update call it makes. I imagine that for non-SPAs the window variable there is just used to have a global you can edit later if needed.

Right below those docs are the separate Single Page Application docs and those instructions never tell you to set window.intercomSettings, and you can see instead the docs just tell you to call window.Intercom('boot')(and update, etc) with the raw settings object as needed. This library's implementation follows the SPA docs by calling window.Intercom('boot'), 'update', etc, but for some reason it also sets window.intercomSettings.

So, it looks like this library is doing a bit of both. I imagine you could just stop setting intercomSettings completely in this library without affecting React SPAs, but I didn't aim to make that large of a change. It would be easy to test - either Intercom works, or it doesn't. I would be shocked if Intercom's JS bundle required window.intercomSettings to be set while also requiring an object in the boot call, but I've seen weirder.

The goal of this PR is just to delete the data about the previously-logged-in user after logging out, which is what the Intercom('shutdown') API already does (by deleting their own cookies). This change is in the spirit of their shutdown docs, though I admit their docs don't mention clearing this directly. I will email their support team, because I think it should be included there, too.

---

Without this PR's change, if someone logs out of a website using Intercom and leaves the tab open, an attacker could read window.intercomSettings, copy the user_hash stored there, log in to the application as a different user, set their own user_hash to the stolen one, and pretend to be the first user. This is just as bad as not clearing the cookies, which shutdown already does.

[devrnt]

Thanks for taking your time and explaining this, really appreciate it! I can totally follow your need to remove window.intercomSettings, but as you mentioned I'll wait for the response to your mail. If you receive no response I'll probably merge this anyway, but let's make sure that we don't break any current behaviour

[ekilah]

Sounds good! I sent Intercom a message, we'll see what they say. I'll update here when I receive a response.

[ekilah]

Got a reply yesterday. Their response confirms that they don't expect an SPA integration like this one to be setting window.intercomSettings at all:

Thanks for the report and your patience!

The basic javascript section of the installation guide assumes that the website will be "...a web app with multiple pages where each one triggers a new page refresh..." which means the previous window.intercomSettings object should be destroyed on page navigation (i.e. logout -> homepage). Whereas the single page installation section assumes the opposite, so examples in that section don't set the window.intercomSettings object directly – showing instead to pass the information directly into the messenger's JS API calls.

[...] there isn't a security vulnerability here but we could update our documentation on how to end sessions so it's more explicit about this. I'll follow up with the team to have that updated appropriately – hope that's helpful.

Thanks,
Josh

[devrnt]

Thanks for the update! so TLDR, window.intercomSettings isn't relevant when Intercom is used in an SPA? The Intercom API expects you to pass in all options/settings through the arguments of the according method?

[ekilah]

Correct...! My PR here doesn't remove it completely, but we/you/I could do that instead, or separately.

[hjoelh]

Hey guys, sounds good. Any news here?

[ekilah]

Hi there @hjoelh - I am mostly waiting for @devrnt to reply. I think this PR is mergeable now, and I'd be happy to follow up with a second PR to remove window.intercomSettings completely if that was desired also.

We could combine them into one PR as well if that is easier on @devrnt , I don't have much of a preference, it's just a slightly larger change I think.

My concern was mostly about making sure this library cleaned up after itself, which this PR does already!

[trevorwhealy]

Noticing the same behavior here. Authenticated user sessions are still persisting even after shutdown is called which is exposing the user-protected Intercom articles and chat messages to a logged out user

[ekilah]

@trevorwhealy feel free to delete window.intercomSettings yourself after you call shutdown until this merges, I tested it on my own application and still have that running in production waiting for this to be released 🤓

[ekilah]

🥳

[devrnt]

v2.0.0

[ekilah]

Thanks for the release @devrnt !

[ekilah]

this library adds a new piece of data to window: window.intercomSettings. this piece of data contains data about the intercom user, like their name, email, and even the user_hash, which is a security token used to guarantee the user chatting with you is who they say they are.

This data should be cleared whenever logging out of your application, otherwise that user_hash and other user information is sitting around in the tab after the user has logged out.

shutdown is the API you are supposed to call when logging out, so this function should do this cleanup, too.

hardShutdown does this already, but it also deletes window.Intercom. That's problematic because if someone logs back in in the same session/tab, you don't want window.Intercom to be missing (nothing adds it back)

[devrnt]

Hi, thanks for creating this PR, really appreciate it.

Some remarks: at the moment the shutdown, follows the suggested shutdown flow, see https://www.intercom.com/help/en/articles/16845-how-do-i-end-a-session. I can't find any docs/fora where it's recommended to delete the window.intercomSettings.
If I can't trace back where the cleanup is recommended I'll keep the recommended "shutdown flow".

[ekilah]

I am under the impression that the Intercom library does not read window.intercomSettings directly, though I don't have proof of that one way or another.

Their "Basic Javascript" docs do tell you to set window.intercomSettings, but then the code blob that it tells you to paste right under that just uses that in the update call it makes. I imagine that for non-SPAs the window variable there is just used to have a global you can edit later if needed.

Right below those docs are the separate Single Page Application docs and those instructions never tell you to set window.intercomSettings, and you can see instead the docs just tell you to call window.Intercom('boot')(and update, etc) with the raw settings object as needed. This library's implementation follows the SPA docs by calling window.Intercom('boot'), 'update', etc, but for some reason it also sets window.intercomSettings.

So, it looks like this library is doing a bit of both. I imagine you could just stop setting intercomSettings completely in this library without affecting React SPAs, but I didn't aim to make that large of a change. It would be easy to test - either Intercom works, or it doesn't. I would be shocked if Intercom's JS bundle required window.intercomSettings to be set while also requiring an object in the boot call, but I've seen weirder.

The goal of this PR is just to delete the data about the previously-logged-in user after logging out, which is what the Intercom('shutdown') API already does (by deleting their own cookies). This change is in the spirit of their shutdown docs, though I admit their docs don't mention clearing this directly. I will email their support team, because I think it should be included there, too.

---

Without this PR's change, if someone logs out of a website using Intercom and leaves the tab open, an attacker could read window.intercomSettings, copy the user_hash stored there, log in to the application as a different user, set their own user_hash to the stolen one, and pretend to be the first user. This is just as bad as not clearing the cookies, which shutdown already does.

[devrnt]

Thanks for taking your time and explaining this, really appreciate it! I can totally follow your need to remove window.intercomSettings, but as you mentioned I'll wait for the response to your mail. If you receive no response I'll probably merge this anyway, but let's make sure that we don't break any current behaviour

[ekilah]

Sounds good! I sent Intercom a message, we'll see what they say. I'll update here when I receive a response.

[ekilah]

Got a reply yesterday. Their response confirms that they don't expect an SPA integration like this one to be setting window.intercomSettings at all:

Thanks for the report and your patience!

The basic javascript section of the installation guide assumes that the website will be "...a web app with multiple pages where each one triggers a new page refresh..." which means the previous window.intercomSettings object should be destroyed on page navigation (i.e. logout -> homepage). Whereas the single page installation section assumes the opposite, so examples in that section don't set the window.intercomSettings object directly – showing instead to pass the information directly into the messenger's JS API calls.

[...] there isn't a security vulnerability here but we could update our documentation on how to end sessions so it's more explicit about this. I'll follow up with the team to have that updated appropriately – hope that's helpful.

Thanks,
Josh

[devrnt]

Thanks for the update! so TLDR, window.intercomSettings isn't relevant when Intercom is used in an SPA? The Intercom API expects you to pass in all options/settings through the arguments of the according method?

[ekilah]

Correct...! My PR here doesn't remove it completely, but we/you/I could do that instead, or separately.

[hjoelh]

Hey guys, sounds good. Any news here?

[ekilah]

Hi there @hjoelh - I am mostly waiting for @devrnt to reply. I think this PR is mergeable now, and I'd be happy to follow up with a second PR to remove window.intercomSettings completely if that was desired also.

We could combine them into one PR as well if that is easier on @devrnt , I don't have much of a preference, it's just a slightly larger change I think.

My concern was mostly about making sure this library cleaned up after itself, which this PR does already!

[trevorwhealy]

Noticing the same behavior here. Authenticated user sessions are still persisting even after shutdown is called which is exposing the user-protected Intercom articles and chat messages to a logged out user

[ekilah]

@trevorwhealy feel free to delete window.intercomSettings yourself after you call shutdown until this merges, I tested it on my own application and still have that running in production waiting for this to be released 🤓

[ekilah]

🥳

[devrnt]

v2.0.0

[ekilah]

Thanks for the release @devrnt !

[ekilah]

this library adds a new piece of data to window: window.intercomSettings. this piece of data contains data about the intercom user, like their name, email, and even the user_hash, which is a security token used to guarantee the user chatting with you is who they say they are.

This data should be cleared whenever logging out of your application, otherwise that user_hash and other user information is sitting around in the tab after the user has logged out.

shutdown is the API you are supposed to call when logging out, so this function should do this cleanup, too.

hardShutdown does this already, but it also deletes window.Intercom. That's problematic because if someone logs back in in the same session/tab, you don't want window.Intercom to be missing (nothing adds it back)

[devrnt]

Hi, thanks for creating this PR, really appreciate it.

Some remarks: at the moment the shutdown, follows the suggested shutdown flow, see https://www.intercom.com/help/en/articles/16845-how-do-i-end-a-session. I can't find any docs/fora where it's recommended to delete the window.intercomSettings.
If I can't trace back where the cleanup is recommended I'll keep the recommended "shutdown flow".

[ekilah]

I am under the impression that the Intercom library does not read window.intercomSettings directly, though I don't have proof of that one way or another.

Their "Basic Javascript" docs do tell you to set window.intercomSettings, but then the code blob that it tells you to paste right under that just uses that in the update call it makes. I imagine that for non-SPAs the window variable there is just used to have a global you can edit later if needed.

Right below those docs are the separate Single Page Application docs and those instructions never tell you to set window.intercomSettings, and you can see instead the docs just tell you to call window.Intercom('boot')(and update, etc) with the raw settings object as needed. This library's implementation follows the SPA docs by calling window.Intercom('boot'), 'update', etc, but for some reason it also sets window.intercomSettings.

So, it looks like this library is doing a bit of both. I imagine you could just stop setting intercomSettings completely in this library without affecting React SPAs, but I didn't aim to make that large of a change. It would be easy to test - either Intercom works, or it doesn't. I would be shocked if Intercom's JS bundle required window.intercomSettings to be set while also requiring an object in the boot call, but I've seen weirder.

The goal of this PR is just to delete the data about the previously-logged-in user after logging out, which is what the Intercom('shutdown') API already does (by deleting their own cookies). This change is in the spirit of their shutdown docs, though I admit their docs don't mention clearing this directly. I will email their support team, because I think it should be included there, too.

---

Without this PR's change, if someone logs out of a website using Intercom and leaves the tab open, an attacker could read window.intercomSettings, copy the user_hash stored there, log in to the application as a different user, set their own user_hash to the stolen one, and pretend to be the first user. This is just as bad as not clearing the cookies, which shutdown already does.

[devrnt]

Thanks for taking your time and explaining this, really appreciate it! I can totally follow your need to remove window.intercomSettings, but as you mentioned I'll wait for the response to your mail. If you receive no response I'll probably merge this anyway, but let's make sure that we don't break any current behaviour

[ekilah]

Sounds good! I sent Intercom a message, we'll see what they say. I'll update here when I receive a response.

[ekilah]

Got a reply yesterday. Their response confirms that they don't expect an SPA integration like this one to be setting window.intercomSettings at all:

Thanks for the report and your patience!

The basic javascript section of the installation guide assumes that the website will be "...a web app with multiple pages where each one triggers a new page refresh..." which means the previous window.intercomSettings object should be destroyed on page navigation (i.e. logout -> homepage). Whereas the single page installation section assumes the opposite, so examples in that section don't set the window.intercomSettings object directly – showing instead to pass the information directly into the messenger's JS API calls.

[...] there isn't a security vulnerability here but we could update our documentation on how to end sessions so it's more explicit about this. I'll follow up with the team to have that updated appropriately – hope that's helpful.

Thanks,
Josh

[devrnt]

Thanks for the update! so TLDR, window.intercomSettings isn't relevant when Intercom is used in an SPA? The Intercom API expects you to pass in all options/settings through the arguments of the according method?

[ekilah]

Correct...! My PR here doesn't remove it completely, but we/you/I could do that instead, or separately.

[hjoelh]

Hey guys, sounds good. Any news here?

[ekilah]

Hi there @hjoelh - I am mostly waiting for @devrnt to reply. I think this PR is mergeable now, and I'd be happy to follow up with a second PR to remove window.intercomSettings completely if that was desired also.

We could combine them into one PR as well if that is easier on @devrnt , I don't have much of a preference, it's just a slightly larger change I think.

My concern was mostly about making sure this library cleaned up after itself, which this PR does already!

[trevorwhealy]

Noticing the same behavior here. Authenticated user sessions are still persisting even after shutdown is called which is exposing the user-protected Intercom articles and chat messages to a logged out user

[ekilah]

@trevorwhealy feel free to delete window.intercomSettings yourself after you call shutdown until this merges, I tested it on my own application and still have that running in production waiting for this to be released 🤓

[ekilah]

🥳

[devrnt]

v2.0.0

[ekilah]

Thanks for the release @devrnt !

[ekilah]

this library adds a new piece of data to window: window.intercomSettings. this piece of data contains data about the intercom user, like their name, email, and even the user_hash, which is a security token used to guarantee the user chatting with you is who they say they are.

This data should be cleared whenever logging out of your application, otherwise that user_hash and other user information is sitting around in the tab after the user has logged out.

shutdown is the API you are supposed to call when logging out, so this function should do this cleanup, too.

hardShutdown does this already, but it also deletes window.Intercom. That's problematic because if someone logs back in in the same session/tab, you don't want window.Intercom to be missing (nothing adds it back)

[devrnt]

Hi, thanks for creating this PR, really appreciate it.

Some remarks: at the moment the shutdown, follows the suggested shutdown flow, see https://www.intercom.com/help/en/articles/16845-how-do-i-end-a-session. I can't find any docs/fora where it's recommended to delete the window.intercomSettings.
If I can't trace back where the cleanup is recommended I'll keep the recommended "shutdown flow".

[ekilah]

I am under the impression that the Intercom library does not read window.intercomSettings directly, though I don't have proof of that one way or another.

Their "Basic Javascript" docs do tell you to set window.intercomSettings, but then the code blob that it tells you to paste right under that just uses that in the update call it makes. I imagine that for non-SPAs the window variable there is just used to have a global you can edit later if needed.

Right below those docs are the separate Single Page Application docs and those instructions never tell you to set window.intercomSettings, and you can see instead the docs just tell you to call window.Intercom('boot')(and update, etc) with the raw settings object as needed. This library's implementation follows the SPA docs by calling window.Intercom('boot'), 'update', etc, but for some reason it also sets window.intercomSettings.

So, it looks like this library is doing a bit of both. I imagine you could just stop setting intercomSettings completely in this library without affecting React SPAs, but I didn't aim to make that large of a change. It would be easy to test - either Intercom works, or it doesn't. I would be shocked if Intercom's JS bundle required window.intercomSettings to be set while also requiring an object in the boot call, but I've seen weirder.

The goal of this PR is just to delete the data about the previously-logged-in user after logging out, which is what the Intercom('shutdown') API already does (by deleting their own cookies). This change is in the spirit of their shutdown docs, though I admit their docs don't mention clearing this directly. I will email their support team, because I think it should be included there, too.

---

Without this PR's change, if someone logs out of a website using Intercom and leaves the tab open, an attacker could read window.intercomSettings, copy the user_hash stored there, log in to the application as a different user, set their own user_hash to the stolen one, and pretend to be the first user. This is just as bad as not clearing the cookies, which shutdown already does.

[devrnt]

Thanks for taking your time and explaining this, really appreciate it! I can totally follow your need to remove window.intercomSettings, but as you mentioned I'll wait for the response to your mail. If you receive no response I'll probably merge this anyway, but let's make sure that we don't break any current behaviour

[ekilah]

Sounds good! I sent Intercom a message, we'll see what they say. I'll update here when I receive a response.

[ekilah]

Got a reply yesterday. Their response confirms that they don't expect an SPA integration like this one to be setting window.intercomSettings at all:

Thanks for the report and your patience!

The basic javascript section of the installation guide assumes that the website will be "...a web app with multiple pages where each one triggers a new page refresh..." which means the previous window.intercomSettings object should be destroyed on page navigation (i.e. logout -> homepage). Whereas the single page installation section assumes the opposite, so examples in that section don't set the window.intercomSettings object directly – showing instead to pass the information directly into the messenger's JS API calls.

[...] there isn't a security vulnerability here but we could update our documentation on how to end sessions so it's more explicit about this. I'll follow up with the team to have that updated appropriately – hope that's helpful.

Thanks,
Josh

[devrnt]

Thanks for the update! so TLDR, window.intercomSettings isn't relevant when Intercom is used in an SPA? The Intercom API expects you to pass in all options/settings through the arguments of the according method?

[ekilah]

Correct...! My PR here doesn't remove it completely, but we/you/I could do that instead, or separately.

[hjoelh]

Hey guys, sounds good. Any news here?

[ekilah]

Hi there @hjoelh - I am mostly waiting for @devrnt to reply. I think this PR is mergeable now, and I'd be happy to follow up with a second PR to remove window.intercomSettings completely if that was desired also.

We could combine them into one PR as well if that is easier on @devrnt , I don't have much of a preference, it's just a slightly larger change I think.

My concern was mostly about making sure this library cleaned up after itself, which this PR does already!

[trevorwhealy]

Noticing the same behavior here. Authenticated user sessions are still persisting even after shutdown is called which is exposing the user-protected Intercom articles and chat messages to a logged out user

[ekilah]

@trevorwhealy feel free to delete window.intercomSettings yourself after you call shutdown until this merges, I tested it on my own application and still have that running in production waiting for this to be released 🤓

[ekilah]

🥳

[devrnt]

v2.0.0

[ekilah]

Thanks for the release @devrnt !