fix(middleware): use includes() for NextAuth pages

[Juneezee]

☕️ Reasoning

Some users could be setting their signIn and error pages option to
/ to disable the automatically generated pages, as suggested in #2330 (reply in thread).

This PR reverts the behaviour for matching signIn and error
pages in handleMiddleware to pre-v4.10.3.

EDIT:

const signInPage = "/"
const errorPage = "/"
const publicPaths = [signInPage, errorPage, "/_next", "/favicon.ico"]

// pathname = "/protected" will always return true
publicPaths.some((p) => pathname.startsWith(p))

Fixes: aedabc8 ("fix: avoid redirect on always public paths")

🧢 Checklist

• Documentation
• Tests
• Ready to be merged

🎫 Affected issues

📌 Resources

• Contributing guidelines
• Code of conduct
• Contributing to Open Source

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Updated
next-auth	⬜️ Ignored (Inspect)		Aug 4, 2022 at 0:26AM (UTC)

[ThangHuuVu]

Hi 👋 So in #5000 we made that change to avoid the infinite re-direct loop when you have a custom error page that is also require authentication. I wasn't aware that setting / to all custom pages is a way to disable all built in page. 🤔 Seems like there is a conflict here. cc @balazsorban44

We would be appreciated if you could open a new issue so that we can investigate and think of a solution together there instead 🙌 I'm going to close this one in the mean time.
Your PR doesn't seem to change the output at all, BTW 🤔:

const signInPage = "/"
const errorPage = "/"
const publicPaths = [signInPage, errorPage, "/_next", "/favicon.ico"]

const pathname = "/"
// current
publicPaths.some((p) => pathname.startsWith(p)) // return true

// your change
[signInPage, errorPage].includes(pathname) // also return true

[balazsorban44]

I think this should be OK, but in that case, you have to ensure middleware is not invoked on /. 🤔 I don't think this needs a PR.

[Juneezee]

Your PR doesn't seem to change the output

@ThangHuuVu Sorry, I made a typo in the code snippet. It should be

const signInPage = "/"
const errorPage = "/"
const publicPaths = [signInPage, errorPage, "/_next", "/favicon.ico"]

// pathname = "/protected/pathA" will always return true
publicPaths.some((p) => pathname.startsWith(p))

So if the middleware has the pages option configured as

pages: {
  signIn: '/',
  error: '/',
}

and

export const config = { matcher: ["/protected/:path*"] }

Visiting /protected/pathA will not redirect to the signIn page.

[Juneezee]

I think this should be OK, but in that case, you have to ensure middleware is not invoked on /. 🤔 I don't think this needs a PR.

@balazsorban44 See #5085 (comment). Middleware is not invoked on /. It works fine on v4.10.2 with signIn and error set as /.

[ThangHuuVu]

Thanks for the explanation, this makes more sense to me now 👍
Would you be able to add a test so that we don't regress again in future changes? 🙏

[Juneezee]

@ThangHuuVu Not sure what went wrong with GitHub, it's not showing the new commit that I've pushed. Should I open another PR that adds the test case after this PR is merged?

[balazsorban44]

Might be because we closed it. It would be nice to have the test and the code in the same PR so we can verify it first. Maybe we should close this PR and just create a new one. Sorry 💚