SR-478 Add cross origin support#9
Conversation
jakewski
left a comment
jakewski
left a comment
There was a problem hiding this comment.
nice let's make an upstream PR as well though plz 🙏 🙏
| recordDOM?: boolean; | ||
| recordCanvas?: boolean; | ||
| recordCrossOriginIframes?: boolean; | ||
| allowedOrigins?: string[]; |
There was a problem hiding this comment.
bit confusing as a top level option, might think it stops recording on certain origins. how about something like allowedIframeOrigins
| let validatedOrigins: ReadonlySet<string> | undefined; | ||
| if (recordCrossOriginIframes) { | ||
| if (!allowedOrigins || allowedOrigins.length === 0) { | ||
| recordCrossOriginIframes = false; |
There was a problem hiding this comment.
this is a breaking change right.. we should make the default behavior * like it was before so that it's not as controversial as an upstream contribution (which we should totally do)
mixpanel can require the origins to be set in our own sdk
| // Drop messages from origins not in the allowlist. | ||
| (this.allowedOrigins && | ||
| !this.allowedOrigins.has(crossOriginMessageEvent.origin)) |
There was a problem hiding this comment.
hmm I'd be inclined to remove this condition. let the child iframe decide where it posts its data, and the parent origin just accepts whatever. this way you can be more granular about specifying allowed origins for child vs parent
the security improvement we're adding here comes from the child only posting to specific origins
wdyt?
There was a problem hiding this comment.
I think we should keep this change because you could have unintended rrweb data from a child iframe posting its data to the parent, right?
There was a problem hiding this comment.
yeah but I don't think that's a security issue. the annoying tradeoff here is that the parent page will need to keep track of a list of all possible iframe origins it intends to embed, but the child page only really needs to keep track of a few.
honestly if we keep this it should probably be a different list of origins in the argument. also we can potentially block this at the mixpanel SDK level as well right?
|
| const config: recordOptions<eventWithTime> = { | ||
| recordCrossOriginIframes: !!allowedOrigins, | ||
| recordCanvas: true, | ||
| allowedOrigins: allowedOrigins, |
There was a problem hiding this comment.
isn't this renamed? is this test not working or not running
There was a problem hiding this comment.
test was working since we had the fallback to all origins '*'. Ideally should have another assertion to check the target origin in the post message or another test entirely.
| recordDOM = true, | ||
| recordCanvas = false, | ||
| recordCrossOriginIframes = false, | ||
| recordCrossOriginIframes: _recordCrossOriginIframes = false, |
There was a problem hiding this comment.
this was from before, but we don't need this anymore. reverted
| // Drop messages from origins not in the allowlist. | ||
| (this.allowedOrigins && | ||
| !this.allowedOrigins.has(crossOriginMessageEvent.origin)) |
There was a problem hiding this comment.
yeah but I don't think that's a security issue. the annoying tradeoff here is that the parent page will need to keep track of a list of all possible iframe origins it intends to embed, but the child page only really needs to keep track of a few.
honestly if we keep this it should probably be a different list of origins in the argument. also we can potentially block this at the mixpanel SDK level as well right?
jakewski
left a comment
jakewski
left a comment
There was a problem hiding this comment.
sweet thx for bearing with the feedback
2 participants
Upstream rrweb-io#1800
This pull request introduces a new allowlist mechanism for cross-origin iframe recording, improving both security and control over which origins can participate in cross-origin event recording.
allowedIframeOriginsoption to therecordAPI (recordOptions), allowing users to specify a list of permitted origins for cross-origin iframe event recording.buildAllowedOriginSet, validates and normalizes this list.IframeManagerto accept and use this allowlist, and to filter incoming postMessages so only those from allowed origins are processed.*).These changes collectively make cross-origin iframe recording more secure and configurable