clerk / javascript

Official Javascript repository for Clerk authentication
https://clerk.com
MIT License
959 stars 212 forks source link

Remix v2_errorBoundary makes sign in 401 error #965

Closed arjunyel closed 1 year ago

arjunyel commented 1 year ago

Package + Version

Dependencies + versions

{
  "dependencies": {
    "@clerk/remix": "^2.2.17",
    "@remix-run/node": "^1.14.3",
    "@remix-run/react": "^1.14.3",
    "@remix-run/vercel": "^1.14.3",
    "react": "^17.0.2",
    "react-dom": "^17.0.2"
  },
  "devDependencies": {
    "@remix-run/dev": "^1.14.3",
    "@remix-run/eslint-config": "^1.14.3",
    "@remix-run/serve": "^1.14.3",
    "@types/react": "^17.0.24",
    "@types/react-dom": "^17.0.9",
    "cross-env": "^7.0.3",
    "eslint": "^8.9.0",
    "prettier": "^2.5.1",
    "typescript": "^4.6.2"
  }
}

Browser/OS

Chrome 112

Description

https://github.com/arjunyel/remix-auth-starter/tree/bug/v2_errorBoundary

In remix.config.js, if v2_errorBoundary: true login results in a 401 error. v2_errorBoundary: false works fine.

v2_errorBoundary - Combine ErrorBoundary/CatchBoundary into a single ErrorBoundary https://remix.run/blog/future-flags

dimkl commented 1 year ago

Hello @arjunyel. Thank you for reporting this.

I have followed the next steps but i couldn't reproduce the error you are describing:

Could you provide more information about the 401 error? Example

arjunyel commented 1 year ago

Oh wow, I am getting no error in the console and there are no headers on the failed request. But it happens 100% of the time for me (and in my other project). Are you signing in with Google? I am.

Any chance we can do a quick screen share in Discord since I can reproduce it 100% of the time? I am @arjunyel in the discord

arjunyel commented 1 year ago

Regular email password login works, but Google sign in throws the 401

dimkl commented 1 year ago

x-clerk-auth-* response headers in the failed request with http 401 The reason the above is not visible is because you need to include those headers in response ( i forgot to mention it above 😅 ). To do so you can use the following snippet in root.ts file:


import { getClerkDebugHeaders } from '@clerk/remix/ssr.server';

export function headers({ actionHeaders, loaderHeaders, parentHeaders, }: { actionHeaders: Headers; loaderHeaders: Headers; parentHeaders: Headers; }) { return { ...getClerkDebugHeaders(loaderHeaders), }; }


I managed to reproduce your issue but making the Clerk short lived session expire before refreshing the page.
I will also try the google sign-in flow and come back to you with more information.
arjunyel commented 1 year ago

Headers are

x-clerk-auth-message: null
x-clerk-auth-reason: uat-missing
x-clerk-auth-status: interstitial

Someone in discord is also getting the uat missing error https://discord.com/channels/856971667393609759/1086202715077627905

dimkl commented 1 year ago

Short answer

Supporting v2_errorBoundary flag, would require to export an ClerkErrorBoundary that will handle both CatchBoundary and ErrorBoundary . We cannot make a quick fix at the moment because the implementation of such ClerkErrorBoundary would require to use useRouteError which is not supported by our minimum peer dependency of the @remix-run/react.

We have created an internal ticket for this and i will keep this issue open. Until we resolve this, i would suggest to avoid using the v2_errorBoundary flag. If it's a must have, i could provide a hack (but i would advise against it).

Extra information

There is special case in Clerk & SSR (eg when an expired session cookie is sent to server) where we do some magic to refresh the session and replay the request (we call it interstitial). In that case an http 401 is returned to the user (using the root CatchBoundary) with an html content.

In the case where v2_errorBoundary is set, the example code (stater template and your provided repo) is not correct since it uses the root level CatchBoundary instead of the the new ErrorBoundary which results in allowing the http 401 code with the interstitial magic to reach Browser and stuck there.

arjunyel commented 1 year ago

@dimkl thank you so much friend! I'll disable v2_errorBoundary for now

arjunyel commented 1 year ago

Would it be possible to increase the minimum peer dependency of the @remix-run/react? Would help getting prepared for Remix v2 which is coming :)

nicksrandall commented 1 year ago

@dimkl latest version of Remix (1.15) will likely be the last before v2 and it now warns when not using the v2_errorBoundary flag. This should probably be addressed soon or not clerk customers will be able to upgrade to Remix v2.

nicksrandall commented 1 year ago

FWIW, this should work as a temporary work around:

export function ErrorBoundary() {
  const error = useRouteError();
  if (isRouteErrorResponse(error)) {
    const { __clerk_ssr_interstitial_html } =
      error?.data?.clerkState?.__internal_clerk_state || {};
    if (__clerk_ssr_interstitial_html) {
      return (
        <html
          dangerouslySetInnerHTML={{ __html: __clerk_ssr_interstitial_html }}
        />
      );
    }
    return (
        //  Current CatchBoundary Component
    );
  }
  return (
    // Current ErrorBoundary Component
  );
}
ZeldOcarina commented 1 year ago

@nicksrandall do you happen to know what the default CatchBoundary and ErrorBoundary components are?

nicksrandall commented 1 year ago

Sorry, I don't know want you are referring to.

ZeldOcarina commented 1 year ago

image If used on root.tsx this would break the default catch-all component remix gives and just make the app crash. Do you happen to know how to manually put it in there? Otherwise I'll just add a small catch-all component and call it a day..

nicksrandall commented 1 year ago

Here you go: https://github.com/remix-run/remix/blob/main/packages/remix-react/errorBoundaries.tsx

ZeldOcarina commented 1 year ago

Thanks a lot! This refactored code seems to work in case anyone needs it:

import {
  RemixRootDefaultCatchBoundary,
  RemixRootDefaultErrorBoundary,
} from "@remix-run/react/dist/errorBoundaries";

export const ErrorBoundary = () => {
  const error = useRouteError();
  if (isRouteErrorResponse(error)) {
    const { __clerk_ssr_interstitial_html } = error?.data?.clerkState?.__internal_clerk_state || {};
    if (__clerk_ssr_interstitial_html) {
      return <html dangerouslySetInnerHTML={{ __html: __clerk_ssr_interstitial_html }} />;
    }
    //  Current CatchBoundary Component
    return <RemixRootDefaultCatchBoundary />;
  } else if (error instanceof Error) {
    return <RemixRootDefaultErrorBoundary error={error} />;
  } else {
    let errorString =
      error == null
        ? "Unknown Error"
        : typeof error === "object" && "toString" in error
        ? error.toString()
        : JSON.stringify(error);
    return <RemixRootDefaultErrorBoundary error={new Error(errorString)} />;
  }
};
xixixao commented 1 year ago

I can confirm that the current Clerk tutorial for Remix isn't working with current version of Remix, 1.15, and that adding the code above to root.jsx makes the app run again.

vedovelli commented 1 year ago

Hello 👋

As Remix V2 is close to be released, does Clerk have an estimate on when an update can be expected? I am specifically asking about v2_errorBoundary.

Thank you!

ZeldOcarina commented 1 year ago

I agree with @vedovelli it's getting increasingly urgent..

dimkl commented 1 year ago

hello 👋 We will try to prioritise the changes to support v2 in the next couple of weeks. In the meantime @clerk/remix is an open source package, if someone wants to contribute, feel free to open a PR and we will try to review it as soon as possible.

suryarajendhran commented 1 year ago

In the meantime can someone please add a note to the docs so others like me don't spend 2 days trying to figure out why Clerk is not working with their shiny new Remix app.

perkinsjr commented 1 year ago

Hey everyone,

I have added this to your getting started guide so people know to disable it, thanks for the feedback!

ZeldOcarina commented 1 year ago

Ok I am using extensively the configuration posted above @perkinsjr. Would you think it's worth adding in the docs?

import {
  RemixRootDefaultCatchBoundary,
  RemixRootDefaultErrorBoundary,
} from "@remix-run/react/dist/errorBoundaries";

export const ErrorBoundary = () => {
  const error = useRouteError();
  if (isRouteErrorResponse(error)) {
    const { __clerk_ssr_interstitial_html } = error?.data?.clerkState?.__internal_clerk_state || {};
    if (__clerk_ssr_interstitial_html) {
      return <html dangerouslySetInnerHTML={{ __html: __clerk_ssr_interstitial_html }} />;
    }
    //  Current CatchBoundary Component
    return <RemixRootDefaultCatchBoundary />;
  } else if (error instanceof Error) {
    return <RemixRootDefaultErrorBoundary error={error} />;
  } else {
    let errorString =
      error == null
        ? "Unknown Error"
        : typeof error === "object" && "toString" in error
        ? error.toString()
        : JSON.stringify(error);
    return <RemixRootDefaultErrorBoundary error={new Error(errorString)} />;
  }
};
perkinsjr commented 1 year ago

I'd prefer to keep the configuration to disable it while we work on the improvements for V2.

We can discuss internally, if we want to put this in depending on the amount of time it will take us to add it to our SDK support.

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.

nicksrandall commented 1 year ago

Bump

anagstef commented 1 year ago

Hello everyone! We just released @clerk/remix@2.7.0 which exports a V2_ClerkErrorBoundary (#1444) to be used by projects that have the v2_errorBoundary future flag set to true. The following code should replace the legacy CatchBoundary in root.tsx file:

export const ErrorBoundary = V2_ClerkErrorBoundary();

I'll be closing this issue as it's now resolved. Feel free to open another one if something comes up. Thank you!

clerk-cookie commented 4 days ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.