Skip to content

Remove eval from frontend bundle #9731

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.

Sign up for GitHub

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

Jump to bottom
Merged
kantuni merged 7 commits into from
Oct 26, 2024
Merged

Remove eval from frontend bundle #9731

kantuni merged 7 commits into from
Oct 26, 2024

Conversation

@kantuni
Copy link
Collaborator

@kantuni kantuni commented Oct 24, 2024 •
edited
Loading

Describe your changes

This PR blocks the use of eval in a browser environment. Please see this spec for more info on why we need to remove it.

This PR adds 2 packages: patch-package and postinstall-postinstall. The first one is used to patch @protobufjs/inquire which includes the eval (see protobufjs/protobuf.js#997 and protobufjs/protobuf.js#1548), and the second one is used to call postinstall on yarn remove as Yarn v1 only calls it on yarn install and yarn add.

GitHub Issue Link (if applicable)

SNOW-1554237

Testing Plan

  • No tests required as there are no implementation changes.

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@sfc-gh-vdonato sfc-gh-vdonato added security-assessment-completed Security assessment has been completed for PR change:other PR contains other type of change impact:internal PR changes only affect internal code labels Oct 24, 2024
@kantuni kantuni force-pushed the kantuni/remove-eval-from-frontend-bundle-v2 branch 2 times, most recently from ff5b2d8 to aada623 Compare October 24, 2024 21:54
@kantuni kantuni force-pushed the kantuni/remove-eval-from-frontend-bundle-v2 branch from aada623 to 1f366f8 Compare October 25, 2024 09:17
@raethlein
Copy link
Collaborator

raethlein commented Oct 25, 2024

No tests required as there are no implementation changes.

Do you have an idea of how we maintain this in the future, e.g. when we update the protobufjs version? When we update it and the patch does not work anymore / break things, will we see it in our e2e tests because it is completely broken?

@raethlein
Copy link
Collaborator

raethlein commented Oct 25, 2024

@kmcgrady Does this interfere with the frontend split effort / needs to be updated our are the changes compatible with each other?

@kantuni
Copy link
Collaborator Author

kantuni commented Oct 25, 2024

Do you have an idea of how we maintain this in the future, e.g. when we update the protobufjs version? When we update it and the patch does not work anymore / break things, will we see it in our e2e tests because it is completely broken?

Hopefully, the next upgrade won't contain that piece (considering the rant around it). But if it does - it's trivial to apply the same update to the future versions. You open the file containing the eval (in this case index.js) of the package you want to patch (in this case @protobufjs/inquire) change the code in place (i.e. add the if statement), then cd into frontend and run yarn patch-package PACKAGE_NAME. The patch file will be updated and you just commit that.

@kantuni
Copy link
Collaborator Author

kantuni commented Oct 25, 2024

@kmcgrady Does this interfere with the frontend split effort / needs to be updated our are the changes compatible with each other?

This change should be compatible even with new package managers, as most of them support this functionality out-of-the-box. For example, for pnpm neither the patch-package nor the postinstall-postinstall are needed.

kmcgrady
kmcgrady approved these changes Oct 25, 2024
View reviewed changes
Copy link
Collaborator

@kmcgrady kmcgrady left a comment

Choose a reason for hiding this comment

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

Yea. I don't think this should cause any issues with the frontend split work @raethlein. I approved it because the tests pass fine, and I assume this works fine in CSP-deployed environments.

@kantuni Will the install fail if someone upgrades the package? Would love to make sure we break hard if any changes occur.

@streamlit streamlit deleted a comment from sfc-gh-hkantuni Oct 25, 2024
@kantuni
Copy link
Collaborator Author

kantuni commented Oct 25, 2024 •
edited
Loading

Yea. I don't think this should cause any issues with the frontend split work @raethlein. I approved it because the tests pass fine, and I assume this works fine in CSP-deployed environments.

@kantuni Will the install fail if someone upgrades the package? Would love to make sure we break hard if any changes occur.

@kmcgrady The patch simply won't be applied to the next version. It will print out a message saying that no patches were applied. That should gives a cue. I haven't found a way to break hard from install as it's a "postinstall" script.

raethlein
raethlein approved these changes Oct 25, 2024
View reviewed changes
Copy link
Collaborator

@raethlein raethlein left a comment

Choose a reason for hiding this comment

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

SGTM!

@kantuni
Copy link
Collaborator Author

kantuni commented Oct 26, 2024 •
edited
Loading

I haven't found a way to break hard from install as it's a "postinstall" script.

Thanks to @kmcgrady who found the --error-on-fail option that will exit with code 1 if the patch-package fails. Gladly, it's enabled by default in CI mode - so no changes are necessary.

@kantuni kantuni merged commit d9ff42e into develop Oct 26, 2024
@whitphx whitphx mentioned this pull request Nov 14, 2024
Merged
edegp pushed a commit to edegp/streamlit that referenced this pull request Jan 19, 2025
@kantuni @edegp
Remove eval from frontend bundle (streamlit#9731)
8ba621a 
## Describe your changes

This PR blocks the use of `eval` in a browser environment. Please see
[this](https://docs.google.com/document/d/1g-fczG7eV5CIIUDd5pnwvGkUn-SLf2vMZeBk9qj1Yk4/)
spec for more info on why we need to remove it.

This PR adds 2 packages:
[patch-package](https://www.npmjs.com/package/patch-package) and
[postinstall-postinstall](https://www.npmjs.com/package/postinstall-postinstall).
The first one is used to patch `@protobufjs/inquire` which includes the
`eval` (see protobufjs/protobuf.js#997 and
protobufjs/protobuf.js#1548), and the second one
is used to call `postinstall` on `yarn remove` as Yarn v1 only calls it
on `yarn install` and `yarn add`.

## GitHub Issue Link (if applicable)

[SNOW-1554237](https://snowflakecomputing.atlassian.net/browse/SNOW-1554237)

## Testing Plan

- No tests required as there are no implementation changes.

---

**Contribution License Agreement**

By submitting this pull request you agree that all contributions to this
project are made under the Apache 2.0 license.
@lukasmasuch lukasmasuch deleted the kantuni/remove-eval-from-frontend-bundle-v2 branch March 7, 2025 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kmcgrady kmcgrady kmcgrady approved these changes

@raethlein raethlein raethlein approved these changes

@sfc-gh-vdonato sfc-gh-vdonato sfc-gh-vdonato approved these changes

Assignees

No one assigned

Labels

change:other PR contains other type of change impact:internal PR changes only affect internal code security-assessment-completed Security assessment has been completed for PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kantuni @raethlein @kmcgrady @sfc-gh-vdonato