`request.url` is mutated between `onRequest` and `onResponse`

[SimenB]

🐛 Bug Report

request.url is different in onRequest vs onResponse.

To Reproduce

Steps to reproduce the behavior:

const fastify = require('fastify')();
const fastifyExpress = require('fastify-express');
const fp = require('fastify-plugin');
const express = require('express');

fastify.register(
  fp(
    (instance, _, next) => {
      instance.addHook('onRequest', (request, _, next) => {
        console.log('req!', request.url);

        next();
      });

      instance.addHook('onResponse', (request, reply, next) => {
        console.log('res!', request.url);

        next();
      });

      next();
    },
    { fastify: '3.x' },
  ),
);

fastify.register(fastifyExpress).after(() => {
  const mainRouter = express.Router();

  const innerRouter = express.Router();

  mainRouter.use('/hubba', innerRouter);

  innerRouter.get('/bubba', (req, res) => {
    console.log('handler', req.url, req.originalUrl);

    res.sendStatus(200);
  });

  fastify.use(mainRouter);
});

fastify.listen(3000);

$ curl http://localhost:3000/hubba/bubba

This will log the following

req! /hubba/bubba
handler /bubba /hubba/bubba
res! /bubba

Expected behavior

I'd expect the logged output to be

req! /hubba/bubba
handler /bubba /hubba/bubba
res! /hubba/bubba

If not that, at least access to the original URL. It's on req.raw.originalUrl due to fastify-express putting it there during an onRequest, but that shouldn't be necessary.

Your Environment

• node version: 12.19.0
• fastify version: 3.6.0
• os: Mac
• any other relevant information

[mcollina]

It's express that is changing req.raw.url, as we do not copy this value to save memory: https://github.com/fastify/fastify/blob/a7948798fb88a2054c65f123996f53714ef2b1da/lib/request.js#L108.

This must be fixed in fastify-express to reset that value after express has finished executing.

I'm moving this to fastify-express as it's a bug there!

[SimenB]

Aha! It was easiest for me to reduce my local error down using fastify-express, didn't think the issue was actually here

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[SimenB]

Add a pinned label or what it's called so the bot leaves it alone?

[satya-nutella]

@mcollina is this issue still open to be worked on? If so can you point me to the right direction

[Eomm]

Go for it!

I think we should change this function:

https://github.com/fastify/fastify-express/blob/16cf8afe04b0d2406c7dc19f5e2cd77dbecd8f02/index.js#L54

providing a custom next callback to understand that express has ended its execution and apply the restore

For sure I would suggest to add a failing test as first step, then I would try to change the code 👍🏽

[satya-nutella]

@Eomm to clarify, will this next callback be passed as an additional parameter to runConnect (sorry if this question was silly, I am new here)

[Eomm]

@meehawk

no, I mean this:

  function runConnect (req, reply, next) {
    if (this[kMiddlewares].length > 0) {
      for (const [headerName, headerValue] of Object.entries(reply.getHeaders())) {
        reply.raw.setHeader(headerName, headerValue)
      }

      this.express(req.raw, reply.raw, customCallback)
    } else {
      next()
    }
    
    function customCallback(err){
       // here we know that express has done its work
       next(err)
    }

  }