S
SolidJS•5mo ago
ChrisThornham

Help Me Understand "use server" Security

I'm new to this topic. So please excuse me if my questions if they're obvious or don't sound well thought out. I'm simply trying to learn. Let's assume I have an app dashboard. When a user visits that dashboard, I run a createEffect() to check their access status. Inside that "client-side" createEffect() I call a server function getUserRecord() Here's a simplified example to show logic. 
  // Get the userId and email of logged in user
  const supabaseAuthContext = useSupabaseAuthContext()!;
  const userId = supabaseAuthContext.supabaseSession?.user.id;
  const userEmail = supabaseAuthContext.supabaseSession?.user.email;

  createEffect(async () => {
    if (userId && userEmail) {
      const userRecord = await getUserRecord(userId);      
      if (userRecord) {
        if (userRecord.has_app_access) {
          // let them access the app
        } else {
          // Navigate away
        }
      }
    }
  });
  // Get the userId and email of logged in user
  const supabaseAuthContext = useSupabaseAuthContext()!;
  const userId = supabaseAuthContext.supabaseSession?.user.id;
  const userEmail = supabaseAuthContext.supabaseSession?.user.email;

  createEffect(async () => {
    if (userId && userEmail) {
      const userRecord = await getUserRecord(userId);      
      if (userRecord) {
        if (userRecord.has_app_access) {
          // let them access the app
        } else {
          // Navigate away
        }
      }
    }
  });
Here's the getUserRecord() function: 
export async function getUserRecord(userId: string) {
  "use server";
  // Get Supabase Admin Client
  const supabaseAdmin = getSupabaseAdminClient();

  if (supabaseAdmin) {
    // Query supabase
    const { data, error } = await supabaseAdmin
      .from("users")
      .select()
      .eq("auth_user_id", userId);

    if (error) {
      console.log(error.message);
    }
    return data;
  }
}
export async function getUserRecord(userId: string) {
  "use server";
  // Get Supabase Admin Client
  const supabaseAdmin = getSupabaseAdminClient();

  if (supabaseAdmin) {
    // Query supabase
    const { data, error } = await supabaseAdmin
      .from("users")
      .select()
      .eq("auth_user_id", userId);

    if (error) {
      console.log(error.message);
    }
    return data;
  }
}
For context, here's the getSupabaseAdminClient() function: 
export function getSupabaseAdminClient() {
  "use server";

  // Get the environment variables
  const projectURL = process.env.SUPABASE_PROJECT_URL;
  const supabaseServiceRole = process.env.SUPABASE_SERVICE_ROLE;
  if (!projectURL || !supabaseServiceRole) {
    // Log error
    console.error("Unable to initialize Supabase Auth Admin Client");
    return;
  }

  return createClient(projectURL, supabaseServiceRole, {
    auth: {
      autoRefreshToken: false,
      persistSession: false,
    },
  });
}
export function getSupabaseAdminClient() {
  "use server";

  // Get the environment variables
  const projectURL = process.env.SUPABASE_PROJECT_URL;
  const supabaseServiceRole = process.env.SUPABASE_SERVICE_ROLE;
  if (!projectURL || !supabaseServiceRole) {
    // Log error
    console.error("Unable to initialize Supabase Auth Admin Client");
    return;
  }

  return createClient(projectURL, supabaseServiceRole, {
    auth: {
      autoRefreshToken: false,
      persistSession: false,
    },
  });
}
31 Replies
ChrisThornham
ChrisThornhamOP•5mo ago
Here's the question: Technically, I'm calling the getUserRecord() function from the client... which leads me to believe that the getUserRecord() function is exposed to the client. Therefore, I'm wondering if it is possible for someone to repeatedly call getUserRecord() from the client? Can they open Chrome dev tools or hack into something to gain access to getUserRecord() and run it from the client with different variables? This might not be a huge concern when I'm reading data, but this could quickly become an issue if exposed a server function that wrote data to my database. Am I thinking about this correctly? If so, how do I securely call server function from the client?
Brendonovich
Brendonovich•5mo ago
Therefore, is it possible for someone to repeatedly call getUserRecord() from the client
Yes, any use server function can be called from the client
Can they open Chrome dev tools or hack into something to gain access to getUserRecord() and run it from the client with different variables?
Yes, they'll be able to supply their own userId value
If so, how do I securely call server function from the client?
Get an auth token into the server function and authenticate the user in there. Whether you do that with cookies or by passing it as an argument is up to you
ChrisThornham
ChrisThornhamOP•5mo ago
Ok... if I pass an auth token to my server function, couldn't any hacker just sign up for an account and pass that check?
Brendonovich
Brendonovich•5mo ago
For the case of getUserRecord i assume you'd want it to get the data of the current user, so use the auth token to figure out the user id to use instead of passing it as an argument
ChrisThornham
ChrisThornhamOP•5mo ago
Ok that makes sense. What about this: Take my original logic one step further: 
  createEffect(async () => {
    // Check to see if a user exists and whether or not they have access
    if (userId && userEmail) {
      // Query supabase for a user record.
      const userRecord = await getUserRecord(userId);      

      if (userRecord) {
        if (userRecord.length > 0) {
          // User record exist. Set access privileges
          setHasPaymentAccess(userRecord[0].has_payment_access);
          setHasSubscriptionAccess(userRecord[0].has_subscription_access);
        } else {
          // No user exists. Create a new user record.
          await createUserRecord(userId, userEmail);
        }
      }
    }
  }}:
  createEffect(async () => {
    // Check to see if a user exists and whether or not they have access
    if (userId && userEmail) {
      // Query supabase for a user record.
      const userRecord = await getUserRecord(userId);      

      if (userRecord) {
        if (userRecord.length > 0) {
          // User record exist. Set access privileges
          setHasPaymentAccess(userRecord[0].has_payment_access);
          setHasSubscriptionAccess(userRecord[0].has_subscription_access);
        } else {
          // No user exists. Create a new user record.
          await createUserRecord(userId, userEmail);
        }
      }
    }
  }}:
Now I'm exposing createUserRecord() to the client. 
export async function createUserRecord(userId: string, email: string) {
  "use server";
  // Create a user Data Object
  const userData = {
    auth_user_id: userId,
    customer_email: email,
  };

  // Get Supabase Admin Client
  const supabaseAdmin = getSupabaseAdminClient();
  if (supabaseAdmin) {
    // Add User Record
    const { error } = await supabaseAdmin.from("users").insert(userData);
    if (error) {
      throw new Error(`Failed to insert user record: ${error.message}`);
    }
  }
}
export async function createUserRecord(userId: string, email: string) {
  "use server";
  // Create a user Data Object
  const userData = {
    auth_user_id: userId,
    customer_email: email,
  };

  // Get Supabase Admin Client
  const supabaseAdmin = getSupabaseAdminClient();
  if (supabaseAdmin) {
    // Add User Record
    const { error } = await supabaseAdmin.from("users").insert(userData);
    if (error) {
      throw new Error(`Failed to insert user record: ${error.message}`);
    }
  }
}
If this function is exposed to the client, what prevents a hacker from continually writing to my DB? Would I add a "unique" clause to my user table to prevent multiple rows with the same userId?
Brendonovich
Brendonovich•5mo ago
You'd probably want to make emails unique and personally i'd generate user IDs on the server instead of client Since signups are a public thing you'll probably want rate limiting at some point And you'll probably want an email verification step at some point userId should also be unique though yeah
ChrisThornham
ChrisThornhamOP•5mo ago
Boy... apps get complicated the further you go, don't they? You think you have something all figured out... then you start thinking and realize there's another hole somewhere. I've got some homework to do. Last question... Let's say I'm using a cache() and load() function from the router like this: 
// CACHE ============================================================
const getOrders = cache(async (): Promise<Order[] | undefined> => {
  "use server";
  try {
    // Get Supabase Admin Client
    const supabase = getSupabaseAdminClient();
    if (supabase) {
      const { data, error } = await supabase.from("orders").select();

      if (error) {
        console.log("Error fetching orders:", error.message);
        return undefined;
      }

      if (!data) {
        console.log("No orders found");
        return undefined;
      }
      return data as Order[];
    }
  } catch (err) {
    console.log("Unexpected error fetching orders:", err);
    return undefined;
  }
}, "orders");

// LOADER ===========================================================
export const route = {
  load() {
    void getOrders();
  },
};

export default function OrdersPage() {
  const orders = createAsync(() => getOrders());
  return (
    <>
      <AdminDashboardLayout>
        <h4>Orders</h4>
        <hr class="my-10" />
        <Show when={orders()} fallback="No orders found.">
          {(orders) => <OrdersTable orders={orders} />}
        </Show>
      </AdminDashboardLayout>
    </>
  );
}
// CACHE ============================================================
const getOrders = cache(async (): Promise<Order[] | undefined> => {
  "use server";
  try {
    // Get Supabase Admin Client
    const supabase = getSupabaseAdminClient();
    if (supabase) {
      const { data, error } = await supabase.from("orders").select();

      if (error) {
        console.log("Error fetching orders:", error.message);
        return undefined;
      }

      if (!data) {
        console.log("No orders found");
        return undefined;
      }
      return data as Order[];
    }
  } catch (err) {
    console.log("Unexpected error fetching orders:", err);
    return undefined;
  }
}, "orders");

// LOADER ===========================================================
export const route = {
  load() {
    void getOrders();
  },
};

export default function OrdersPage() {
  const orders = createAsync(() => getOrders());
  return (
    <>
      <AdminDashboardLayout>
        <h4>Orders</h4>
        <hr class="my-10" />
        <Show when={orders()} fallback="No orders found.">
          {(orders) => <OrdersTable orders={orders} />}
        </Show>
      </AdminDashboardLayout>
    </>
  );
}
Is getOrders() exposed to the client, even if it's driven by the solid router? I think it is, right?
Brendonovich
Brendonovich•5mo ago
Yes since there's a use server there Solid router has basically no knowledge of server functions, it'll just use what you give it
ChrisThornham
ChrisThornhamOP•5mo ago
So a hacker get simply call getOrders() can get all of the orders?
Brendonovich
Brendonovich•5mo ago
Yeah
ChrisThornham
ChrisThornhamOP•5mo ago
So how do people get order securely?
Brendonovich
Brendonovich•5mo ago
Auth token
ChrisThornham
ChrisThornhamOP•5mo ago
Can you point me in a direction? Where can I read about this?
Brendonovich
Brendonovich•5mo ago
I assume supabase would have some examples, but the basic premise is that you get the auth token into the function, whether with cookies or as an argument or otherwise, and then make sure the auth token belongs to someone who is an admin Whether that's by fetching the user info from your database or validatin the auth token itself depends on the implementation
ChrisThornham
ChrisThornhamOP•5mo ago
Ok... That's a great start. Thank you for all of the help with this. I see a full day or two ahead of me. Have a great night.
Brendonovich
Brendonovich•5mo ago
Fwiw i think you're doing this the hard way - supabase can already manage database access for you with its auth Using the admin sdk is way more effort since you have to secure everything yourself
ChrisThornham
ChrisThornhamOP•5mo ago
You mean with Row Level Security?
Brendonovich
Brendonovich•5mo ago
Yeah
ChrisThornham
ChrisThornhamOP•5mo ago
Ok... I'm definitley new to this... so I'm open to any suggestions. I had a model like this working earlier. Maybe I'll revisit that logic tomorrow and see if I can improve my RLS policies.
Brendonovich
Brendonovich•5mo ago
The only tricky part is that you'd need to create the supabase client differently on the client and server Since on the client it'll automatically have access to cookies and stuff but on the server you need to provide them yourself
ChrisThornham
ChrisThornhamOP•5mo ago
That part I'm pretty comfortable with. I ran into some issues with the policies earlier, so I thought, "just run it on the server... that's secure"... but then I kept thinking and realized that might not be as secure as I thought. I'll take a fresh look at this tomorrow. If I can improve my policies, your approach will be better and easier. Thanks again! Ok. I've spent way too much time on this, but I learned a lot. I could grant access using RLS policies, but keeping access logic on the server makes more sense to me. This pattern also appears to have benefits, and I think it is very secure unless I'm missing something. Here's what I did. 1. Turn on RLS for all tables. 2. Don't create any policies. This locks client-side code from interacting with the DB altogether. 3. Use the server to control CRUD operations and access. Here's how I did it. Unlike Next, which is server first, SolidStart is client first. So instead of using the @supabase/ssr package to monitor Auth State, I used a regular Context Provider to monitor Auth State on the client. Supabase has an onAuthStateChange() function that monitors changes in Auth State. onAuthStateChange() provides an event and session when it detects changes in Auth state. I use that session to 1. Update the provider (so the client has access to the session) 2. Set a cookie (so the server has access to the session) --- Here's a simplified example: 
// Create the onAuthStateChange listener
const { data: { subscription }, } = supabase.auth.onAuthStateChange((_event, session) => {
  if (!!session) {
    // A session exists. Update the auth store
    setAuthStore({ isAuthenticated: true, supabaseSession: session });

    // Set a cookie for server-side code
    setAuthCookie(session);
  } else {
    // No session exists. Update the auth store
    setAuthStore({ isAuthenticated: false, supabaseSession: null });

    // Delete the cookie used for server-side code
    deleteAuthCookie();
  }
});

// Cookie Functions
function setAuthCookie(session: AuthSession) {
  "use server";
  setCookie("sb-session-token", session.access_token, {
    httpOnly: true,
    secure: true,
    sameSite: "strict",
  });
}

function deleteAuthCookie() {
  "use server";
  deleteCookie("sb-session-token");
}
// Create the onAuthStateChange listener
const { data: { subscription }, } = supabase.auth.onAuthStateChange((_event, session) => {
  if (!!session) {
    // A session exists. Update the auth store
    setAuthStore({ isAuthenticated: true, supabaseSession: session });

    // Set a cookie for server-side code
    setAuthCookie(session);
  } else {
    // No session exists. Update the auth store
    setAuthStore({ isAuthenticated: false, supabaseSession: null });

    // Delete the cookie used for server-side code
    deleteAuthCookie();
  }
});

// Cookie Functions
function setAuthCookie(session: AuthSession) {
  "use server";
  setCookie("sb-session-token", session.access_token, {
    httpOnly: true,
    secure: true,
    sameSite: "strict",
  });
}

function deleteAuthCookie() {
  "use server";
  deleteCookie("sb-session-token");
}
Then I can easily access the user on the server like this: 
"use server";
const sessionToken = getCookie("sb-session-token");
const { data } = await supabase.auth.getUser(sessionToken);
"use server";
const sessionToken = getCookie("sb-session-token");
const { data } = await supabase.auth.getUser(sessionToken);
I think this approach is beneficial for the following reasons. 1. It's immediately apparent what's happening when reading the code base. I don't have switch from server functions to RLS policies to figure out what's going on. 2. I don't have to worry about RLS policies changing. If an RLS policy changes, my app could stop working if the server or client code doesn't align with the new RLS policy. 3. At my current level of understanding, this seems very secure. The odds of a hacker guessing an access token are next to impossible. 4. I can use rate limiting and unique constraints to handle most other concerns. 5. Admin roles are also pretty simple to manage. I can create a table of admin users and check if the current user is in the table. Again a hacker would have to guess an access token to get past this check, which is next to impossible. Does this seem logical and secure?
Brendonovich
Brendonovich•5mo ago
Is your app using ssr?
ChrisThornham
ChrisThornhamOP•5mo ago
Yes, my app is using SSR
Brendonovich
Brendonovich•5mo ago
Not sure i agree with the characterisation that start is client-first but it's certainly a bit easier to do client stuff. Regardless, this seems like a fine approach
ChrisThornham
ChrisThornhamOP•5mo ago
Good point. I might not have the terminology correct. I guess what I'm trying to say is in Next, you have to opt into "use client." By default, all components are server components. But SolidStart doesn't have "server components." Everything is a "client component" that can call functions that live on the server.
Brendonovich
Brendonovich•5mo ago
yeah that's good enough, i could be more pedantic but it's not worth it haha though start will be getting islands eventually
ChrisThornham
ChrisThornhamOP•5mo ago
I don't mind pedantic, haha. I'm learning so, I've been trying to dig deep into this stuff and really understand how it all works.
Brendonovich
Brendonovich•5mo ago
well technically no components are server or client by default, their status as server or client depends on where they're imported from entrypoints like page.ts, layout.ts etc are server by default, but a random file without use client will be isomorphic and can end up in either or both the client and server module graphs back to your auth approach, it's secure as long as you correctly check that data you're accessing or mutating is owned by the current user
ChrisThornham
ChrisThornhamOP•5mo ago
You said: "as you correctly check that data you're accessing or mutating is owned by the current user" I agree. I think that's pretty straightforward once I use the access token to get the current user. Once I have the current user I can make sure the data I'm accessing or mutating is owned by the current user with a simple filter. Supabase uses .eq() 
const { data, error } = await supabase
  .from("users")
  .select()
  .eq("user_id", userID)
  .single();
const { data, error } = await supabase
  .from("users")
  .select()
  .eq("user_id", userID)
  .single();
Brendonovich
Brendonovich•5mo ago
yeah that'll do it
ChrisThornham
ChrisThornhamOP•5mo ago
Thank you! I really appreciate all your help with this. I'll explore your points above a bit more.
Want results from more Discord servers?
Add your server