supabase icon indicating copy to clipboard operation
supabase copied to clipboard

NextJS Middleware Auth Examples recommends using function that doesn't exist

Open ethanniser opened this issue 1 year ago • 6 comments

Improve documentation

Link

https://supabase.com/docs/guides/auth/server-side/nextjs?queryGroups=router&router=app

Describe the problem

The main issue is that the current example is structured in a way were it assumes the only thing you will ever do in your middleware is authenticate with supabase, but often I would think that is not the case.

My current project has some complex redirection logic, so I can't just simply return the supabaseResponse. The docs suggest making a new NextResponse and manually setting the cookies

const myNewResponse = NextResponse.next({ request })
myNewResponse.cookies.setAll(supabaseResponse.cookies.getAll())

The only problem is the cookies property on NextResponse is of type ResponseCookies which does not actually have a setAll method.

Describe the improvement

It would be fantastic if there was:

  • A recommended way to pass on the Supabase cookies to new responses
  • An example where the middleware does more than simply return await updateSession
  • Maybe an example repo to go off of and could be linted as to avoid recommending non existent or depreciated apis

I'm upgrading my auth from 0.6 @supabase/auth-helpers-nextjs and I was actually in the process of writing a separate issue earlier asking for the docs to no longer recommend get, set and delete, but #27242 literally merged as I was writing it (thanks @hf), but I've run into this separate issue now. If any other additional information is needed just let me know- happy to help.

Thanks so much

ethanniser avatar Jun 24 '24 19:06 ethanniser

Just to link things together I thought I'd share that there's also a Discussion I just came across that covers a couple other documentation issues on the same page (https://supabase.com/docs/guides/auth/server-side/nextjs?queryGroups=router&router=app).

Discussion: https://github.com/orgs/supabase/discussions/27619

To summarize quickly:

  • data: user should be data: { user} (otherwise if block is never executed)
  • The if statement needs to skip the login route or it'll create a redirect loop (e.g., if (!user && !request.nextUrl.pathname.startsWith("/login")))
  • NextResponse.redirect('/login') doesn't work in middleware (reference: https://nextjs.org/docs/messages/middleware-relative-urls)... I believe it should be:
const url = request.nextUrl.clone();
url.pathname = "/login";
return NextResponse.redirect(url);

EDIT: I've opened a PR for the issues from the Discussion, but it doesn't address this Issue. https://github.com/supabase/supabase/pull/27622.

ErikPetersenDev avatar Jun 28 '24 13:06 ErikPetersenDev

@ErikPetersenDev running in this issue, could you propose a solution for the setAll issue ? In the last updated doc, it still has the setAll call on the new response but it doesn't exist, like @ethanniser said

My previous middleware but using the new documentation, I loose the cookies

import { NextRequest } from 'next/server';
import { createI18nMiddleware } from 'next-international/middleware';

import { type CookieOptions, createServerClient } from '@supabase/ssr';

const handleI18nRouting = createI18nMiddleware({
  locales: ['en', 'fr'],
  defaultLocale: 'en',
  urlMappingStrategy: 'rewriteDefault',
});

export async function middleware(request: NextRequest) {
  const response = handleI18nRouting(request);

  const supabase = createServerClient(
    process.env.NEXT_PUBLIC_SUPABASE_URL!,
    process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!,
    {
      cookies: {
        get(name: string) {
          return request.cookies.get(name)?.value;
        },
        set(name: string, value: string, options: CookieOptions) {
          request.cookies.set({name, value, ...options});
          response.cookies.set({name, value, ...options});
        },
        remove(name: string, options: CookieOptions) {
          request.cookies.set({name, value: '', ...options});
          response.cookies.set({name, value: '', ...options});
        }
      }
    }
  );

  await supabase.auth.getUser();
  return response;
}

Thank you.

wadjeroudi avatar Jul 02 '24 05:07 wadjeroudi

Hi @wadjeroudi! I'm not part of the Supabase team and not very familiar with this particular issue, but I did just spend some time looking into it and might have a solution:

For the "setAll" issue, I think the right way to do it may be something like:

  const supabaseResponseCookies = supabaseResponse.cookies.getAll();
  supabaseResponseCookies.forEach((cookie) =>
    myNewResponse.cookies.set(cookie)
  );
// do things with myNewResponse
return myNewResponse

ErikPetersenDev avatar Jul 02 '24 13:07 ErikPetersenDev

The points outlined in this issue are fixed by the linked PR so I think this is safe to close. Massive thanks for doing this and keeping the docs up to date. If you think the cookie setting in the example is not correct currently, comment here and we can re-open.

encima avatar Jul 03 '24 06:07 encima

Hi @encima ! In my original comment above I was noting a different issue in the docs close to this one. I ended up fixing that issue and mentioned the PR in an edit (which then linked that PR to this Issue). I don't believe that addressed the original issue opened by @ethanniser though. Sorry if linking that caused a problem!

For example, the title of the issue is related to a comment in the example code that suggests using myNewResponse.cookies.setAll(supabaseResponse.cookies.getAll()) in a situation where setAll is not available (see below). And I think there might have been some other things this Issue was looking to address. Maybe worth reopening, but I'll leave it up to you!

  // IMPORTANT: You *must* return the supabaseResponse object as it is. If you're
  // creating a new response object with NextResponse.next() make sure to:
  // 1. Pass the request in it, like so:
  //    const myNewResponse = NextResponse.next({ request })
  // 2. Copy over the cookies, like so:
  //    myNewResponse.cookies.setAll(supabaseResponse.cookies.getAll()) <----- cookies.setAll can't be used there
  // 3. Change the myNewResponse object to fit your needs, but avoid changing
  //    the cookies!
  // 4. Finally:
  //    return myNewResponse
  // If this is not done, you may be causing the browser and server to go out
  // of sync and terminate the user's session prematurely!

  return supabaseResponse

ErikPetersenDev avatar Jul 03 '24 10:07 ErikPetersenDev

Thanks for clarifying, I will reopen and keep it open to address this!

encima avatar Jul 03 '24 10:07 encima

Hi @encima ! In my original comment above I was noting a different issue in the docs close to this one. I ended up fixing that issue and mentioned the PR in an edit (which then linked that PR to this Issue). I don't believe that addressed the original issue opened by @ethanniser though. Sorry if linking that caused a problem!

For example, the title of the issue is related to a comment in the example code that suggests using myNewResponse.cookies.setAll(supabaseResponse.cookies.getAll()) in a situation where setAll is not available (see below). And I think there might have been some other things this Issue was looking to address. Maybe worth reopening, but I'll leave it up to you!

  // IMPORTANT: You *must* return the supabaseResponse object as it is. If you're
  // creating a new response object with NextResponse.next() make sure to:
  // 1. Pass the request in it, like so:
  //    const myNewResponse = NextResponse.next({ request })
  // 2. Copy over the cookies, like so:
  //    myNewResponse.cookies.setAll(supabaseResponse.cookies.getAll()) <----- cookies.setAll can't be used there
  // 3. Change the myNewResponse object to fit your needs, but avoid changing
  //    the cookies!
  // 4. Finally:
  //    return myNewResponse
  // If this is not done, you may be causing the browser and server to go out
  // of sync and terminate the user's session prematurely!

  return supabaseResponse

Glad I found this issue, since I'm very confused now. The function updateSession now returns supabaseResponse, but what should be done with it, or is just calling it enough?

I've multiple middlewares in a NextJS project with responses so I had to create a chain in order to run through them. Which looks like:

middleware.ts

const middlewares = [withMiddleware1, withMiddleware2]
export default chain(middlewares)


// do not localize next.js paths
export const config = {
    matcher: ['/((?!api|_next/static|_next/image|images|assets|favicon.ico|sw.js).*)']
};

chain function:

export type CustomMiddleware = (
    request: NextRequest,
    event: NextFetchEvent,
    response: NextResponse
) => NextMiddlewareResult | Promise<NextMiddlewareResult>;

type MiddlewareFactory = (middleware: CustomMiddleware) => CustomMiddleware;

export function chain(functions: MiddlewareFactory[], index = 0): CustomMiddleware {
    const current = functions[index];

    if (current) {
        const next = chain(functions, index + 1);
        return current(next);
    }

    return (request: NextRequest, event: NextFetchEvent, response: NextResponse) => {
        return response;
    };
}

withMiddleware1.ts <-- localization

const i18nConfig = {
    locales: availableLocales,
    defaultLocale: defaultLocale
};

export function withMiddleware1(middleware: CustomMiddleware) {
    return async (request: NextRequest, event: NextFetchEvent) => {
        // The first middleware in the chain has to create the response
        // object and pass it down the chain.
        const response = i18nRouter(request, i18nConfig);

        // Call the next middleware and pass the request and response
        return middleware(request, event, response);
    };
}

withMiddleware2.ts <--- supabase auth

export function withMiddleware2(middleware: CustomMiddleware) {
    return async (request: NextRequest, event: NextFetchEvent, response: NextResponse) => {
       const updatedResonse = await updateSession(request); // <-- What to do with updatedResonse? 

        // Call the next middleware and pass the request and response
        return middleware(request, event, response);
    };
}

If I return the updatedResonse instead of response here: return middleware(request, event, updatedResonse);, I will lose my earlier responses (from withMiddleware1 in my case).

RowinVanAmsterdam avatar Aug 05 '24 19:08 RowinVanAmsterdam

@RowinVanAmsterdam I think you should have it work by passing the response to updateSession as a argument, and within updateSession check if response is undefined (so not created from a previous middleware) and in that case take the supabaseResponse as defined in their documentation.

nassmim avatar Sep 18 '24 17:09 nassmim

any news on this? I actually have the exact same issue as mentioned by @RowinVanAmsterdam. It's been two days I am working on this, could not find a way to make it work properly. I managed to get either the user session or the internationalisation but never both.

[EDIT] The middleware supabase is setup correctly. Whenever I remove the chained middlewares and I work only with the supabase one, everything is ok. But when I add back the middleware for handling internationalisation, getUser() fails. So it's really about how to pass the request/response correctly so that cookies are not lost in the way.

nassmim avatar Oct 02 '24 07:10 nassmim

Finally solved this, but not sure it is the recommended way. Instead of having supabase middleware run first, I put it has the last middleware to ensure the response is not modified after it runs. And within the previous middlewares I modify the "middleware-scope" request, which I pass to the next middleware. So far that makes the job, did not run into any issue.

nassmim avatar Oct 03 '24 16:10 nassmim

Finally solved this, but not sure it is the recommended way. Instead of having supabase middleware run first, I put it has the last middleware to ensure the response is not modified after it runs. And within the previous middlewares I modify the "middleware-scope" request, which I pass to the next middleware. So far that makes the job, did not run into any issue.

Sounds more like a workaround than a definitive fix. Especially if you have a similar middleware that also needs to be at the end. I have fixed it by creating a response object in the first and pass it down the chain, so maybe you can use it or adapt it to your project:

middleware.ts:

import { stackMiddlewares } from './utils/stackMiddlewares';
import { i18nMiddleware } from './shared/i18n/middleware/i18nMiddleware';
import { authenticationMiddleware } from './utils/Supabase/authenticationMiddleware';

/**
 * Middleware stack for the application
 * iterate through the imported middleware functions. The first middleware in the chain has to create the response object,
 * and pass it down the chain.
 */

const middlewares = [i18nMiddleware, authenticationMiddleware];
export default stackMiddlewares(middlewares);

// do not localize next.js paths
export const config = {
    matcher: ['/((?!api|_next/static|_next/image|images|assets|favicon.ico|sw.js).*)']
};

stackMiddlewares.ts:

import { NextMiddlewareResult } from 'next/dist/server/web/types';
import { NextResponse } from 'next/server';
import type { NextFetchEvent, NextRequest } from 'next/server';

type MiddlewareFactory = (middleware: NextMiddlewareExtended) => NextMiddlewareExtended;

export type NextMiddlewareExtended = (
    request: NextRequest,
    event: NextFetchEvent,
    response: NextResponse
) => NextMiddlewareResult | Promise<NextMiddlewareResult>;

export function stackMiddlewares(functions: MiddlewareFactory[], index = 0): NextMiddlewareExtended {
    const current = functions[index];

    if (current) {
        const next = stackMiddlewares(functions, index + 1);
        return current(next);
    }

    return (request: NextRequest, event: NextFetchEvent, response: NextResponse) => {
        return response;
    };
}

This is my first middleware, so it has to create a response object and returns it:

i18nMiddleware.ts

import { type NextFetchEvent, type NextRequest } from 'next/server';
import { i18nRouter } from 'next-i18n-router';
import { NextMiddlewareExtended } from '../../../utils/stackMiddlewares';
import { availableLocales, defaultLocale } from '@/shared/i18n/settings';

const i18nConfig = {
    locales: availableLocales,
    defaultLocale: defaultLocale
};

/**
 * Middleware that sets up the i18n router.
 * Creates a new response object, since it's the first middleware in the chain.
 */

export function i18nMiddleware(middleware: NextMiddlewareExtended) {
    return async (request: NextRequest, event: NextFetchEvent) => {
        const response = i18nRouter(request, i18nConfig);

        return middleware(request, event, response);
    };
}

Here it receives the response from the previous middlewares, modifies it and once again returns it so the next middleware in the chain has access to it.

authenticationMiddleware.ts:

import { NextResponse, type NextFetchEvent, type NextRequest } from 'next/server';
import { NextMiddlewareExtended } from '../stackMiddlewares';
import { createServerClient } from '@supabase/ssr';
import { appPaths } from '@/utils/appPaths';

export function authenticationMiddleware(middleware: NextMiddlewareExtended) {
    return async (request: NextRequest, event: NextFetchEvent, response: NextResponse) => {
        const isAuthPage = request.nextUrl.pathname.startsWith('/auth');
        let supabaseResponse = response;

        const supabase = createServerClient(process.env.NEXT_PUBLIC_SUPABASE_URL!, process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!, {
            cookies: {
                getAll() {
                    return request.cookies.getAll();
                },
                setAll(cookiesToSet) {
                    cookiesToSet.forEach(({ name, value, options }) => request.cookies.set(name, value));
                    supabaseResponse = NextResponse.next({
                        request
                    });
                    cookiesToSet.forEach(({ name, value, options }) => supabaseResponse.cookies.set(name, value, options));
                }
            }
        });

        const {
            data: { user }
        } = await supabase.auth.getUser();

        // Redirect to the profile page if the user is already authenticated and is trying to access an authentication page
        if (isAuthPage && user) {
            return NextResponse.redirect(new URL(appPaths.app.profile, request.url));
        }

        // IMPORTANT: You *must* return the supabaseResponse object as it is. If you're
        // creating a new response object with NextResponse.next() make sure to:
        // 1. Pass the request in it, like so:
        //    const myNewResponse = NextResponse.next({ request })
        // 2. Copy over the cookies, like so:
        //    myNewResponse.cookies.setAll(supabaseResponse.cookies.getAll())
        // 3. Change the myNewResponse object to fit your needs, but avoid changing
        //    the cookies!
        // 4. Finally:
        //    return myNewResponse
        // If this is not done, you may be causing the browser and server to go out
        // of sync and terminate the user's session prematurely!

        return middleware(request, event, supabaseResponse);
    };
}

I hope this helps a bit.

RowinVanAmsterdam avatar Oct 04 '24 11:10 RowinVanAmsterdam

Hello, I am also having the same problem. Will there be a fix for this bug soon?

(Code)B9w0g3_12-04-2024

Also, why do you market this product as an alternative to Firebase? If you still plan to do that, at the very least, please update your documentation for Next.js. It would save others (including me) from having to deal with this huge headache while using Supabase.

FlawlessCasual17 avatar Dec 05 '24 05:12 FlawlessCasual17

UPDATE: I think I have a solution for my headache:

This is what should have been put in the documentation

import { createServerClient } from '@supabase/ssr'
import { cookies } from 'next/headers'

export const createClient = (cookieStore: ReturnType<typeof cookies>) => createServerClient(
    process.env.NEXT_PUBLIC_SUPABASE_URL!,
    process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!, {
    cookies: {
        async getAll() { return (await cookieStore)?.getAll() },
        setAll(cookiesToSet) {
            try {
                cookiesToSet.forEach(async ({ name, value, options }) =>
                    (await cookieStore)?.set(name, value, options))
            } catch {
                // The `setAll` method was called from a Server Component.
                // This can be ignored if you have middleware refreshing
                // user sessions.
            }
        }
    }
})

Other solution (async arrow function)

import { createServerClient } from '@supabase/ssr'
import { cookies } from 'next/headers'

export const createClient = async (cookieStore: ReturnType<typeof cookies>) => createServerClient(
    process.env.NEXT_PUBLIC_SUPABASE_URL!,
    process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!, {
    cookies: {
        async getAll() { return (await cookieStore)?.getAll() },
        setAll(cookiesToSet) {
            try {
                cookiesToSet.forEach(async ({ name, value, options }) =>
                    (await cookieStore)?.set(name, value, options))
            } catch {
                // The `setAll` method was called from a Server Component.
                // This can be ignored if you have middleware refreshing
                // user sessions.
            }
        }
    }
})

FlawlessCasual17 avatar Dec 05 '24 18:12 FlawlessCasual17

@RowinVanAmsterdam I love the pattern you suggested 🎉 It feels nice and clean.

I had to slightly modify this pattern in order to get next-intl to work. Without copying over the headers, routes will 404's for me when the token is being reset

import { createServerClient } from "@supabase/ssr";
import { NextResponse, type NextFetchEvent, type NextRequest } from "next/server";
import { NextMiddlewareExtended } from "./stack-middlewares";

export function authenticationMiddleware(middleware: NextMiddlewareExtended) {
  return async (request: NextRequest, event: NextFetchEvent, response: NextResponse) => {
    let supabaseResponse = response;

    const supabase = createServerClient(
      process.env.NEXT_PUBLIC_SUPABASE_URL!,
      process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!,
      {
        cookies: {
          getAll() {
            return request.cookies.getAll();
          },
          setAll(cookiesToSet) {
            // Preserve existing response and its headers
            const existingHeaders = new Headers(supabaseResponse.headers);

            cookiesToSet.forEach(({ name, value, options }) => request.cookies.set(name, value));
            supabaseResponse = NextResponse.next({
              request,
            });

            // Copy over all headers from the previous response
            existingHeaders.forEach((value, key) => {
              supabaseResponse.headers.set(key, value);
            });

            // Now set the new cookies
            cookiesToSet.forEach(({ name, value, options }) => supabaseResponse.cookies.set(name, value, options));
          },
        },
      }
    );

    await supabase.auth.getUser();

    // IMPORTANT: You *must* return the supabaseResponse object as it is. If you're
    // creating a new response object with NextResponse.next() make sure to:
    // 1. Pass the request in it, like so:
    //    const myNewResponse = NextResponse.next({ request })
    // 2. Copy over the cookies, like so:
    //    myNewResponse.cookies.setAll(supabaseResponse.cookies.getAll())
    // 3. Change the myNewResponse object to fit your needs, but avoid changing
    //    the cookies!
    // 4. Finally:
    //    return myNewResponse
    // If this is not done, you may be causing the browser and server to go out
    // of sync and terminate the user's session prematurely!

    return middleware(request, event, supabaseResponse);
  };
}

~~This is working for me locally, but not on Vercel 🤔 I'm trying to figure out what I'm messing up here~~ Had a cookie name conflict, now it's working 🎉

matthova avatar Feb 17 '25 00:02 matthova

@RowinVanAmsterdam just moved to your approach today to have something cleaner. it works perfectly, thanks!

nassmim avatar Mar 13 '25 22:03 nassmim

Hi everyone, due to inactivity on this issue I've moved the issue over to discussions/enhancements. Thank you for your help on this one!

Hallidayo avatar Apr 08 '25 21:04 Hallidayo