fastify / point-of-view

Template rendering plugin for Fastify
MIT License
335 stars 86 forks source link

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

Closed mweberxyz closed 5 months ago

mweberxyz commented 5 months ago

Prerequisites

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 commented 5 months ago

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 commented 5 months ago

This issue is solved by https://github.com/fastify/point-of-view/pull/405

gurgunday commented 5 months ago

Great!

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

mweberxyz commented 5 months ago

@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? 🤷‍♂️