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

fix: update screenshot.js DOM text reinterpreted as HTML #2322

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

Shivam7-1
Copy link
Contributor

By using innerText, it will avoid the risk of HTML injection, as these properties automatically escape any HTML special characters in the provided text. This helps prevent cross-site scripting (XSS) vulnerabilities by treating the input as plain text rather than interpreted HTML. Always be cautious when dealing with user input or dynamic content to prevent security risks.

@Shivam7-1 Shivam7-1 requested a review from a team as a code owner April 14, 2024 06:53
@Shivam7-1 Shivam7-1 requested review from rachel-fenichel and removed request for a team April 14, 2024 06:53
Copy link

conventional-commit-lint-gcf bot commented Apr 14, 2024

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@Shivam7-1
Copy link
Contributor Author

Hi @rachel-fenichel Could You Please Review this PR
Thanks

@BeksOmega BeksOmega requested review from BeksOmega and removed request for rachel-fenichel April 17, 2024 16:27
Copy link
Contributor

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

Honestly can't figure out what this code is currently achieving by copying all of the text area values to the innerHTML. But can't hurt to change it over to innerText (especially when this should only be used for testing and not in production).

@BeksOmega BeksOmega changed the title Update screenshot.js DOM text reinterpreted as HTML fix: update screenshot.js DOM text reinterpreted as HTML Apr 17, 2024
@Shivam7-1
Copy link
Contributor Author

Hi @BeksOmega Thanks For Reviewing
Above PR was approved recently, and I wanted to check in to see if there's anything else needed from my side to proceed with the merge.

Thanks

@BeksOmega BeksOmega merged commit 7a5dcea into google:master Apr 18, 2024
9 of 10 checks passed
@BeksOmega
Copy link
Contributor

Nope you're all good @Shivam7-1 =) Thank you for the fix!

@NeilFraser
Copy link
Contributor

I agree @BeksOmega, this code (before or after this change) makes no sense to me. If the contents of these textareas are supposed to contain HTML code, then it was a vulnerability, and it is now broken. But I can't figure out where these textareas are. Anyone know how to test this? There's no mention of screenshots in the readme for this plugin.

@BeksOmega
Copy link
Contributor

You can right click the workspace and hit "download screenshot". Even if you comment out this code, it appears the screenshot still works (all text is included). I dug back into the history, but Sam committed it whole cloth to core, and there were no comments on the PR about this code.

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.

4 participants