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: Replace usage of eval to obfuscate binary path from bundlers #1374

Merged
merged 3 commits into from
Oct 31, 2022

Conversation

timfish
Copy link
Contributor

@timfish timfish commented Oct 25, 2022

eval is not ideal because it causes Rollup to display warnings.

As per my test repo, it's reasonably easy to obfuscate paths from @vercel/nft without using eval as the evaluator can only simplify so far.

This PR adds a test to check that the binaries are not picked up by @vercel/nft.

For the purposes of demonstrating the newly added test, the first commit replaces the eval usage with path.resolve() which will cause the test to fail as the binary is picked up.

A subsequent commit will add the mildly obfuscated path generation.

Copy link
Member

@lforst lforst left a comment

Choose a reason for hiding this comment

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

nice 👍 (I'll hold off on merging in case somebody still has concerns about our plans)

@timfish
Copy link
Contributor Author

timfish commented Oct 27, 2022

I guess if this needs to be released as 1.x I should have branched off the 1.x branch!

@lforst
Copy link
Member

lforst commented Oct 27, 2022

I guess if this needs to be released as 1.x I should have branched off the 1.x branch!

Probably. Is it possible for you to still change the base?

Copy link
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

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

Nice work figuring out a successful obfuscation strategy. @lforst and @Lms24 and I tried like the dickens for like an hour one day and couldn't manage to trick it.

I guess if this needs to be released as 1.x I should have branched off the 1.x branch!

P.S. Does anyone remember why the webpack plugin is stuck on sentry-cli 1.x? @kamilogorek, maybe? In any case, I think it actually needs to be merged into both 1.x and 2.x, right? Because eventually we'll upgrade, and we'll want to have it there.

@timfish
Copy link
Contributor Author

timfish commented Oct 28, 2022

Does anyone remember why the webpack plugin is stuck on sentry-cli 1.x?

Node.js support:

getsentry/sentry-webpack-plugin#383 (comment)

@timfish
Copy link
Contributor Author

timfish commented Oct 28, 2022

Nice work figuring out a successful obfuscation strategy

I have "history" with it's predecessor @vercel/webpack-asset-relocator so when nft was released I spent far too long looking through the strategies it uses and experimented with the evaluation it performs and was left suitably impressed!

@lforst lforst changed the title feat: Replace usage of eval to obfuscate binary path from bundlers ref: Replace usage of eval to obfuscate binary path from bundlers Oct 31, 2022
@lforst lforst changed the title ref: Replace usage of eval to obfuscate binary path from bundlers fix: Replace usage of eval to obfuscate binary path from bundlers Oct 31, 2022
@lforst lforst merged commit 5d26b56 into getsentry:master Oct 31, 2022
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.

3 participants