Merge pull request 'Security fixes: session hardening, open redirect, SSR path injection' (#1) from security-fixes into main

Reviewed-on: #1

app/routes/login.tsx

@@ -22,9 +22,23 @@ import { useToast } from "~/hooks/use-toast";
 import { useSubmit, useLoaderData } from "@remix-run/react";
 import { User } from "~/utils/token-manager.server";
 
+// Only allow same-site, path-relative redirects. Rejects absolute URLs
+// (https://evil.com) and protocol-relative URLs (//evil.com) to prevent
+// open-redirect / phishing after login.
+function safeRedirect(
+  to: FormDataEntryValue | string | null,
+  defaultRedirect = "/"
+): string {
+  if (!to || typeof to !== "string") return defaultRedirect;
+  if (!to.startsWith("/") || to.startsWith("//") || to.startsWith("/\\")) {
+    return defaultRedirect;
+  }
+  return to;
+}
+
 export async function loader({ request }: LoaderFunctionArgs) {
   const url = new URL(request.url);
-  const redirectTo = url.searchParams.get("redirectTo") || "/";
+  const redirectTo = safeRedirect(url.searchParams.get("redirectTo"));
   return json({ redirectTo });
 }
 
@@ -290,7 +304,7 @@ export const meta: MetaFunction = () => {
 export async function action({ request }: ActionFunctionArgs) {
   const clonedRequest = request.clone();
   const formData = await clonedRequest.formData();
-  const redirectTo = (formData.get("redirectTo") as string) || "/";
+  const redirectTo = safeRedirect(formData.get("redirectTo"));
 
   try {
     const user = (await authenticator.authenticate(
@@ -315,7 +329,7 @@ export async function action({ request }: ActionFunctionArgs) {
       refresh_token_timestamp: currentTime,
     });
 
-    return redirect(redirectTo || "/", {
+    return redirect(redirectTo, {
       headers: {
         "Set-Cookie": await sessionStorage.commitSession(session, {
           expires: new Date(new Date().setMonth(new Date().getMonth() + 1)),

app/routes/product.$id.tsx

@@ -20,7 +20,7 @@ export async function loader({ params }: LoaderFunctionArgs) {
   const { id } = params;
   try {
     const response = await fetch(
-      `${getInternalApiUrl()}/api/product/v1/products/${id}/`
+      `${getInternalApiUrl()}/api/product/v1/products/${encodeURIComponent(id ?? "")}/`
     );
 
     if (!response.ok) {

app/routes/seller.$sellerId._index.tsx

@@ -59,7 +59,7 @@ export async function loader({ params }: LoaderFunctionArgs) {
     throw new Response("Seller ID is required", { status: 400 });
   }
   try {
-    const response = await fetch(`${getInternalApiUrl()}/seller/${sellerId}/`);
+    const response = await fetch(`${getInternalApiUrl()}/seller/${encodeURIComponent(sellerId)}/`);
 
     if (!response.ok) {
       throw new Error(`Failed to fetch seller: ${response.status}`);

app/routes/store.$storeId.product.$id.tsx

@@ -71,7 +71,7 @@ export async function loader({ params }: LoaderFunctionArgs) {
   const { id } = params;
   try {
     const response = await fetch(
-      `${getInternalApiUrl()}/api/product/v1/products/${id}/`,
+      `${getInternalApiUrl()}/api/product/v1/products/${encodeURIComponent(id ?? "")}/`,
     );
 
     if (!response.ok) {

app/sessions.server.ts

@@ -1,6 +1,17 @@
 import { createCookieSessionStorage, Session } from "@remix-run/node";
 import { isProduction } from "./utils/get-env";
 
+const isProd = process.env.NODE_ENV === "production";
+const sessionSecret = process.env.REMIX_SECRET;
+
+// Fail closed: never ship a guessable signing secret to production. A signed
+// cookie with a known secret can be forged, allowing session tampering.
+if (isProd && !sessionSecret) {
+  throw new Error(
+    "REMIX_SECRET environment variable must be set in production."
+  );
+}
+
 // export the whole sessionStorage object
 export const sessionStorage = createCookieSessionStorage({
   cookie: {
@@ -8,8 +19,11 @@ export const sessionStorage = createCookieSessionStorage({
     sameSite: "lax", // this helps with CSRF
     path: "/", // remember to add this so the cookie will work in all routes
     httpOnly: true, // for security reasons, make this cookie http only
-    secrets: [process.env.REMIX_SECRET ?? "secret"], // replace this with an actual secret
-    secure: isProduction(), // enable this in prod only
+    maxAge: 60 * 60 * 24 * 30, // 30 days (matches refresh-token lifetime)
+    // Dev-only fallback; production is guarded by the check above.
+    secrets: [sessionSecret ?? "dev-only-insecure-secret"],
+    // Send only over HTTPS in production (NODE_ENV is the reliable server signal).
+    secure: isProd || isProduction(),
   },
 });
 export const { getSession, commitSession, destroySession } = sessionStorage;