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

eval not allowed in Middleware pages/foo/_middleware #30674

Closed
mauriciosoares opened this issue Oct 30, 2021 · 16 comments · Fixed by #30883
Closed

eval not allowed in Middleware pages/foo/_middleware #30674

mauriciosoares opened this issue Oct 30, 2021 · 16 comments · Fixed by #30883
Labels
bug Issue was opened via the bug report template. Middleware Related to Next.js Middleware

Comments

@mauriciosoares
Copy link

What version of Next.js are you using?

12.0.1

What version of Node.js are you using?

14.17.6

What browser are you using?

Chrome

What operating system are you using?

macOS

How are you deploying your application?

not deployed, tested on localhost

Describe the Bug

Whenever I try to use the module jsonwebtoken inside a middleware, I got the following warnings on the console:

warn  - ./node_modules/events/events.js
`eval` not allowed in Middleware pages/foo/_middleware

./node_modules/function-bind/implementation.js
`eval` not allowed in Middleware pages/foo/_middleware

./node_modules/function-bind/index.js
`eval` not allowed in Middleware pages/foo/_middleware

./node_modules/get-intrinsic/index.js
`eval` not allowed in Middleware pages/foo/_middleware

./node_modules/has/src/index.js
`eval` not allowed in Middleware pages/foo/_middleware

./node_modules/is-generator-function/index.js
`eval` not allowed in Middleware pages/foo/_middleware

./node_modules/lodash.isplainobject/index.js
`eval` not allowed in Middleware pages/foo/_middleware

./node_modules/readable-stream/lib/_stream_writable.js
`eval` not allowed in Middleware pages/foo/_middleware

Expected Behavior

The app still works just fine, but I believe this warning shouldn't happen, considering that this module can be used on both front and back end side of my app.

Btw, I haven't done much with middlewares, so I was only able to reproduce this issue only with this specific npm module. If I notice that importing any other module logs the same warning I'll update the issue here.

To Reproduce

image

you can try it on this repository: https://github.com/mauriciosoares/middlwares-and-rewrites/tree/eval-not-allowed-in-middleware, using the branch eval-not-allowed-in-middleware.

To reproduce it you just need to access the url http://localhost:3000/foo/bar, and the warning should appear on the console.

@mauriciosoares mauriciosoares added the bug Issue was opened via the bug report template. label Oct 30, 2021
@mauriciosoares
Copy link
Author

mauriciosoares commented Oct 30, 2021

Also, seems to me that this warning only happens once after the server starts (or restarts due to any code change) and when I first enter a url that the middleware runs...

If I refresh the same url, then the warning doesn't seem to appear again

@tavindev
Copy link

According to the docs, eval is not allowed in Middlewares (Edge Functions). I assume you're trying to use jsonwebtoken for authentication, so unless you need the decoded token value, there's a lightweight version that works inside Edge Functions since it doesn't use eval at all

You can check the docs' example here

@timneutkens timneutkens added the Middleware Related to Next.js Middleware label Oct 31, 2021
@mauriciosoares
Copy link
Author

@gustavo-dev thanks for the explanation... so eval is not allowed on edge functions, but are all middlewares edge functions by default?

From my (very shallow) understanding of middlewares, they can be deployed as edge functions (which is an infra structure and concept provided by vercel... I think? 😂 ), but you don't necessarily need to...

So all edge functions are middlewares, but are all middlewares edge functions? 🤔

@tavindev
Copy link

tavindev commented Oct 31, 2021

I think you're mixing the concepts a little bit.

Edge Functions are similar to CDN's (but different).

A CDN (Content Delivery Network) is used to serve static content in a fast way. Therefore they need to be located close to the client. The downside with this approach is the inability to deliver custom content to different clients based on their location, for example.

Another method that could solve this would be processing each request on the server (SSR). However, the drawback now is speed because the server handling these requests is not necessarily close to your end-user.

That's where Edge Functions come in! They work just like a CDN but they allow you to run server-side logic on the Edge, close to your client, which enables us to achieve both speed and dynamism.

A middleware, on the other hand, is a function that runs before a request is completed. If you've used ExpressJs before, you should be familiar with this concept. They can modify the request/response objects and are useful for authentication, among other things.

Hence, Middlewares are not necessarily Edge Functions and Edge Functions are not necessarily Middlewares. However, when deploying to Vercel, Middleware is the only code that runs in Edge Functions.

Here are a few links regarding these concepts if you want:

@mauriciosoares
Copy link
Author

Ok, but my point is, Middlewares are not necessarily Edge functions, and Eval is not supported on edge functions as you mentioned on your comment...

Doesn't that mean that if I use eval on middlewares, that are NOT edge functions, they should work just fine, meaning that the warning here is not necessary?

@tavindev
Copy link

tavindev commented Nov 2, 2021

Sure, if you use eval on middlewares that are not Edge Functions, then it should work fine.

@mauriciosoares
Copy link
Author

And they do work, but now my terminal gets a ton of warnings because of something that I won't even be using... That makes debugging a bit harder unfortunately :/

Also since eval seems to work on middlewares, the log is very misleading, saying that "eval" not allowed in Middleware...

I'll wait for someone from the core team to share their opinion here, to know if this is intended or not.

@javivelasco
Copy link
Member

javivelasco commented Nov 2, 2021

Running Javascript code at the edge is very sensitive and providers are very conservative and ban certain APIs. Among many reasons there is speed or being able to optimize code by statically analyzing it.

The explanation from @gustavo-dev was great and indeed, you can use Next.js middleware with eval when running next start. On the other hand, if you deploy middleware for example to Vercel, eval will not in production.

In order to help people don't get into this situation, we are showing a warning in Next.js if we detect you are dynamically evaluating code. This warning will show up when the code is parsed. Note that currently in development there is no tree-shaking, so (at the time of writing this comment) it is possible that you get a warning about using eval in code that is required by the middleware but that will not make it to production since it may be removed on build thanks to tree-shaking.

@kodiakhq kodiakhq bot closed this as completed in #30883 Nov 5, 2021
kodiakhq bot pushed a commit that referenced this issue Nov 5, 2021
Co-authored-by: Tobias Koppers <sokra@users.noreply.github.com>

With this PR we are updating the way we check the usage of `eval` and other dynamic code evaluation (like `new Function`) for middleware. Now instead of simply showing a warning it will behave differently depending on if we are building or in development.

- Development: we replace the dynamic code with a wrapper so that we print a warning only when the code is used. We don't fail in this scenario as it is possible that once the application is built the code that uses `eval` is left out.
- Build: we detect with tree shaking if the code that will be bundled into the middleware includes any dynamic code and in such scenario we make the build fail as don't want to allow it for the production environment.

Closes #30674

## Bug

- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have helpful link attached, see `contributing.md`

## Documentation / Examples

- [ ] Make sure the linting passes by running `yarn lint`
@denik1981
Copy link

denik1981 commented Nov 6, 2021

Not sure about this one, but I'm facing the same error. I have looked deep into the code and I didn't find an eval statement in there. The complain I'm getting come from common supportive libraries like for example lodash.isplainObject.

@javivelasco What do you mean by

In order to help people don't get into this situation, we are showing a warning in Next.js if we detect you are dynamically evaluating code.

What does detect mean here? So it isn't just the eval() statement and more things can be consider dynamic? If so, could the team be kind and expand this definition of dynamic so we can trace and eventually fix the error.

@denik1981
Copy link

No worries. I think I found it. I have almost an exact match from the failure libraries of @javivelasco.

This is dyanmic evaluation without using eval from the get-intrinsic module.

var $Function = Function;
var getEvalledConstructor = function (expressionSyntax) {
	try {
		return $Function('"use strict"; return (' + expressionSyntax + ').constructor;')();
	} catch (e) {}
};

Maybe the docs in Next might expand this definition of no eval().
A future TODO task for us the community will be to find compatible Edge libraries for the most common ones cause I have an almost 90% match of the libraries that @javivelasco posted in here and I'm sure a lot of us will be facing this issue sooner or later.

@javivelasco
Copy link
Member

Yeah sorry about that. It's not only eval but any type of dynamic code indeed

@IRediTOTO
Copy link

According to the docs, eval is not allowed in Middlewares (Edge Functions). I assume you're trying to use jsonwebtoken for authentication, so unless you need the decoded token value, there's a lightweight version that works inside Edge Functions since it doesn't use eval at all

You can check the docs' example here

I tried your solution, but console still shown that error. Really not understand why, function still run and show result...
const verify = jwt.verify(token, 'secret',);

@tavindev
Copy link

@IRediTOTO Are you importing jwt from the right package?

import jwt from '@tsndr/cloudflare-worker-jwt'
//...

@IRediTOTO
Copy link

@gustavo-dev yes sure, maybe it come from my nextjs setup, something run when middle where run. Func still run fine, just show the warning. Actually I can use jose package in it.

@vvo
Copy link
Member

vvo commented Nov 28, 2021

Hey there, one thing I do not understand is where inside the readable-stream package is there any use of eval? I have been looking through the code and couldn't find any reference to it.

See: https://github.com/nodejs/readable-stream/blob/v3.6.0/lib/_stream_writable.js which is the one used when building middleware.

Thanks!

@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 27, 2022
natew pushed a commit to natew/next.js that referenced this issue Feb 16, 2022
Co-authored-by: Tobias Koppers <sokra@users.noreply.github.com>

With this PR we are updating the way we check the usage of `eval` and other dynamic code evaluation (like `new Function`) for middleware. Now instead of simply showing a warning it will behave differently depending on if we are building or in development.

- Development: we replace the dynamic code with a wrapper so that we print a warning only when the code is used. We don't fail in this scenario as it is possible that once the application is built the code that uses `eval` is left out.
- Build: we detect with tree shaking if the code that will be bundled into the middleware includes any dynamic code and in such scenario we make the build fail as don't want to allow it for the production environment.

Closes vercel#30674

## Bug

- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have helpful link attached, see `contributing.md`

## Documentation / Examples

- [ ] Make sure the linting passes by running `yarn lint`
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue was opened via the bug report template. Middleware Related to Next.js Middleware
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants