feat: Iterate on toolbar iframe message passing and api endpoint domain #81942

ryan953 · 2024-12-10T21:35:59Z

This moves the request-login and request-logout messages back into the window.postMessage handler, and reserves the MessageChannel and ports for logged-in api traffic only.
Overall the code is reorganized to better make the 3 message event listeners clearer:

did-login event listener on the window. Guarded to only accept messages from the same domain as this page is on (aka, from sentry itself). This is how the cookie/token gets moved around.
request-login & request-logout event listener on the window. A separate handler that guards for messages from the referrerOrigin only. The referrer should also be the same as window.parent.location.origin, but we can't query for that value directly.
MessageChannel ports event listener. This is only setup after the server has validated login, and checked project permissions. The ports are setup using postMessage(..., referrerOrigin) where referrerOrigin was validated on the server and passed back from the server, so we're very sure that we're setting up the MessageChannel against the correct domain only.

These three concerns are split up, and there's some extra code to make things more explicit. For example, we have explicit handleXYZ() methods that have some copy+pasted delegation code inside. This is intentionally separate from the const fooMessageDispatch = {...} objects as these dispatchers make it easier to see which commands or events are supported by each handler.

I've also injected regionUrl into the template, so the fetch command doesn't rely on the SDK setting the correct sentry api domain. Instead we can prefix api requets with the correct domain, and at the same time insert the cookie/token value to the request. This means that tokens/cookies will not be sent to any domain other than regionUrl, which is set by the server.

This diagram might be helpful for reviewers. It shows the interactions between the SDK, this /iframe/ page, and the /login-success/ page which emits the did-login message. It was tough to express in that sequence diagram the guards that are in place, i'll have to draw up another one with that info.
What is shown are

the 2 messages that can come in from the SDK: request-login and request-logout
the 1 message from the /login-success/ page: did-login
the port-connect system, which happens only after state=logged-in
when a state change is required, we emit the stale event and rely on the SDK to reload this iframe page. Internal state like that never mutates, we take state from the server at all times.

src/sentry/templates/sentry/toolbar/iframe.html

billyvg · 2024-12-11T18:56:42Z

src/sentry/templates/sentry/toolbar/iframe.html

+              document.cookie = getCookieValue(cookie, window.location.hostname);
+              document.cookie = getCookieValue(cookie, regionUrl);


why save cookies to both of these domains?

window.location.hostname is going to be used when we reload the iframe, it's like demo.sentry.io
and regionUrl is going to be used for api calls. It'll look like us.sentry.io

It's possible to make the api calls against the org-subdomain, but @silo region calls behave differently when the org is in de region :(

Making requests to the org subdomain will go to control and should be routed to the right region based on the org slug. However, that does require that all the APIs you're calling have org slugs in them. However, going to the region servers is better than going through the control proxy.

Ah yeah, so when i started calling /users/${userId}/ it required changes again to get the regionUrl.

lesson for the front-end if we ever try to get away from that window.__initialData json

src/sentry/toolbar/views/iframe_view.py

markstory · 2024-12-12T15:50:56Z

src/sentry/templates/sentry/toolbar/iframe.html

+              document.cookie = getCookieValue(cookie, window.location.hostname);
+              document.cookie = getCookieValue(cookie, regionUrl);


Making requests to the org subdomain will go to control and should be routed to the right region based on the org slug. However, that does require that all the APIs you're calling have org slugs in them. However, going to the region servers is better than going through the control proxy.

markstory · 2024-12-12T15:51:20Z

src/sentry/templates/sentry/toolbar/iframe.html

+
+        const parentWindowMessageDispatch = {
+          'request-login': ({ delay_ms }) => {
+            const origin = window.location.origin.endsWith('.sentry.io')


Does this need to work for single-tenant or self-hosted? The system.base-hostname or system.url-prefix runtime options have the bare host, and protocol + host respectively. You could use those options to make this work in other environments.

ideally it'll work on ST or Self-hosted.

this is here specifically because our redirect logic is messy. There's some notes here about redirects: #80003

tl/dr: In SaaS if we include the subdomain acme.sentry.io in the initial request, but the user needs to login, then we'll forget the redirect target and dump them into /issues. But if we ask for sentry.io/some-page/ then the user will end up at acme.sentry.io/some-page/ in all cases.

…eChannel (#142) Related to: getsentry/sentry#81942 Changes: - The `request-login` and `request-logout` messages are going over `iframe.contentWindow.postMessage()` now. The MessagePort is reserved for when the iframe is logged in and properly configured. - When state _changes_ we dispose of any ports (would only been connected if we were previously logged in) and reload the iframe. This ensures that we get a new state told to us from the html file - We've dropped the `sentryApiPath` config value, because the config endpoint (domain, region if needed, and this prefix) is set inside the iframe js scripts. The `fetch` command of the proxy can only make fetch requests to the sentry api now, not to other domains.

Our fetch requests are cross-domain by design, we're making requests from a customer domain like `acme.sentry.io` into a an region like `us.sentry.io`. Therefore we need to include credentials. This is also how the website is configured: https://github.com/getsentry/sentry/blob/aa22f5d2373fc19e224bc9cc1fb30c405f05d782/static/app/api.tsx#L338 More docs: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch#including_credentials Previously we had `same-origin` which worked because we weren't using the regionUrl. We were instead sending api requests to the customer domain at `acme.sentry.io`. This meant that api requests were slower than using the regionUrl, but and also we were unable to make requests for api endpoints that don't include an `/:organization/` segment. So using the regionUrl is an improvement, and this PR updates the credentials field to match. Another thing to consider is that before we were using `window.location.origin` to make requests. That has since been replaced by https://github.com/getsentry/sentry/blob/aa22f5d2373fc19e224bc9cc1fb30c405f05d782/src/sentry/templates/sentry/toolbar/iframe.html#L31 so we can trust that we're always sending these credentials off to a domain that the server trusts and told us about. Related to #81942

…in (#81942) Related to getsentry/sentry-toolbar#142 This moves the `request-login` and `request-logout` messages back into the window.postMessage handler, and reserves the MessageChannel and ports for logged-in api traffic only. Overall the code is reorganized to better make the 3 message event listeners clearer: - `did-login` event listener on the window. Guarded to only accept messages from the same domain as this page is on (aka, from sentry itself). This is how the cookie/token gets moved around. - `request-login` & `request-logout` event listener on the window. A separate handler that guards for messages from the `referrerOrigin` only. The referrer should also be the same as window.parent.location.origin, but we can't query for that value directly. - `MessageChannel` ports event listener. This is only setup after the server has validated login, and checked project permissions. The ports are setup using `postMessage(..., referrerOrigin)` where `referrerOrigin` was validated on the server and passed back from the server, so we're very sure that we're setting up the MessageChannel against the correct domain only. These three concerns are split up, and there's some extra code to make things more explicit. For example, we have explicit `handleXYZ()` methods that have some copy+pasted delegation code inside. This is intentionally separate from the `const fooMessageDispatch = {...}` objects as these dispatchers make it easier to see which commands or events are supported by each handler. I've also injected `regionUrl` into the template, so the `fetch` command doesn't rely on the SDK setting the correct sentry api domain. Instead we can prefix api requets with the correct domain, and at the same time insert the cookie/token value to the request. This means that tokens/cookies will not be sent to any domain other than `regionUrl`, which is set by the server. [This diagram](https://www.mermaidchart.com/play?utm_source=mermaid_js&utm_medium=banner_ad&utm_campaign=teams#pako:eNqlU7FuwjAQ_RWLJUGQQDsilamdSiWkDl1YTHxN3Sa2azswlP57z7GbhhAEFAnkJPfu3d17569BJhkMZgMDnxWIDO45zTUtV4IQRbXlGVdUWPJ8_0iocUc3wl8RDi448Y9dQCFzLup4_bQSDkArK0VVrkG7N8eezOfunBEuuI2H7rOGzBKdr2k8HRP83U7dmd7UwSZrFFqYESazqgRhU6oUCBYHXOjQFfhFbrlgcptSxh42mLDgxoIAHUclGENziMbkjQpWwMK1_FKjn3xoSK5nXVKN8RO0iZcjkKo6JVXS2JARG0st7GvhUxg3iIOl1EFIFCPIXlhSp92hFzmwRFbWE5zQukdtf6aZFLYZZq-_SLudMjapfY8apr8RR345miEl2hZHkw7eL1CtSEdphwe9X5RxFgqOSSblB4eDwsn8cGU81KQGbHwk7QxLIhS3gKgr2hFbGkR7iTQkSMv8xWhZB4WB4B0XG1rgmErLd2fbbtcJMFlSlKz5Hszm4kyvr3UbtypqkfVcFQbFgfZ9CRcrfo7mZ6lOOqJfqmFr6FMjKOwuQWUFcuLWutdhx_3_XFDHs1cIHTq-1D1oo9ptNIf7D75_AInT_g4) might be helpful for reviewers. It shows the interactions between the SDK, this `/iframe/` page, and the `/login-success/` page which emits the `did-login` message. It was tough to express in that sequence diagram the guards that are in place, i'll have to draw up another one with that info. What is shown are - the 2 messages that can come in from the SDK: `request-login` and `request-logout` - the 1 message from the `/login-success/` page: `did-login` - the `port-connect` system, which happens only after `state=logged-in` - when a state change is required, we emit the `stale` event and rely on the SDK to reload this iframe page. Internal state like that never mutates, we take state from the server at all times.

Our fetch requests are cross-domain by design, we're making requests from a customer domain like `acme.sentry.io` into a an region like `us.sentry.io`. Therefore we need to include credentials. This is also how the website is configured: https://github.com/getsentry/sentry/blob/aa22f5d2373fc19e224bc9cc1fb30c405f05d782/static/app/api.tsx#L338 More docs: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch#including_credentials Previously we had `same-origin` which worked because we weren't using the regionUrl. We were instead sending api requests to the customer domain at `acme.sentry.io`. This meant that api requests were slower than using the regionUrl, but and also we were unable to make requests for api endpoints that don't include an `/:organization/` segment. So using the regionUrl is an improvement, and this PR updates the credentials field to match. Another thing to consider is that before we were using `window.location.origin` to make requests. That has since been replaced by https://github.com/getsentry/sentry/blob/aa22f5d2373fc19e224bc9cc1fb30c405f05d782/src/sentry/templates/sentry/toolbar/iframe.html#L31 so we can trust that we're always sending these credentials off to a domain that the server trusts and told us about. Related to #81942

feat: Iterate on toolbar iframe message passing and api endpoint domain

1a5468d

ryan953 requested a review from a team December 10, 2024 21:36

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Dec 10, 2024

ryan953 mentioned this pull request Dec 10, 2024

feat: Iterate on ApiProxy to keep login/logout message off the MessageChannel getsentry/sentry-toolbar#142

Merged

billyvg reviewed Dec 11, 2024

View reviewed changes

snake_case

63e0c12

vercel bot deployed to Preview December 11, 2024 20:35 View deployment

ryan953 requested a review from billyvg December 11, 2024 21:09

markstory reviewed Dec 12, 2024

View reviewed changes

ryan953 requested a review from markstory December 12, 2024 16:42

billyvg approved these changes Dec 12, 2024

View reviewed changes

ryan953 merged commit 3e5d70b into master Dec 12, 2024
49 checks passed

ryan953 deleted the ryan953/toolbar-auth branch December 12, 2024 19:16

ryan953 mentioned this pull request Dec 13, 2024

fix(toolbar): Include credentials with fetch requests #82108

Merged

github-actions bot locked and limited conversation to collaborators Dec 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Iterate on toolbar iframe message passing and api endpoint domain #81942

feat: Iterate on toolbar iframe message passing and api endpoint domain #81942

ryan953 commented Dec 10, 2024 •

edited

Loading

billyvg Dec 11, 2024

ryan953 Dec 11, 2024

markstory Dec 12, 2024

ryan953 Dec 12, 2024

markstory Dec 12, 2024

markstory Dec 12, 2024

ryan953 Dec 12, 2024

		document.cookie = getCookieValue(cookie, window.location.hostname);
		document.cookie = getCookieValue(cookie, regionUrl);

feat: Iterate on toolbar iframe message passing and api endpoint domain #81942

feat: Iterate on toolbar iframe message passing and api endpoint domain #81942

Conversation

ryan953 commented Dec 10, 2024 • edited Loading

billyvg Dec 11, 2024

Choose a reason for hiding this comment

ryan953 Dec 11, 2024

Choose a reason for hiding this comment

markstory Dec 12, 2024

Choose a reason for hiding this comment

ryan953 Dec 12, 2024

Choose a reason for hiding this comment

markstory Dec 12, 2024

Choose a reason for hiding this comment

markstory Dec 12, 2024

Choose a reason for hiding this comment

ryan953 Dec 12, 2024

Choose a reason for hiding this comment

ryan953 commented Dec 10, 2024 •

edited

Loading