Respect url search param in callback URL` about remix-auth-oauth2 HOT 9 OPEN

Hi @Katarina-UBCO and @sergiodxa!

I was looking to do the same thing and stumbled upon this solution. This solution doesn't work with all OAuth2 APIs, since some strictly enforce that the callback URL must precisely match a specified string (so an appended dynamic query string is not allowed). The Discord OAuth2 API is an example.

I did some searching and I think the correct approach is to use the OAuth2 state parameter (article). Currently, remix-auth-oauth2 generates this state on the fly (as a uuid) and uses it as a nonce to verify that the request round-trip hasn't been tampered with (a best practice!).

I think we can should be able to pass in our own state as part of this state parameter, in addition to the nonce.

The following patches should help make this clear. I'm sure the variable names and function names could be improved (e.g. generateState is a bit awkward now receiving state), but this was the minimal diff.

Here's the patch for remix-auth-oauth2:

diff --git a/node_modules/remix-auth-oauth2/build/index.js b/node_modules/remix-auth-oauth2/build/index.js
index 4d8335b..bcba1b4 100644
--- a/node_modules/remix-auth-oauth2/build/index.js
+++ b/node_modules/remix-auth-oauth2/build/index.js
@@ -80,7 +80,7 @@ class OAuth2Strategy extends remix_auth_1.Strategy {
         // Redirect the user to the callback URL
         if (url.pathname !== callbackURL.pathname) {
             debug("Redirecting to callback URL");
-            let state = this.generateState();
+            let state = this.generateState(options.state);
             debug("State", state);
             session.set(this.sessionStateKey, state);
             throw (0, server_runtime_1.redirect)(this.getAuthorizationURL(request, state).toString(), {
@@ -206,8 +206,9 @@ class OAuth2Strategy extends remix_auth_1.Strategy {
         url.search = params.toString();
         return url;
     }
-    generateState() {
-        return (0, uuid_1.v4)();
+    generateState(state) {
+        const str = JSON.stringify({ nonce: (0, uuid_1.v4)(), ...state });
+        return Buffer.from(str).toString("base64");
     }
     /**
      * Format the data to be sent in the request body to the token endpoint.

And the patch for remix-auth (types):

diff --git a/node_modules/remix-auth/build/authenticator.d.ts b/node_modules/remix-auth/build/authenticator.d.ts
index 70223ef..3cc99ce 100644
--- a/node_modules/remix-auth/build/authenticator.d.ts
+++ b/node_modules/remix-auth/build/authenticator.d.ts
@@ -78,7 +78,7 @@ export declare class Authenticator<User = unknown> {
      *   });
      * };
      */
-    authenticate(strategy: string, request: Request, options?: Pick<AuthenticateOptions, "successRedirect" | "failureRedirect" | "throwOnError" | "context">): Promise<User>;
+    authenticate(strategy: string, request: Request, options?: Pick<AuthenticateOptions, "successRedirect" | "failureRedirect" | "throwOnError" | "context" | "state">): Promise<User>;
     /**
      * Call this to check if the user is authenticated. It will return a Promise
      * with the user object or null, you can use this to check if the user is
diff --git a/node_modules/remix-auth/build/strategy.d.ts b/node_modules/remix-auth/build/strategy.d.ts
index 7c106e7..d29be39 100644
--- a/node_modules/remix-auth/build/strategy.d.ts
+++ b/node_modules/remix-auth/build/strategy.d.ts
@@ -31,6 +31,11 @@ export interface AuthenticateOptions {
      * If not defined, it will return null
      */
     failureRedirect?: string;
+    /**
+     * The optional state to include in the request.
+     * If not defined, it will return null
+     */
+    state?: any;
     /**
      * Set if the strategy should throw an error instead of a Reponse in case of
      * a failed authentication.
      *

In our application code, the login route can pass along whatever state is needed:

// app/routes/auth/login/tsx
import type { ActionArgs } from "@remix-run/node";
import { SocialsProvider } from "remix-auth-socials";
import { authenticator } from "~/auth.server";

export async function action({ request }: ActionArgs) {
  const url = new URL(request.url);
  const returnTo = url.searchParams.get("returnTo") || "/notes";
  return authenticator.authenticate(SocialsProvider.DISCORD, request, {
    failureRedirect: "/",
    state: { returnTo },
  });
}

And the callback route can parse the state query parameter from the request URL to retrieve it:

// app/routes/auth/callback.tsx
import type { LoaderArgs } from "@remix-run/node";
import { SocialsProvider } from "remix-auth-socials";
import { authenticator } from "~/auth.server";

const decodeBase64 = (data: string) => {
  return Buffer.from(data, "base64").toString("ascii");
};

const getOAuth2State = (request: Request) => {
  const url = new URL(request.url);
  const state = url.searchParams.get("state");
  if (!state) {
    return null;
  }
  const decoded = decodeBase64(state);
  return JSON.parse(decoded);
};

export async function loader({ request }: LoaderArgs) {
  const state = getOAuth2State(request);
  return authenticator.authenticate(SocialsProvider.DISCORD, request, {
    successRedirect: state?.returnTo || "/notes",
    failureRedirect: "/auth/login",
  });
}

from remix-auth-oauth2.

I like your idea of making the behavior a first class feature of the library. I imagine folks may have other use cases for putting metadata into state, but I agree that custom state and redirect management should be distinct features.

From an API point of view, maybe it's an option to specify which query parameter key might have a redirect target in it? Something like redirectQueryParam: "redirectTo", which then gets passed in to both of your suggested successRedirect and failureRedirect function handlers?

from remix-auth-oauth2.

Great. Going to implement it and create a PR and we can tune it over there? Thanks for the quick reply ;)

from remix-auth-oauth2.

I think the best strategy would be to enable users to set their own state/nonce, as mentioned by @sgoodrow. Instead of encoding the state in the actual URL (which might not be supported) you could use a tiny cache, like this:

// app/routes/auth.login.tsx
import type { ActionArgs } from "@remix-run/node";
import { authenticator } from "~/auth.server";
import stateCache from "~/stateCache.server";
import uuid from "uuid";

export async function action({ request }: ActionArgs) {
  const url = new URL(request.url);
  const returnTo = url.searchParams.get("returnTo") || "/";
  
  const state = uuid();
  stateCache.set(state, {
    returnTo,
  });
  
  return authenticator.authenticate("provider", request, {
    state,
  });
}

And in the callback route:

// app/routes/auth.callback.tsx
import type { LoaderArgs } from "@remix-run/node";
import { authenticator } from "~/auth.server";
import stateCache from "~/stateCache.server";

export async function loader({ request }: LoaderArgs) {
  const url = new URL(request.url);
  const urlState = url.searchParams.get("state");
  
  const cachedState = stateCache.get(urlState);
  
  return authenticator.authenticate("provider", request, {
    successRedirect: cachedState?.returnTo,
    failureRedirect: "/auth/login",
  });
}

This is the same solution used by others such as Auth0, and would mean no breaking changes for users of this library.
I'd be happy to put a PR if people agree, otherwise I'd have to continue using my own fork 😅

from remix-auth-oauth2.

The draft PR open #46

from remix-auth-oauth2.

I've used patch-package just to quickly fix it in our project and here is the diff file:

diff --git a/node_modules/remix-auth-oauth2/build/index.js b/node_modules/remix-auth-oauth2/build/index.js
index 771570a..b6a8a62 100644
--- a/node_modules/remix-auth-oauth2/build/index.js
+++ b/node_modules/remix-auth-oauth2/build/index.js
@@ -179,14 +179,20 @@ class OAuth2Strategy extends remix_auth_1.Strategy {
         };
     }
     getCallbackURL(url) {
+        // https://github.com/sergiodxa/remix-auth-oauth2/issues/45
+        let callbackURL;
         if (this.callbackURL.startsWith("http:") ||
             this.callbackURL.startsWith("https:")) {
-            return new URL(this.callbackURL);
+            callbackURL = new URL(this.callbackURL);
+        } else if (this.callbackURL.startsWith("/")) {
+            callbackURL = new URL(this.callbackURL, url);
+        } else {
+          callbackURL = new URL(`${url.protocol}//${this.callbackURL}`);
         }
-        if (this.callbackURL.startsWith("/")) {
-            return new URL(this.callbackURL, url);
+        if (!callbackURL.searchParams.get('redirect_uri') && url.searchParams.get('redirect_uri')) {
+          callbackURL.searchParams.set('redirect_uri', url.searchParams.get('redirect_uri'));
         }
-        return new URL(`${url.protocol}//${this.callbackURL}`);
+        return callbackURL;
     }
     getAuthorizationURL(request, state) {
         let params = new URLSearchParams(this.authorizationParams(new URL(request.url).searchParams));

from remix-auth-oauth2.

Hi @sgoodrow and @sergiodxa

It's Katarina here (I no longer work at UBCO so will be using my personal account from now on).

Very good point about oauth @sgoodrow . I've actually found it in oauth2 spec (Surprisingly, auth0 doesn't implement it this way and allows you to redirect to callback with dynamic query params, and they are wrong!). So I had a bit more thought about this.

I think ideally, there is no need to change remix-auth as other strategies might not use the state. This is easy to solve. One way would be:

  async authenticate(
    request: Request,
    sessionStorage: SessionStorage,
    options: AuthenticateOptions & { state?: Record<string, unknown> }
  ): Promise<User> {
    // ...
  }

I've thought further though. And actually, we could use an existing! successRedirect option as follows:

1. Preserve encoded nonce + successRedirect as redirectTo in the state (in a way as @sgoodrow suggested) - when redirecting to auth provider.
2. When verifying (in callback) - Decode state from URL
3. After all checks have passed - call this.success with successRedirect set to redirectTo from the state (if set) otherwise fallback to options.successRedirect.

👆 can be used as follows:

// app/routes/auth/login/tsx
import type { ActionArgs } from "@remix-run/node";
import { SocialsProvider } from "remix-auth-socials";
import { authenticator } from "~/auth.server";

export async function action({ request }: ActionArgs) {
  const url = new URL(request.url);
  const returnTo = url.searchParams.get("returnTo");
  return authenticator.authenticate(SocialsProvider.DISCORD, request, {
    failureRedirect: "/",
    successRedirect: returnTo || undefined,
  });
}

This would preserve the successRedirect retrieved from the url.searchParams.get("returnTo") in the state as redirectTo.

// app/routes/auth/callback.tsx
import type { LoaderArgs } from "@remix-run/node";
import { SocialsProvider } from "remix-auth-socials";
import { authenticator } from "~/auth.server";

export async function loader({ request }: LoaderArgs) {
  return authenticator.authenticate(SocialsProvider.DISCORD, request, {
    successRedirect: "/notes",
    failureRedirect: "/auth/login",
  });
}

This would retrieve the redirectTo from the state and redirect there (if exists) or redirect to /notes if not set.

👆 solves only problem with dynamic redirect to, rather than having a completely customisable state compared to @sgoodrow solution. I find it less work for the consumer of the library though as they don't need to manually parse the state.

We could also eventually add a different option (flag) to authenticate options to indicate what to do when redirectTo is present in the state. Alternatively (what I like the most) we could make the successRedirect a string or a function, something like:

  async authenticate(
    request: Request,
    sessionStorage: SessionStorage,
    options: AuthenticateOptions & {
      successRedirect?: string | ((redirectTo?: string | undefined) => string);
    }
  ): Promise<User> {
    // ...
  }

And then:

// app/routes/auth/callback.tsx
import type { LoaderArgs } from "@remix-run/node";
import { SocialsProvider } from "remix-auth-socials";
import { authenticator } from "~/auth.server";

export async function loader({ request }: LoaderArgs) {
  return authenticator.authenticate(SocialsProvider.DISCORD, request, {
    successRedirect: (redirectTo) => redirectTo ?? "/notes",
    failureRedirect: "/auth/login",
  });
}

If we wanted to give the consumer more power, we could add the state option to the authenticate method (as described on the top) and pass the parsed state as a parameter of the successRedirect function.

What do you guys think?

I'm keen on creating a PR with this as I've already prototyped it locally anyway 🤣

from remix-auth-oauth2.

I like your idea of making the behavior a first class feature of the library. I imagine folks may have other use cases for putting metadata into state, but I agree that custom state and redirect management should be distinct features.

From an API point of view, maybe it's an option to specify which query parameter key might have a redirect target in it? Something like redirectQueryParam: "redirectTo", which then gets passed in to both of your suggested successRedirect and failureRedirect function handlers?

Good idea. To improve on it a bit, maybe we could pass in the entire redirect string. This would help in scenarios where it may be necessary to pass in multiple query parameters (i.e. ?redirectTo=gohere&param1=one&param2=two)

from remix-auth-oauth2.

I think ideally, there is no need to change remix-auth as other strategies might not use the state.

While this is true, I think it could be documented in such a way that it's clear it's only relevant when using some strategies but not others. I also think exposing the raw state is preferable because one might have a different reason to save state data other than redirecting. By exposing state directly, you allow for more flexibility.

from remix-auth-oauth2.