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

Is eval strictly necessary? #47

Open
JudahGabriel opened this issue Jan 22, 2025 · 4 comments
Open

Is eval strictly necessary? #47

JudahGabriel opened this issue Jan 22, 2025 · 4 comments

Comments

@JudahGabriel
Copy link

JudahGabriel commented Jan 22, 2025

Is eval strictly necessary in this library?

This library uses eval("(-1).toFixed(-1);") to trigger an exception and determine the browser from the exception message length.

What's the problem? Most secure web apps have a Content Security Policy (CSP) that prevents eval. Indeed, at many big corporations, such as GitHub and Microsoft, CSPs that prevent eval is required. Try it now in your browser: hit F12 and in the console, paste eval("(-1).toFixed(-1);") - you'll see GitHub prevents it:

Image

Looking at the code closer, the real exception is calling toFixed(-1). And this library is using the exception message length to determine the browser. It's not at all clear why eval is needed at all. Is it?

@Joe12387
Copy link
Owner

Joe12387 commented Feb 4, 2025

Hi there,

Shortly after I added the new (-1).toFixed(-1) method, someone requested via a PR that this be encapsulated, I don't recall why. If this is causing issues for you, feel free to submit a PR reversing it.

Thanks,
Joe

@coder0107git
Copy link

Seems like it was added in #45 to stop bundlers from optimizing it away.

@JudahGabriel
Copy link
Author

JudahGabriel commented Feb 5, 2025

Interesting. Perhaps we can avoid this by doing a parseInt call on the strings, then passing those to toFixed. That should also prevent bundlers from optimizing it away:

function feid (): number {
   let toFixedEngineID = 0;
   let neg = parseInt("-1");
   try {
      neg.toFixed(neg);
   } catch (e) {
     toFixedEngineID = (e as Error).message.length 
   }
   return toFixedEngineID
}

To know for sure if this works, we'd need to try it on detectIncognito's bundler. (Does it use one?)

Joe12387 added a commit that referenced this issue Feb 9, 2025
@Joe12387
Copy link
Owner

Joe12387 commented Feb 9, 2025

@JudahGabriel I committed that snippet, we'll see if that causes any issues. If it doesn't, I'll push it to npm.

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

No branches or pull requests

3 participants