Sandbox: Use toString to create observe and resize script string #42872
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
walbo
added
[Type] Enhancement
|
Size Change: -454 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
mirka
approved these changes
Aug 4, 2022
There was a problem hiding this comment.
Interesting! The reasoning makes sense and it seems like this would be beneficial in multiple ways.
From what I could gather, I don't think this would be problematic (security, reliability), but I would feel better if we got one more reviewer's approval as well.
walbo
requested review from
a team,
oandregal and
noahtallen
and removed request for
a team
I don't think there shoud be any security conserns, but having a final sign-off doesn't hurt 😄 Don't know who to request a review from to get that. @aaronrobertshaw Can you add someone from security team as a reviewer? Would also be good to get someone from the mobile team to make sure it works as expected in the mobile app as well. |
samueljseay
approved these changes
Aug 8, 2022
There was a problem hiding this comment.
My 2 cents from a security perspective is that I don't see an additional security threat being introduced here. This function is not modifiable as user input and there is already a way to pass your own function as a prop, a more immediate danger would be users of this component not being aware of the security implications of that.
To abuse this code would require introducing some kind of other XSS vulnerability to the site and at that point there would be easier ways to perform an attack.
dcalhoun
added
the
Mobile App - Automation
|
I opened wordpress-mobile/gutenberg-mobile#5083 to allow a few more mobile-specific CI tests to run against these changes. I hope to spend time testing the changes in the mobile editor within the next couple of days. |
dcalhoun
reviewed
Aug 9, 2022
There was a problem hiding this comment.
The additional mobile CI tasks passed in wordpress-mobile/gutenberg-mobile#5083. 🎉 Unfortunately, I discovered a regression on Android that I have outlined further in a separate comment.
If we address the Android regression, I do not see a reason why these changes cannot be made to the native mobile file. However, like other commenters, I would defer to others more experienced in security matters to comment on any possible vulnerabilities in these changes.
Thanks for the ping!
|
Hey all 👋 Is this PR ready to be merged? Since some time has passed, should we rebase it before doing so? |
Co-authored-by: David Calhoun <438664+dcalhoun@users.noreply.github.com>
walbo
force-pushed
the
try/sandbox-script
branch
from
df75807
to
c8678b9
Compare
I have rebased the PR. I belive it's ready to be merged. |
|
Makes sense to me — let's merge once CI is ✅ ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
Use
toStringon theobserveAndResizeJSfunction to create string used in the iframe.Why?
How?
Used
toStringinstead of hardcoding the function as a string.Testing Instructions
Test the editor where the
Sandboxcomponent is used. Ex theCustom HTMLblock in preview mode.