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: support lavamoat webpack plugin #30134

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from
Draft

Conversation

weizman
Copy link
Member

@weizman weizman commented Feb 5, 2025

This PR focuses on preparing webpack plugin to work well with MetaMask effort to integrate it (see LavaMoat/LavaMoat#1526).

In order to do that, I had to:

  • b36e47a - add to the webpack config support in scuttling and unsafe context modules (this includes a new scuttling exception for webpack to avoid clashing with core webpack logic)
  • 6c0201e - update policy json file
  • 60239a4 - fix contentscript and inpage crashes so they don't clash with webpack core logic nor scuttling logic (see more feat: support lavamoat webpack plugin #30134 (comment))

These make it so that when integrated with the MetaMask PR, compilation and execution of the app works well

Copy link
Contributor

github-actions bot commented Feb 5, 2025

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

Copy link

socket-security bot commented Feb 5, 2025

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@lavamoat/[email protected] eval Transitive: environment +7 9.54 MB lmbot

View full report↗︎

Copy link

socket-security bot commented Feb 5, 2025

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert Package NoteSourceCI
New author npm/@lavamoat/[email protected] 🚫
Obfuscated code npm/@lavamoat/[email protected] 🚫
New author npm/[email protected] 🚫
New author npm/[email protected] 🚫

View full report↗︎

Next steps

What is new author?

A new npm collaborator published a version of the package for the first time. New collaborators are usually benign additions to a project, but do indicate a change to the security surface area of a package.

Scrutinize new collaborator additions to packages because they now have the ability to publish code into your dependency tree. Packages should avoid frequent or unnecessary additions or changes to publishing rights.

What is obfuscated code?

Obfuscated files are intentionally packed to hide their behavior. This could be a sign of malware.

Packages should not obfuscate their code. Consider not using packages with obfuscated code.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Feb 5, 2025
generatePolicy: true,
runChecks: true, // Candidate to disable later for performance. useful in debugging invalid JS errors, but unless the audit proves me wrong this is probably not improving security.
readableResourceIds: true,
inlineLockdown: /^runtime|contentscript\.js/u,

Check failure

Code scanning / CodeQL

Missing regular expression anchor High

Misleading operator precedence. The subexpression '^runtime' is anchored at the beginning, but the other parts of this regular expression are not

Copilot Autofix AI 3 days ago

To fix the problem, we need to ensure that both parts of the regular expression are properly anchored. This can be done by using non-capturing groups and applying the ^ anchor to both alternatives. The corrected regular expression should be /^(runtime|contentscript\.js)/u.

Suggested changeset 1
development/webpack/webpack.config.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/development/webpack/webpack.config.ts b/development/webpack/webpack.config.ts
--- a/development/webpack/webpack.config.ts
+++ b/development/webpack/webpack.config.ts
@@ -191,3 +191,3 @@
       readableResourceIds: true,
-      inlineLockdown: /^runtime|contentscript\.js/u,
+      inlineLockdown: /^(runtime|contentscript\.js)/u,
       unlockedChunksUnsafe: /inpage\.js/u,
EOF
@@ -191,3 +191,3 @@
readableResourceIds: true,
inlineLockdown: /^runtime|contentscript\.js/u,
inlineLockdown: /^(runtime|contentscript\.js)/u,
unlockedChunksUnsafe: /inpage\.js/u,
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@weizman
Copy link
Member Author

weizman commented Feb 18, 2025

Update:

  • inpage failed because it tried to access chrome which was scuttled by contentscript
  • I fixed that temporarily by making inpage run before contentscript (instead of the opposite), but also changed inpage to add the textContent script asynchronously instead of synchronously, so that the actual logic runs only after contentscript as it should
    • this is probably not the right way to go about it, because it changes order of execution which most likely will cause problems
  • In addition, I identified this bug in my lavamoat pr feat(webpack): introduce enhancements to support webpack integration LavaMoat/LavaMoat#1526 (comment) which caused another error and prevented inpage from working
  • Given the temp change above (which should still be properly addressed) and the lavamoat bug fix, inpage now seems to load error-free

@weizman
Copy link
Member Author

weizman commented Feb 19, 2025

Update:

  • 13c545d makes inpage work properly without the need for replacing the order of it with contentscript
  • solved by:
    • moving the injection logic to a function that is declared in contentscript instead of inpage, where the scuttled primordials are still accessible
    • set the function to a globally accessible object (such as the document)
    • reducing inpage to just grabbing that function from the document object and invoking it
  • This results in the same effect, but without the errors

@weizman weizman force-pushed the webpack-lavamoat-new branch from 79d1b01 to 13c545d Compare February 24, 2025 15:43
@weizman weizman force-pushed the webpack-lavamoat-new branch from 13c545d to cb6fbb0 Compare February 24, 2025 15:52
@weizman weizman force-pushed the webpack-lavamoat-new branch from cb6fbb0 to 60239a4 Compare February 24, 2025 15:55
@weizman weizman changed the title draft - try handle 2m old diff feat: support lavamoat webpack plugin Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
INVALID-PR-TEMPLATE PR's body doesn't match template team-lavamoat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants