fastify-passport: `state` for passport causes `fastify-passport` to fail

Prerequisites

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

Fastify version

3.22.1

Plugin version

0.4.3

Node.js version

16.x

Operating system

macOS

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

macOS Big Sure 11.3.1

Description

When I am trying to pass some encoded state to the strategy the callback URLs shows the user as unauthenticated, it’s also unclear how I could pass the encoded state from the redirect endpoint dynamically.

Steps to Reproduce

fastifyPassport.use(
    "github",
    new Github.Strategy(
      {
        clientID: "xxx",
        clientSecret: "xxx",
        callbackURL: "http://lvh.me:3000/callback",
        scope: ["user:email", "user,user:email"],
        userProfileURL: "https://api.github.com/user",
        state: Buffer.from(JSON.stringify({ returnTo: "/mypage" })).toString(
          "base64"
        ),
        passReqToCallback: true,
      },
      function (req, accessToken, refreshToken, profile, done) {
        done(null, { ...profile, accessToken, query });
      }
    )
  );

server.get(
    "/github",
    {
      preValidation: fastifyPassport.authenticate("github", {
        successRedirect: "/callback",
        authInfo: false,
        assignProperty: "user",
      }),
    },
    () => {}
  );
  
  server.get(
  "/callback",
  {
    preValidation: fastifyPassport.authenticate("github", {
      authInfo: false,
      session: false,
    }),
  },
  async (request, reply, err, user = {}, info = {}, status = {}) => {
    reply.send({ user: request.user || {} });
  }
);

Expected Behavior

Step through the prevalidation hook and return the encoded state to the endpoint.

About this issue

  • Original URL
  • State: closed
  • Created 3 years ago
  • Comments: 16 (15 by maintainers)

Most upvoted comments

Let’s link the PR so this issue is closed when the PR is merged then, please. Same with the other issue if not linked already

Thanks @climba03003 for summarizing the problem.

The reason why fastify-secure-session offers a get/set/delete methods is to protect against prototype pollution attacks. Maybe we should add an option there to use a plain object for it. I’m also ok in moving to a plain object, it’s not strictly needed anyway and familiarity is likely more important.

It’s interesting to note that @fastify/session use plain objects. So maybe the best option is to just support it out of the box: https://github.com/fastify/fastify-passport/issues/319.

Can you explain how this all work to cause this problem?

@mcollina The underlay issue is how passport consumer expect to use request.session. As passport is made for express, it expect to directly mutate the request.session object.

However, fastify-secure-session is expect to use .get, .set, .delete to update the state of session. It lead to the problem, when ever a passport strategy that require the usage of session, fastify-secure-session can never be supported.

Here comes to two solution.

  1. fastify-secure-session provide a proxy session object to support this use-case.
  2. fastify-passport provide a custom request, and bind to a proxy session object.

@simoneb ~I am no longer working on this~ I will take a look at this again today

This seems to be an issue with fastify-secure-session and not fastify-passport. I tried the snippet above using fastify-session(https://github.com/SerayaEryn/fastify-session) and it seems to work fine with state. I will dig deeper and try to find the issue.