vercel / next.js

The React Framework
https://nextjs.org
MIT License
125.09k stars 26.72k forks source link

Middleware can't detect image requests #68217

Open HidayetCanOzcan opened 1 month ago

HidayetCanOzcan commented 1 month ago

Link to the code that reproduces this issue

https://github.com/HidayetCanOzcan/middleware-bug

To Reproduce

  1. Start the application in dev mode (npm run dev)
  2. Check for the console to see if middleware pathname _next/image exist

Current vs. Expected behavior

After running the application I expected to see a log message that shows image requests but only pathnames listed is;

⚠️ /vercel.svg ⚠️ /_next/static/css/app/layout.css ⚠️ /_next/static/chunks/webpack.js ⚠️ /next.svg ⚠️ /_next/static/chunks/main-app.js ⚠️ /_next/static/chunks/app-pages-internals.js ⚠️ /_next/static/chunks/app/page.js ⚠️ /favicon.ico

/_next/image is missing

Provide environment information

Operating System:
Platform: win32
Arch: x64
Version: Windows 11 Pro
Binaries:
Node: 20.11.1
npm: 10.2.4
Yarn: N/A
pnpm: N/A
Relevant packages:
next: 14.2.5
react: ^18
react-dom: ^18

Which area(s) are affected? (Select all that apply)

Image (next/image), Middleware

Which stage(s) are affected? (Select all that apply)

next dev (local), next build (local), next start (local)

Additional context

I also tried to add matcher for the image path and also try to add it to config file as image path but none of them resolve the issue.

AliRazaDev27 commented 1 month ago

Can you provide more info regarding what you actually want to do? As the middleware run on every incoming request and prints the pathname, when you do npm run dev, it starts statically generating the pages, thus you see those logs in your console, Now i don't see any imge reference in your code, from where are you expecting this _next/image request ?

HidayetCanOzcan commented 1 month ago

No reference needed, middleware should be able to detect and intercept before image optimization work.

icyJoseph commented 1 month ago

Alright, so the problem seems to come from here, normalizeAndAttachMetadata, when used like so:

        finished = await this.normalizeAndAttachMetadata(req, res, parsedUrl)
        if (finished) return
        finished = await this.handleCatchallMiddlewareRequest( // We never arrive here for `/_next/image`

This call, returns finished true, and then the middleware is never run for /_next/image paths.

I wonder, if, through some code refactor, the case where /_next/image paths, had to also run their middleware, has been accidentally sequenced so that, the image is served and considered a finished request. Some of these changes are 11 months old... but I'll try to see the git diff, to try and spot a refactor mishap.

icyJoseph commented 1 month ago

Ok, I think I have a fix, PR coming up!

HidayetCanOzcan commented 1 month ago

Thanks @icyJoseph

icyJoseph commented 1 month ago

I'm still investigating. Though in that PR the fix - works... I'm not sure about it. I'm also adding a test to prevent this regressing again.

It would be good to narrow down which version introduced this.. 🤔

HidayetCanOzcan commented 1 month ago

I'm not so sure either, I didn't realize it until I put the image optimization end into security testing.

icyJoseph commented 1 month ago

For more reference, I've been able to narrow down where the issue was introduced:

In 13.5.7-canary.17, the middleware catches /_next/image, but in 13.5.7-canary.18, it doesn't anymore.

KoenBrouwer commented 1 month ago

Just FYI: I actually stumbled on this issue a while ago, when I was working on 2 ideas:

  1. Since we have a multi-tenant kind of application, where - on build time - the host on which the application will be exposed, and the URL of our CMS backend are unknown, we need to have the remotePatterns parameter be dynamic, based on an environment variable. This is not possible in next.config.js in standalone mode. This could be fairly easily done when /_next/image is routed through the middleware.

  2. We found out that the the quality parameter can be abused to keep generating new cached images and therefor using our system resources. The solution is to only allow a certain range or set of values, eg. 20, 50, 80. This is also not possible in next.config.js, but this could also easily be done in middleware.

HidayetCanOzcan commented 1 month ago

Actually, I tried to access this feature in middleware for the same reason. For now, we continue by allowing only certain hosts and certain sizes as you said. If we can trigger it in image requests, we will continue by adding a customized headers. But thanks for the heads up anyway :)

KoenBrouwer commented 2 weeks ago

Any updates on this? Next/image is virtually unusable in its current state. Maybe have a maintainer look at this?

giri-jeedigunta commented 2 days ago

Any further updates or guidelines on how to address this?

Edit by maintainer bot: Comment was automatically minimized because it was considered unhelpful. (If you think this was by mistake, let us know). Please only comment if it adds context to the issue. If you want to express that you have the same problem, use the upvote 👍 on the issue description or subscribe to the issue for updates. Thanks!