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

When onError hook is defined, failure to compile template crashes server (ejs) #404

Closed
2 tasks done
mweberxyz opened this issue Feb 9, 2024 · 4 comments · Fixed by #405
Closed
2 tasks done

When onError hook is defined, failure to compile template crashes server (ejs) #404

mweberxyz opened this issue Feb 9, 2024 · 4 comments · Fixed by #405

Comments

@mweberxyz
Copy link
Contributor

mweberxyz commented Feb 9, 2024

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.26.0

Plugin version

8.2.0

Node.js version

20.x

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

13.6.4

Description

When fastify has an onError hook defined, attempts to render an invalid template result in a server crash due to multiple calls to send.

I am using the ejs engine and have only confirmed the bug with that engine.

Steps to Reproduce

const fastify = require("fastify")();

fastify.register(require("@fastify/view"), {
  engine: {
    ejs: require("ejs"),
  },
});

fastify.get("/", (req, reply) => {
  // Note the mistake in the ternary statement -- the second `?` should be a `:`
  reply.view({
    raw: `<p>Welcome to my <%= Math.random() > 0.5 ? 'site' ? 'place' %></p>`,
  });
});

fastify.addHook("onError", async (request, reply, err) => {
  console.error(err);
});

fastify.listen({ port: 3000 }, (err) => {
  if (err) throw err;
  console.log(`server listening on ${fastify.server.address().port}`);
});

Upon loading in a browser:
CleanShot 2024-02-09 at 10 47 23

After commenting out the onError hook:
CleanShot 2024-02-09 at 10 48 13

Expected Behavior

Regardless of whether onError has been defined, failure to compile template should return an error to the client, rather than crashing the server.

@mweberxyz mweberxyz changed the title When onError hook is defined, failure to compile template crashes server When onError hook is defined, failure to compile template crashes server (ejs) Feb 9, 2024
@mweberxyz
Copy link
Contributor Author

The first send occurs in the catch handler in readCallbackParser, which then returns undefined. Then in 3 cases, readCallbackEnd is called with the compile argument set to undefined, which catches an error (TypeError: compile is not a function), and that.send is called with the second error.

The double-call to send occurs whether the onError is defined or not, so the crash itself is a symptom of the existence of the handler.

@mweberxyz
Copy link
Contributor Author

This issue is solved by #405

@gurgunday
Copy link
Member

gurgunday commented Feb 18, 2024

Great!

Can you please add a unit test to #405 and put closes #404 there

mweberxyz added a commit to mweberxyz/point-of-view that referenced this issue Feb 19, 2024
Fixes fastify#404

Add a regression test for previously incorrect behavior: when the `fastify` instance had an `onError` added, errors thrown by renderer resulted in server crash.
mweberxyz added a commit to mweberxyz/point-of-view that referenced this issue Feb 19, 2024
Closes fastify#404

Add a regression test for previously incorrect behavior: when the `fastify` instance had an `onError` added, errors thrown by renderer resulted in server crash.
@mweberxyz
Copy link
Contributor Author

@gurgunday Added the test, but it didn't link this issue to the PR or vice versa. I assume because I am not a collaborator/member of this repo? 🤷‍♂️

mcollina pushed a commit that referenced this issue Feb 19, 2024
* refactor(index): async flow control

* refactor(index): factor up all html minification

* refactor(index): remove readCallbackEnd

* refactor(index): factor up all undefined `page` checks

* refactor(index): review nits

- viewDecorator: remove spurious defined check
- decorateReply: capitalize getHeader check
- minify definition: null instead of false when not enabled
- getRequestedPath: remove spurious optional chaining
- use optional chaining for layout checks
- avoid “assign to `x` then return `x`” pattern

* test(ejs): add test for invalid template

Closes #404

Add a regression test for previously incorrect behavior: when the `fastify` instance had an `onError` added, errors thrown by renderer resulted in server crash.

* refactor(index): avoid reading the same file concurrently

---------

Co-authored-by: uzlopak <aras.abbasi@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants