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

[media] Check origin of messages #1643

Merged
merged 1 commit into from
Sep 26, 2023
Merged

Conversation

dayo09
Copy link
Contributor

@dayo09 dayo09 commented Sep 15, 2023

This commit checks origin of messages in event handlers

ONE-vscode-DCO-1.0-Signed-off-by: Dayoung Lee [email protected]


For #1624 (comment)

media/CircleGraph/index.js Outdated Show resolved Hide resolved
media/CircleGraph/index.js Outdated Show resolved Hide resolved
@dayo09 dayo09 force-pushed the 0915-check-origin branch 2 times, most recently from a28fe3d to 1fa31fb Compare September 25, 2023 11:55
Comment on lines +150 to +153
if (window.origin !== event.origin) {
console.log("Unexpected Origin: ", event.origin);
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seanshpark I noticed that there is a potential security problem from Verify the origin of the received message.

I assumed that the messages are coming from the window's origin, would it be okay for CircleGraph?

Copy link
Contributor

@seanshpark seanshpark Sep 26, 2023

Choose a reason for hiding this comment

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

I'm not sure about technical background of this, but I think event itself would be coming from vscode process(? or thread?, the node-js).
And not sure about the fix but if this has no problem loading the model, I think it would be OK.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree this changes because this is a similar solution from sonarsource.com.

https://rules.sonarsource.com/javascript/RSPEC-2819/

window.addEventListener("message", function(event) {

  if (event.origin !== "http://example.org") // Compliant
    return;

  console.log(event.data)
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jyoungyun I actually referred that solution :-D

@dayo09
Copy link
Contributor Author

dayo09 commented Sep 26, 2023

@jyoungyun PTAL :-D

(Should I Pr this drafts at once or separately?)

@jyoungyun jyoungyun marked this pull request as ready for review September 26, 2023 04:25
@jyoungyun
Copy link
Collaborator

@dayo09 Could you update the commit title so that there is no DRAFT: prefix. :)

This commit checks origin of messages in event handlers

ONE-vscode-DCO-1.0-Signed-off-by: Dayoung Lee <[email protected]>
@dayo09 dayo09 changed the title DRAFT: Check origin of messages [media] Check origin of messages Sep 26, 2023
@dayo09 dayo09 added the 2 approvals 2 approvals required to be merged label Sep 26, 2023
Copy link
Collaborator

@jyoungyun jyoungyun left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@seanshpark seanshpark left a comment

Choose a reason for hiding this comment

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

LGTM

@jyoungyun jyoungyun merged commit c2b6036 into Samsung:main Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 approvals 2 approvals required to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants