state for passport causes fastify-passport to fail

[vorillaz]

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.

[mcollina]

Thanks for reporting! You are probably the best person equipped for diagnosing and fixing this. Would you like to give it a go and send a PR?

[vorillaz]

Sure thing @mcollina , I'll come back at you as soon as I have a PR up and running.

[sameer-coder]

@vorillaz Did you get a chance to work on this one?

[sameer-coder]

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.

[simoneb]

@sameer-coder are you doing any work on this?

[sameer-coder]

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

[sameer-coder]

I have investigated the issue. fastify-passport works fine with fastify-session and state. There is an issue with state when fastify-secure-session is used but the issue is with the way passport-oauth2 handles the session and not with fastify-passport.
I believe we can close this issue.

[mcollina]

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

[climba03003]

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.

[mcollina]

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:
#319.

[sameer-coder]

Thanks @climba03003

Thanks @mcollina for the explanation.

So should we move from getter-setter to a plain object approach in fastify-secure-session? I can get started on that later today and create a PR soon.

[mcollina]

IMHO I think it would be better to work on #319 first and see if any changes are needed here. I don't think the OP cares about using fastify-secure-session - we just need to provide a way to be compatible with the rest of the passport ecosystem.

[sameer-coder]

Sure. I will see if there are any needed changes there. Thanks

[simoneb]

Is this being worked on as part of #421?

[sameer-coder]

Is this being worked on as part of #421?

yes

[simoneb]

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