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

tristan957 · 2020-05-13T01:26:26Z

PR Checklist

Please check if your PR fulfills the following requirements:

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 · 2020-05-13T01:28:06Z

@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 · 2020-05-13T01:37:28Z

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 · 2020-05-16T20:48:08Z

@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 · 2020-05-18T03:51:24Z

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.

lib/loaders/express.loader.ts

micaelmbagira · 2020-06-24T12:52:31Z

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

tristan957 · 2020-06-24T14:14:05Z

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

micaelmbagira · 2020-06-24T14:17:06Z

@tristan957 Will do now

lib/loaders/express.loader.ts

tristan957 · 2020-06-24T14:40:09Z

Sweet let me know how it goes.

tristan957 · 2020-06-24T14:42:02Z

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

tristan957 · 2020-06-24T14:44:29Z

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 · 2020-06-24T14:46:41Z

@tristan957 yes you can add me

tristan957 · 2020-06-24T14:50:47Z

you should be in now

micaelmbagira · 2020-06-24T14:59:39Z

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

tristan957 · 2020-06-24T15:07:16Z

lib/loaders/express.loader.ts

@@ -48,6 +48,14 @@ export class ExpressLoader extends AbstractLoader {
 app.use(express.static(clientPath, options.serveStaticOptions));
 app.get(options.renderPath, renderFn);
 }
+
+ app.use((err: Error, req, res, next: Function) => {


@micaelmbagira I just learned this recently, but express exports a type called NextFunction specifically for the next parameter. Might be better to use that instead.

Not sure if that would be useful in that case. We need to send the 404 to the client and I don't see any other way to do it

No your solution is good. I am just suggesting a narrower type on next. Right now we are using Function, but we could use https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/express/index.d.ts#L107. It would help narrow the typing down a bit :). Unless there is something that I am missing.

tristan957 · 2020-06-24T15:07:44Z

@micaelmbagira thanks for your work :)

micaelmbagira · 2020-07-08T14:14:45Z

@kamilmysliwiec could you review the changes? 🙂

micaelmbagira · 2020-08-29T20:15:40Z

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

jmcdo29

Overall the change looks pretty small, though due to a change in functionality it would probably need to wait until the next major version. Is there any chance we can get an update to fastify.lodader.ts as well, to make sure we have parity between adapters?

jmcdo29 · 2020-08-29T21:39:55Z

lib/loaders/express.loader.ts

@@ -48,6 +48,14 @@ export class ExpressLoader extends AbstractLoader {
 app.use(express.static(clientPath, options.serveStaticOptions));
 app.get(options.renderPath, renderFn);
 }
+
+ app.use((err: Error, req, res, next: Function) => {
+ if (err?.message?.includes('ENOENT')) {


Quick question: What happens if the route doesn't exist anywhere in the server? Would an error with ENOENT still come back, or does express take care of that in some other way?

If the route doesn't exist it returns 404

{ "response":{ "statusCode":404, "message":"ENOENT: no such file or directory, stat '/usr/local/foo/bar/index.html'", "error":"Not Found" }, "status":404, "message":"ENOENT: no such file or directory, stat '/usr/local/foo/bar/index.html'" }

I am testing my fix with this sample app that has no route but only ServeStaticModule pointing to a path that does not exist.

micaelmbagira · 2020-08-30T11:59:48Z

~~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 · 2020-08-30T17:50:20Z

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

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

Co-authored-by: Micaël Mbagira <[email protected]>

micaelmbagira · 2020-08-30T20:56:22Z

@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 · 2020-11-04T23:11:24Z

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 · 2020-11-05T00:10:04Z

I am in the same boat.

gax97 · 2021-04-15T16:44:29Z

Any updates?

gabdusch · 2021-08-29T14:02:52Z

Any updates?

adalinesimonian · 2022-09-19T20:36:37Z

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 · 2023-02-24T17:47:15Z

Any updates?

tristan957 · 2023-02-24T18:01:14Z

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

Lokkve0126 · 2023-05-01T15:35:26Z

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"));
            });
          });
        });

kamilmysliwiec reviewed May 23, 2020

View reviewed changes

lib/loaders/express.loader.ts Outdated Show resolved Hide resolved

kamilmysliwiec added the enhancement New feature or request label Jun 19, 2020

micaelmbagira approved these changes Jun 24, 2020

View reviewed changes

lib/loaders/express.loader.ts Outdated Show resolved Hide resolved

micaelmbagira force-pushed the 404-fix branch from 3566eae to 836d017 Compare June 24, 2020 14:57

tristan957 commented Jun 24, 2020

View reviewed changes

jmcdo29 suggested changes Aug 29, 2020

View reviewed changes

tristan957 and others added 5 commits August 30, 2020 22:48

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

f3598b8

fix: check for message being undefined

321479d

Send NotFoundException instead of setting the status code

18e54be

Co-authored-by: Micaël Mbagira <[email protected]>

fixup! Send NotFoundException instead of setting the status code

9376e1e

fixup! fixup! Send NotFoundException instead of setting the status code

3687a84

micaelmbagira force-pushed the 404-fix branch from 836d017 to 3687a84 Compare August 30, 2020 20:52

raphaelsoul mentioned this pull request Feb 17, 2022

There is not good way to catch "File not found" errors on static assets #139

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tristan957 commented May 13, 2020 •

edited

tristan957 commented May 13, 2020

tristan957 commented May 13, 2020

micaelmbagira commented May 16, 2020

tristan957 commented May 18, 2020

micaelmbagira commented Jun 24, 2020

tristan957 commented Jun 24, 2020

micaelmbagira commented Jun 24, 2020

tristan957 commented Jun 24, 2020

tristan957 commented Jun 24, 2020

tristan957 commented Jun 24, 2020

micaelmbagira commented Jun 24, 2020

tristan957 commented Jun 24, 2020

micaelmbagira commented Jun 24, 2020

tristan957 Jun 24, 2020 •

edited

micaelmbagira Jun 24, 2020

tristan957 Jun 24, 2020

tristan957 commented Jun 24, 2020

micaelmbagira commented Jul 8, 2020

micaelmbagira commented Aug 29, 2020

jmcdo29 left a comment

jmcdo29 Aug 29, 2020

micaelmbagira Aug 30, 2020 •

edited

micaelmbagira commented Aug 30, 2020

jmcdo29 commented Aug 30, 2020

micaelmbagira commented Aug 30, 2020

donnd-t commented Nov 4, 2020 •

edited

tristan957 commented Nov 5, 2020

gax97 commented Apr 15, 2021

gabdusch commented Aug 29, 2021

adalinesimonian commented Sep 19, 2022 •

edited

NeoDobby commented Feb 24, 2023

tristan957 commented Feb 24, 2023

Lokkve0126 commented May 1, 2023

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

Are you sure you want to change the base?

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

Conversation

tristan957 commented May 13, 2020 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

tristan957 commented May 13, 2020

tristan957 commented May 13, 2020

micaelmbagira commented May 16, 2020

tristan957 commented May 18, 2020

micaelmbagira commented Jun 24, 2020

tristan957 commented Jun 24, 2020

micaelmbagira commented Jun 24, 2020

tristan957 commented Jun 24, 2020

tristan957 commented Jun 24, 2020

tristan957 commented Jun 24, 2020

micaelmbagira commented Jun 24, 2020

tristan957 commented Jun 24, 2020

micaelmbagira commented Jun 24, 2020

tristan957 Jun 24, 2020 • edited

Choose a reason for hiding this comment

micaelmbagira Jun 24, 2020

Choose a reason for hiding this comment

tristan957 Jun 24, 2020

Choose a reason for hiding this comment

tristan957 commented Jun 24, 2020

micaelmbagira commented Jul 8, 2020

micaelmbagira commented Aug 29, 2020

jmcdo29 left a comment

Choose a reason for hiding this comment

jmcdo29 Aug 29, 2020

Choose a reason for hiding this comment

micaelmbagira Aug 30, 2020 • edited

Choose a reason for hiding this comment

micaelmbagira commented Aug 30, 2020

jmcdo29 commented Aug 30, 2020

micaelmbagira commented Aug 30, 2020

donnd-t commented Nov 4, 2020 • edited

tristan957 commented Nov 5, 2020

gax97 commented Apr 15, 2021

gabdusch commented Aug 29, 2021

adalinesimonian commented Sep 19, 2022 • edited

NeoDobby commented Feb 24, 2023

tristan957 commented Feb 24, 2023

Lokkve0126 commented May 1, 2023

tristan957 commented May 13, 2020 •

edited

tristan957 Jun 24, 2020 •

edited

micaelmbagira Aug 30, 2020 •

edited

donnd-t commented Nov 4, 2020 •

edited

adalinesimonian commented Sep 19, 2022 •

edited