-
-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Respect url search param in callback URL` #45
Comments
I've used 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)); |
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 I think we can should be able to pass in our own state as part of this The following patches should help make this clear. I'm sure the variable names and function names could be improved (e.g. Here's the patch for 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 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 // 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",
});
} |
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 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!
👆 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 // 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 👆 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 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 What do you guys think? I'm keen on creating a PR with this as I've already prototyped it locally anyway 🤣 |
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 |
Great. Going to implement it and create a PR and we can tune it over there? Thanks for the quick reply ;) |
The draft PR open #46 |
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. |
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 |
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. |
Hi Sergio, thanks heaps for all the open source work you've done for the community!
I'd like to request a feature, so that it is possible to redirect to somewhere after successful login. The use case:
/auth/login?redirect_uri=https://example.com/my-path-somewhere/123
which redirects them to the oauth2 provider, e.g. auth0 universal login page./auth/callback?redirect_uri=https://example.com/my-path-somewhere/123
.redirect_uri
from therequest.url
and set the thesuccessRedirect
to/my-path-somewhere/123
(or manually redirect).I think to implement this all what needs to be changed is the
getCallbackURL
method to respect the url search params of theurl
argument.I'm keen on doing this myself and contributing to the library, just let us know please if that's ok.
Cheers, Katarina
The text was updated successfully, but these errors were encountered: