-
Notifications
You must be signed in to change notification settings - Fork 52
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
fix(status): Send response with 404 when ENOENT is found #207
base: master
Are you sure you want to change the base?
Conversation
@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. |
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. |
@tristan957 Yes you need to clone, build and link it |
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. |
@kamilmysliwiec what's the status on this one? Is there anything left to do or can it be merged? |
@micaelmbagira really just needs to be tested I think. I haven't had a chance recently. |
@tristan957 Will do now |
Sweet let me know how it goes. |
@micaelmbagira I committed your suggested change. Does everything work for you now? |
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 |
@tristan957 yes you can add me |
you should be in now |
@tristan957 @kamilmysliwiec Done, please let me know if any more changes needed. In any case, seems to work fine |
lib/loaders/express.loader.ts
Outdated
@@ -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) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@micaelmbagira thanks for your work :) |
@kamilmysliwiec could you review the changes? 🙂 |
@kamilmysliwiec Hello? Can we merge that one? Or is there anything to be adjusted? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
lib/loaders/express.loader.ts
Outdated
@@ -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')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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? |
How so? I have a working example on my machine. |
Co-authored-by: Micaël Mbagira <[email protected]>
@jmcdo29 Never mind it was my bad. With fastify, the fix is not easy because 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 |
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. |
I am in the same boat. |
Any updates? |
1 similar comment
Any updates? |
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 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 |
Any updates? |
I have given up on getting this merged, and I no longer work with Nest. |
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"));
});
});
}); |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
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?
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.