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

feat: add security properties for iFrame #989

Merged
merged 2 commits into from
Jan 23, 2024
Merged

Conversation

Skaiir
Copy link
Contributor

@Skaiir Skaiir commented Jan 16, 2024

Closes #901

@Skaiir Skaiir requested a review from vsgoulart January 16, 2024 05:09
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Jan 16, 2024
@github-actions github-actions bot temporarily deployed to demo-901-iframe-security January 16, 2024 05:09 Destroyed
@github-actions github-actions bot temporarily deployed to demo-901-iframe-security January 16, 2024 05:09 Destroyed
@vsgoulart
Copy link
Contributor

@Skaiir Didn't @volodymyr-melnykc ask for the show security attributes toggle to be removed?

Copy link
Contributor

@vsgoulart vsgoulart left a comment

Choose a reason for hiding this comment

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

@Skaiir everything looks fine to me, we just need to check the previous comment I made about the toggle

@Skaiir
Copy link
Contributor Author

Skaiir commented Jan 20, 2024

@Skaiir Didn't @volodymyr-melnykc ask for the show security attributes toggle to be removed?

Yeah the PR was made before those comments, sorting this out now.

@github-actions github-actions bot temporarily deployed to demo-901-iframe-security January 20, 2024 16:14 Destroyed
@Skaiir
Copy link
Contributor Author

Skaiir commented Jan 20, 2024

Made some adjustments. I'm still a bit apprehensive with regards to this configuration here (ignore validation error).

grafik

This is an XSS attack surface, because if you've got allow-scripts and allow-same-origin that's the equivalent of the iframe basically not being a sandbox anymore, and yeah if there's malicious scripts it can elevate its own priviledges to doing everything, plus it has access to the session cookies, which means it can escalate into session hijacking.

Okay after some heavy reading I'm a little reassured. So, allow-same-origin only really de-sanboxes if the parent and iframe are on the same domain. This would mean that for an XSS attack to occur, it would require something on whatever js embedding domain to already be vulnerable to script injection. Anyways, I am going to rename allow-same-origin's user text from 'Storage and Cookies' to 'Allow same origin'. Because right now it gives the impression that you can't use basic website functionality without this set, which is not true. That way nobody will assume what it does and will have to read into it.

@github-actions github-actions bot temporarily deployed to demo-901-iframe-security January 20, 2024 17:08 Destroyed
@github-actions github-actions bot temporarily deployed to demo-901-iframe-security January 21, 2024 06:37 Destroyed
@Skaiir Skaiir requested a review from vsgoulart January 21, 2024 10:16
@github-actions github-actions bot temporarily deployed to demo-901-iframe-security January 22, 2024 06:12 Destroyed
@github-actions github-actions bot temporarily deployed to demo-901-iframe-security January 22, 2024 08:55 Destroyed
@github-actions github-actions bot temporarily deployed to demo-901-iframe-security January 22, 2024 09:00 Destroyed
Copy link
Contributor

@vsgoulart vsgoulart left a comment

Choose a reason for hiding this comment

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

LGTM

@Skaiir Skaiir merged commit 7478695 into develop Jan 23, 2024
13 of 14 checks passed
@Skaiir Skaiir deleted the 901-iframe-security branch January 23, 2024 01:14
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iFrame component: Support security attributes
3 participants