fix(status): Send response with 404 when ENOENT is found

[tristan957]

PR Checklist

Please check if your PR fulfills the following requirements:

• [x] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• [ ] Tests for the changes have been added (for bug fixes / features)
• [ ] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #139

What is the new behavior?

Sends the response back with a 404 when ENOENT is included in the error message.

Does this PR introduce a breaking change?

[x] Yes
[ ] No

Maybe it is a breaking change. If you are someone who is catching 500 level errors in a catch all interceptor and checking for ENOENT existence in your error message, then chances are you would be affected by this change.

[tristan957]

@kamilmysliwiec I am not familiar with Fastify, so I would not feel comfortable trying to make a similar change, nor do I know if the same behavior even exists in the FastifyAdapter.

I am looking into testing now.

[tristan957]

I am actually not sure how to test this is my project. @kamilmysliwiec would appreciate a tip here. I thought npm link and adding it to my project would do the trick but no dice.

[micaelmbagira]

@tristan957 Yes you need to clone, build and link it npm linkin the clone repo and npm link @nestjs/serve-static in your project.

[tristan957]

Hi @micaelmbagira I will give that a try again hopefully some time this week. Maybe I forgot to build it last time I tried, and had only linked it.

[micaelmbagira]

@kamilmysliwiec what's the status on this one? Is there anything left to do or can it be merged?

[tristan957]

@micaelmbagira really just needs to be tested I think. I haven't had a chance recently.

[micaelmbagira]

@tristan957 Will do now

[tristan957]

Sweet let me know how it goes.

[tristan957]

@micaelmbagira I committed your suggested change. Does everything work for you now?

[tristan957]

Whoops looks like an import statement is needed according CircleCI. @micaelmbagira I can add you as a contrib on my fork or you can suggest the change and I can commit it again. Later today I can squash the commits

[micaelmbagira]

@tristan957 yes you can add me

[tristan957]

you should be in now

[micaelmbagira]

@tristan957 @kamilmysliwiec Done, please let me know if any more changes needed. In any case, seems to work fine

[tristan957]

@micaelmbagira thanks for your work :)

[micaelmbagira]

@kamilmysliwiec could you review the changes? 🙂

[micaelmbagira]

@kamilmysliwiec Hello? Can we merge that one? Or is there anything to be adjusted?

[micaelmbagira]

~Yes, I will make the change for fastify.loader.ts.~ serve-static seems to be broken (not working at all) with fastify...

Regarding the release, it's actually a bug fix IMO: it should not return 500 if the files are not found. What do you think?

[jmcdo29]

serve-static seems to be broken (not working at all) with fastify...

How so? I have a working example on my machine.

[micaelmbagira]

@jmcdo29 Never mind it was my bad. So, I found out the issue is not caused by express.static nor fastify-static (they both behave as expected when the requested file does not exist) but by app.get(options.renderPath, renderFn);.

With fastify, the fix is not easy because res.send(stream) causes the error to happen inside fastify. Hence I decided to use fs.exists. For express, I scoped the error handling to res.sendFile (because that's what triggers the error) instead of having this general error handler.

Overall, I don't think the solution looks super nice but couldn't come up with something better...

@kamilmysliwiec if you have time to give it a look

[donnd-t]

Can this be merged? And not as a major release as this is a bug IMO. Besides the current behaviour being wrong I doubt if my security department will allow me to deploy any app with a broken HTTP status like this. A merge would be much appreciated otherwise I will need to fork or move to an alternate solution.

However, there is one problem with the PR. The response includes the full internal path of the file that wasn't found. This is a security issue as it exposes information about the internal server. If the file name is included in the error in needs to be relative to the public endpoint. Also, it would be nice is the error was completely customizable.

[tristan957]

I am in the same boat.

[gax97]

Any updates?

[gabdusch]

Any updates?

[art1c0]

Any updates?

[adalinesimonian]

Is there any interest from the maintainers to review this PR? It's been open for a few years but at least to my eyes looks like it was in a working state.

Folks have been working around this by creating global exception filters, but then if an ENOENT springs up from somewhere else in your application when you don't expect it, it gets handled like a 404, which is not desired.

Edit: Turns out global filters don't quite work for this. Nest's logic checks if any filters apply by using the type that's passed to the @Catch(MyError) decorator, and if there's a match, your filter takes over with no way to fall through. So in order to catch ENOENT errors I'd have to use @Catch(Error) and then I lose the ability to allow any other filters to handle the error. i.e. there's no way just to handle errors with error.code === 'ENOENT'.

[NeoDobby]

Any updates?

[tristan957]

I have given up on getting this merged, and I no longer work with Nest.

[Lokkve0126]

you can do it with

app.get(renderPath, async (req: any, res: any) => {
          await new Promise((resolve, reject) => {
            const stream = fs.createReadStream(indexFilePath);
            stream.on("data", (chunk) => {
              res.type('text/html').send(chunk);
              resolve(undefined);
            });
            stream.on("error", () => {
              reject(new NotFoundException("Not found"));
            });
          });
        });