Skip to content
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

Add origin filtering for expose #605

Merged
merged 4 commits into from
Jan 24, 2023

Conversation

benjamind
Copy link
Collaborator

Partial fix for #603 allowing a user to specify an origin filtering regexp/string array to control which origins comlink will listen to.

Not sure if a full fix for #603 would require prevention of prototype changes, but I feel like if you expose on window you're always going to be a bit vulnerable unless you specify a filtering origin.

ep.addEventListener("message", function callback(ev: MessageEvent) {
if (!ev || !ev.data) {
return;
}
if (!isAllowedOrigin(origins, ev.origin)) {
console.warn(`Invalid origin '${ev.origin}' for comlink proxy`);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure exactly how to handle invalid messages, a warning seems useful, throwing an exception can be pretty hard to handle in this case but also seems more correct. Thoughts @surma ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree with a warning.

Copy link
Collaborator

@surma surma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate you taking this in. One variable naming nit, then we can merge and release!

src/comlink.ts Outdated Show resolved Hide resolved
ep.addEventListener("message", function callback(ev: MessageEvent) {
if (!ev || !ev.data) {
return;
}
if (!isAllowedOrigin(origins, ev.origin)) {
console.warn(`Invalid origin '${ev.origin}' for comlink proxy`);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree with a warning.

@benjamind
Copy link
Collaborator Author

Hah, @surma it appears you need to sign the Google CLA :-)

https://github.com/GoogleChromeLabs/comlink/pull/605/checks?check_run_id=10831963910

@surma
Copy link
Collaborator

surma commented Jan 24, 2023

It doesn’t let me add an alternate email address for some reason. But I can override. So Imma go ahead an merge this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants