Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add support to allow only specific cross origins #1800
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add support to allow only specific cross origins #1800
Changes from all commits
bd6c019057cfd9c84c70d04515d85bae041f1dcb5eFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The record options key here is
allowedOrigins, but the API/type added in this PR isallowedIframeOrigins. As written, the allowlist won’t be applied (and TypeScript should flag this as an unknown property onrecordOptions). Rename this option toallowedIframeOriginsso the test actually exercises the new behavior.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object.freeze(set)does not actually make aSetimmutable (callers can still mutate it viaadd/delete), so this can give a false sense of safety. If the goal is to prevent mutation, consider returning a defensive copy when exposing it, or avoiding the freeze and relying on theReadonlySettype (and not exporting the mutable instance).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buildAllowedOriginSetis designed to throw on an empty allowlist, butrecord()currently skips validation whenallowedIframeOrigins.length === 0, which meansallowedIframeOrigins: []silently falls back to wildcard behavior. If an empty array is intended to mean “deny all”, handle it explicitly; otherwise, call the validator whenever the option is present so the configuration error is surfaced consistently.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allowedIframeOriginsis treated as an allowlist, but if it normalizes to an empty set you currently setvalidatedOriginsback toundefined, which re-enables the wildcard ('*') behavior later. This is fail-open: a misconfigured/invalid allowlist ends up allowing all origins. Consider either throwing whenallowedIframeOriginsis provided but yields no valid origins, or keeping an empty set and treating it as “deny all” (no postMessage + drop all incoming messages).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
validatedOriginsis set, this loop sendspostMessagetowindow.parentfor each origin. If anytargetOrigindoesn't match the actual parent window origin, browsers throw aDOMExceptionand the recording will break (and the loop won't continue). SinceallowedIframeOriginsis also used for validating incoming iframe origins, it’s likely to contain origins that are not the parent origin. Consider resolving the parent origin once (e.g., fromdocument.referrerwhen available) and sending a singlepostMessage, or wrapping eachpostMessageintry/catchand continuing on mismatch.Check warning on line 561 in packages/rrweb/src/record/index.ts
packages/rrweb/src/record/index.ts#L561
Check warning on line 579 in packages/rrweb/src/record/index.ts
packages/rrweb/src/record/index.ts#L579
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.