Skip to content
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

Paths with a "." are ignored by authMiddleware() #1656

Open
6 of 21 tasks
IGassmann opened this issue Aug 30, 2023 · 10 comments
Open
6 of 21 tasks

Paths with a "." are ignored by authMiddleware() #1656

IGassmann opened this issue Aug 30, 2023 · 10 comments
Labels
bug Something isn't working nextjs prioritized This issue has been triaged and the team is working on it

Comments

@IGassmann
Copy link
Contributor

IGassmann commented Aug 30, 2023

  • I have reviewed the documentation
  • I have searched for existing issues
  • I have ensured I am on the most recent version, and checked the changelog for that version
  • I have provided the Frontend API key from my application dashboard: pk_test_dGVuZGVyLXF1YWdnYS0zMC5jbGVyay5hY2NvdW50cy5kZXYk
  • I have provided a minimal reproduction or replay recording below

Brief Summary of the Issue

Next.js routes that include a . in their path are ignored by the authMiddleware(), even though . is a valid character to have within a URL path:

// `encodeURIComponent()` encodes characters such as ?, =, /, &, :
console.log(`paths/with/${encodeURIComponent('a.dot')}/are/okay`);
// Expected output: "paths/with/a.dot/are/okay"

This makes those pages error in Next.js if they use auth() in one of their Server Components.

Potential Solution

The Clerk's default proposed middleware config matcher doesn't match URL paths that include a . in their path.

Editing the matcher to allow . doesn't solve the issue, because the authMiddleware() also ignores those URL paths by default due to its DEFAULT_IGNORED_ROUTES.

Both the DEFAULT_CONFIG_MATCHER and the DEFAULT_IGNORED_ROUTES need to be updated in the authMiddleware() code and in the docs to allow for . in URL paths.

Minimal Reproduction or Replay

  • git clone https://github.com/IGassmann/dot-clerk-issue
  • npm install the required dependencies.
  • npm run dev to launch the development server.
  • Open http://localhost:3000
  • Click in "View Demo"
  • This will open http://localhost:3000/dot.path/dashboard which will throw:
Error: Clerk: auth() was called but it looks like you aren't using `authMiddleware` in your middleware file. Please use `authMiddleware` and make sure your middleware matcher is configured correctly and it matches this route or page. See https://clerk.com/docs/quickstarts/get-started-with-nextjs

Package + Version

  • @clerk/clerk-js
  • @clerk/clerk-react
  • @clerk/nextjs
  • @clerk/remix
  • @clerk/types
  • @clerk/themes
  • @clerk/localizations
  • @clerk/clerk-expo
  • @clerk/backend
  • @clerk/clerk-sdk-node
  • @clerk/shared
  • @clerk/fastify
  • @clerk/chrome-extension
  • gatsby-plugin-clerk
  • build/tooling/chore
  • other:
@jescalan jescalan added the needs-triage A ticket that needs to be triaged by a team member label Aug 31, 2023
@jescalan
Copy link
Contributor

Hey @IGassmann - just want to drop a little context here. The reason we ignore paths with a . currently is an attempt to avoid running auth on requests to assets like images, css files, etc, which have a wide range of extensions and all have a . in them. It would probably produce a number of issues if we tried to guess what actual extensions people use for files and got it wrong in any situation, so the . detection felt like casting a wider net, given that using . is relatively uncommon for non-asset paths.

However, what you have brought up here is entirely valid and we're going to think about how we can provide a solution for you here. Any ideas you have are of course welcome as well!

@BRKalow BRKalow added prioritized This issue has been triaged and the team is working on it and removed needs-triage A ticket that needs to be triaged by a team member labels Sep 7, 2023
@BRKalow
Copy link
Member

BRKalow commented Sep 26, 2023

Hey @IGassmann we have recently updated our default matcher and ignored routes patterns to be more tolerant of . in path segments. Can you update @clerk/nextjs, update your middleware matcher to match what we have in our documentation, and let us know if this addresses your issue? 🙏

@IGassmann
Copy link
Contributor Author

IGassmann commented Oct 2, 2023

Hey @BRKalow! Unfortunately, this doesn't solve our use case because we do have URLs in the following format:

  • /events/page.viewed
  • /events/user.signed.up

Our event names follow the dot case notation and can appear in the last segment of a URL. The Clerk's matcher would exclude those.

I wonder if this Next.js middleware config is enough to avoid running auth on requests to assets:

export const config = {
  matcher: [
    /*
     * Match all request paths except for the ones starting with:
     * - _next/static (static files)
     * - _next/image (image optimization files)
     * - favicon.ico (favicon file)
     */
    '/((?!_next/static|_next/image|favicon.ico).*)',
  ],
}

Next.js's static files (JS, CSS, fonts...) files that are statically imported in code are all served from _next/static, while images that are used with the <Image /> are all served from _next/image. The only assets that I know of that would be outside of those directories would be file assets placed in the public/ directory that aren't being statically imported in code:

  • public/robots.txt
  • public/sitemap.xml
  • public/logo.png with <img src="/logo.png" /> over import logo from '../public/logo.png'; <Image src={logo} />
  • ...

@LekoArts LekoArts added bug Something isn't working nextjs labels Oct 4, 2023
@LekoArts
Copy link
Member

LekoArts commented Oct 4, 2023

Thanks for providing that additional information 👍

We currently have these tests: https://github.com/clerkinc/javascript/blob/164f3aac7928bc69301846130cc77986569d4e91/packages/nextjs/src/server/authMiddleware.test.ts#L134-L147

When I add your usage example to them, they fail:

✕ does not match /protected/hello.example
✕ does not match /protected/hello.world.example
✕ does not match /protected/hello.world.example.here

So we have more fine-tuning here to do.

Your suggestion for the matcher is the same as https://nextjs.org/docs/pages/building-your-application/routing/middleware#matcher - we should try if we can make it simpler like that 👍

@codyjk
Copy link

codyjk commented Nov 4, 2023

I also ran into this issue, and after going down a rabbit hole through the source code I figured out that it was due to .s being in the path, and ultimately ended up here.

In the app I'm building, I have pages for domains in the format /domains/:domain - e.g. /domains/example.com. As a workaround, I'm instead modeling the URL as /domains/:domain/page (e.g. /domains/example.com/page) but I'd prefer not to have to do that.

Is this being worked on? Any expectation on when this will be supported?

@BRKalow
Copy link
Member

BRKalow commented Nov 6, 2023

Hi @codyjk! This can be worked around by updating your middleware matcher and passing a custom ignoredRoutes option to authMiddleware(). The default ignoredRoutes value is:

ignoredRoutes: [`/((?!api|trpc))(_next.*|.+\\.[\\w]+$)`]

We do a best-effort match here, but as you have seen it doesn't handle all edge cases. Feel free to tweak the middleware matcher and ignoredRoutes pattern to handle your use case. 🙏

@codyjk
Copy link

codyjk commented Nov 11, 2023

Hi @codyjk! This can be worked around by updating your middleware matcher and passing a custom ignoredRoutes option to authMiddleware(). The default ignoredRoutes value is:

ignoredRoutes: [`/((?!api|trpc))(_next.*|.+\\.[\\w]+$)`]

We do a best-effort match here, but as you have seen it doesn't handle all edge cases. Feel free to tweak the middleware matcher and ignoredRoutes pattern to handle your use case. 🙏

Thanks for the suggestion - unless I'm missing something, any path that matches ignoredRoutes won't be able to use auth(), correct? Unfortunately that won't work for my use case.

Are there any plans to formally support paths in the form /path/containing.periods?

@BRKalow
Copy link
Member

BRKalow commented Nov 13, 2023

@codyjk Correct, but you can provide your own ignoredRoutes matchers to support your specific path format:

ignoredRoutes: [`/((?!api|trpc|domains))(_next.*|.+\\.[\\w]+$)`]

And updating the middleware matcher:

export const config = {
  matcher: ['/((?!.+\\.[\\w]+$|_next).*)', '/', '/(api|trpc|domains)(.*)'],
};

This would prevent any routes starting with /domains from being ignored, allowing you to use auth() in those pages.

@codyjk
Copy link

codyjk commented Nov 20, 2023

@BRKalow - makes sense, that did the trick. Thanks for the help!

@reginaldl
Copy link

Hi,

Just got caught by the same issue. I believe this could potentially lead to security issues where dev modifies matcher to include routes with . and these routes are not properly redirected for sign-in.
I understand we'd preferably ignore static assets here. Though, the current behavior is very implicit and confusing right now because from a dev perspective when ignoredRoutes is not set, we assume that matcher would properly include set routes, but it doesn't.
Could DEFAULT_IGNORED_ROUTES be empty instead and let devs decide what to match in matcher?
At least, updating the Options table with default values in the documentation would be super useful.

I have to say Next.js doesn't make it easy to filter public assets in the middleware. They seem to ignore public assets in their doc:

  matcher: [
    /*
     * Match all request paths except for the ones starting with:
     * - api (API routes)
     * - _next/static (static files)
     * - _next/image (image optimization files)
     * - favicon.ico (favicon file)
     */
    '/((?!api|_next/static|_next/image|favicon.ico).*)',
  ],
}```

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working nextjs prioritized This issue has been triaged and the team is working on it
Projects
None yet
Development

No branches or pull requests

6 participants