clerk / javascript

Official Javascript repository for Clerk authentication
https://clerk.com
MIT License
1.04k stars 235 forks source link

redirectToSignIn not respecting publishableKey passed to authMiddleware #1442

Open dcyoung opened 1 year ago

dcyoung commented 1 year ago

I'm using a custom env var naming scheme in order to provide env vars at deploy time. I am able to supply the publishableKey as a parameter to the middleware,

export default authMiddleware({
    publishableKey: process.env.ENV_VAR_NAME,
    async afterAuth(auth, req, evt) {
        if (!auth.isPublicRoute ) {
            if (!auth.userId) {
                // eslint-disable-next-line @typescript-eslint/no-unsafe-return
                return redirectToSignIn({

but when I call redirectToSignIn within afterAuth, I am getting an error:

Server Error
Error: Missing publishableKey. You can get your key at https://dashboard.clerk.com/last-active?path=api-keys.

This error happened while generating the page. Any console logs will be displayed in the terminal window.
Source
src/middleware.ts (45:40) @ afterAuth

  44 | // eslint-disable-next-line @typescript-eslint/no-unsafe-return
> 45 | return redirectToSignIn({
     |                        ^
  46 |     returnBackUrl: ...,
  48 | });
dimkl commented 1 year ago

Hello @dcyoung It seems that custom environment variables for that use case are not currently supported. Let me take a look and provide you with a workaround.

dimkl commented 1 year ago

Hello @dcyoung we are iterating over our API to support this case but currently, I would suggest you try to pass the specific env variable if it's possible. If it's not, I think you could use the draft code below.

// helper.ts - edited copy of packages/nextjs/src/server/redirect.ts
import { constants, redirect } from '@clerk/backend';
import { NextResponse } from 'next/server';

export const setHeader = (res: Response, name: string, val: string) => {
    res.headers.set(name, val);
    return res;
};

const redirectAdapter = (url: string) => {
    const res = NextResponse.redirect(url);
    return setHeader(res, constants.Headers.ClerkRedirectTo, 'true');
};

export const { redirectToSignIn, redirectToSignUp } = redirect({
  redirectAdapter,
  signInUrl: '<your value if needed>',
  signUpUrl: '<your value if needed>',
  publishableKey: '<your value if needed>',
});
// middleware.ts
import { customRedirectToSignIn } from './helper.ts'; // file above

authMiddleware({
    afterAuth: (auth: AuthObject, req) => {
      // ....
      return customRedirectToSignIn({ returnBackUrl: req.experimental_clerkUrl.href });
      // ...
    }
});

I will ping you back when a fix or workaround is available.

clerk-cookie commented 1 year ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

clerk-cookie commented 1 year ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

clerk-cookie commented 10 months ago

Hello 👋

We currently close issues after 40 days of inactivity. It's been 30 days since the last update here. If we missed this issue, please reply here. Otherwise, we'll close this issue in 10 days.

As a friendly reminder: The best way to see an issue fixed is to open a pull request. If you're not sure how to do that, please check out our contributing guide.

Thanks for being a part of the Clerk community! 🙏

dcyoung commented 10 months ago

Bump

kevinmitch14 commented 5 months ago

Could be related to https://github.com/clerk/javascript/pull/3001

dimkl commented 5 months ago

Hello @kevinmitch14, The https://github.com/clerk/javascript/pull/3001 is not related to redirecToSignIn / redirecToSignUp. The PR attempts to fix issues related to the redirects triggered by the authMiddleware() itself, not the helpers.

dimkl commented 5 months ago

Hello @dcyoung , We have attempted to solve the current issue in our new major version @clerk/nextjs@v5.0.0-beta.37 (currently in beta). Check the following example:

import { clerkMiddleware } from '@clerk/nextjs/server';

export default clerkMiddleware((auth, request, next)=>{
   if (!auth.userId) {
    return auth().redirectToSignIn();
   }
}, {
  publishableKey: 'env-or-whatever'
});

export const config = {
  matcher: ['/((?!.*\\..*|_next).*)', '/', '/(api|trpc)(.*)'],
};
kevinmitch14 commented 5 months ago

Hey @dimkl what I've found is that as soon as you pass a custom publishableKey in middleware. Eg. publishableKey: process.env.CUSTOM_PK_NAME or even hardcoding the publishableKey (publishableKey: "...").

You get the "Error: Missing publishableKey" error. It happens no matter what if you do not use the env variable name NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY.

I think that the environment variables are not read correctly when they are passed in any way other than the default env variable method used in the docs.

NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY="..."
CLERK_SECRET_KEY="..."

Furthermore, if you pass random values directly into the middleware and also have the ENV vars defined, the ENV vars take precedence over the passed values.

// This will still work if you have NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY defined in env vars.
authMiddleware({
  secretKey: "randomvalue",
  publishableKey: "randomvalue",
})
kevinmitch14 commented 5 months ago

Hello @dcyoung , We have attempted to solve the current issue in our new major version @clerk/nextjs@v5.0.0-beta.37 (currently in beta). Check the following example:

This still does not work as expected, the same issue persists. Error: @clerk/backend: Missing publishableKey. You can get your key at https://dashboard.clerk.com/last-active?path=api-keys.

If I pass a publishableKey to middleware but also define it as an ENV VAR, it will work because the ENV VAR overrides the passed value. But once I comment out the env var, the issue will come back.

dimkl commented 5 months ago

@kevinmitch14 You are correct, there is an issue with the publishableKey and secretKey parameters not being respected. We are looking into this and we will fix the issue with the parameters having priority over the env and avoid raising error when the env are not set for the internal code of the middleware and not for the redirect* helpers. I would like make clear that the redirectToSignIn() exported helper will not be fixed in the v4 to support the parameters passed to middleware. The helper is deprecated and it's replacement (auth().redirectToSignIn()) will be fixed in v5 version of the @clerk/nextjs package. The reason is that it requires a breaking change to be fixed, and we can't introduce that in the v4 version.

nbkhope commented 4 months ago

I've been trying to pass publishableKey to the Express middleware ("@clerk/clerk-sdk-node": "^5.0.0",):


createClerkExpressWithAuth({
  clerkClient: createClerkClient({
    secretKey: 'sk'
  }),
  publishableKey: 'pk',
})

but it always appears as empty string when I debug the source code from Clerk at https://github.com/clerk/javascript/blob/83ec173b08bdf18fda805e0d68e0034dbae0eb24/packages/sdk-node/src/clerkExpressWithAuth.ts#L5-L6

Despite it reading like it would take it as argument.

I don't know what is going on, but I keep getting the following error because of that:

my-app\node_modules\@clerk\shared\dist\keys.js:81
      throw new Error("Publishable key not valid.");
            ^

Error: Publishable key not valid.
kevinmitch14 commented 4 months ago

I think this has been fixed, #3001

dimkl commented 4 months ago

@nbkhope This issue related to the @clerk/nextjs package and not to the @clerk/clerk-sdk-node. Could you open a separate GH issue for your case and provide a minimal reproduction repo so we could verify that the issue is valid and investigate and assert the fix more quickly? @kevinmitch14 the https://github.com/clerk/javascript/pull/3001 fixes fixes the issue of respecting the publishableKey parameter passed in the middleware (eg clerkMiddleware) of @clerk/nextjs, it's to the @clerk/clerk-sdk-node.

cenobitedk commented 3 months ago

@nbkhope I've found a way to make it work.

With environment vars

import "dotenv/config"; // To read env vars

const clerkMiddleware = createClerkExpressWithAuth({
  clerkClient: createClerkClient({
    secretKey: process.env.SECRET_KEY,
    publishableKey: process.env.PUBLISHABLE_KEY,
  }),
});

app.get(
  "/protected",
  clerkMiddleware(),
  (req: WithAuthProp<Request>, res: Response) => {
    res.json(req.auth);
  }
);

Without environment vars

const clerkMiddleware = createClerkExpressWithAuth({
  clerkClient: createClerkClient({
    secretKey: "sk_test_somethingSecret",
    publishableKey: "pk_test_somethingPublishable",
  }),
});

app.get(
  "/protected",
  clerkMiddleware(),
  (req: WithAuthProp<Request>, res: Response) => {
    res.json(req.auth);
  }
);

Simpler version with env vars

// Fixed names, make sure these are not renamed.
CLERK_SECRET_KEY=sk_test_somethingSecret
CLERK_PUBLISHABLE_KEY=pk_test_somethingPublishable
import "dotenv/config";

app.get(
  "/protected",
  ClerkExpressWithAuth(),
  (req: WithAuthProp<Request>, res: Response) => {
    res.json(req.auth);
  }
); 
nbkhope commented 3 months ago

@cenobitedk hey thanks for the message. Instead of trying to pass the values with the constructor functions, I ended up letting it automatically detect it as Clerk usually presents in the examples. For that to work, inject my configuration manager values into the required variables into my process.env. Something like the following, before the lines that call ClerkExpressWithAuth:

process.env.CLERK_PUBLISHABLE_KEY = getValueFromSomewhereElseNotInEnvFile();
// ... (repeat for any other necessary variable)