Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 92 additions & 0 deletions backend/routes/users.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ import path from "path";
import fs from "fs";
import { fileURLToPath } from "url";
import { fileTypeFromFile } from "file-type";
import { createClient } from "@supabase/supabase-js";
import { requireAuth } from "../middlewares/requireAuth.js";
import { validate } from "../middlewares/validate.js";
import { profileSchemas } from "../validation/schemas.js";

const __filename = fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);
Expand Down Expand Up @@ -72,6 +75,95 @@ const safeUnlink = (filePath) => {
}
};

// SECURITY (#1851): Whitelist of fields that standard users can update on their own profile.
// Privileged fields (is_admin, is_mentor, points, rating, badges, sessions_completed,
// streak, etc.) are explicitly excluded to prevent mass-assignment privilege escalation.
const ALLOWED_PROFILE_FIELDS = new Set([
"name",
"bio",
"skills",
"avatar_url",
"interests",
"teach_subjects",
"learn_subjects",
]);

/**
* SECURITY (#1851): Server-side profile update endpoint with strict field whitelisting.
*
* This endpoint provides defense-in-depth against mass-assignment attacks.
* Even though Supabase RLS protects privileged columns, this endpoint:
* 1. Validates input with Zod (.strict() rejects unknown keys)
* 2. Explicitly whitelists allowed fields before writing to database
* 3. Uses the service-role client to bypass RLS (validating authorization server-side)
* 4. Logs all profile update attempts for audit trail
*/
router.put("/:userId/profile", requireAuth, validate(profileSchemas.updateProfile), async (req, res) => {
try {
const { userId } = req.params;
const authenticatedUserId = req.user?.id;

// SECURITY: Ensure users can only update their own profile (or admins can update any)
const isAdmin = req.roles?.includes("admin") || req.user?.app_metadata?.role === "admin";
if (userId !== authenticatedUserId && !isAdmin) {
return res.status(403).json({ error: "Not authorized to update this profile" });
}

// SECURITY (#1851): Strict field whitelisting — only extract allowed fields
const allowedUpdates = {};
for (const field of ALLOWED_PROFILE_FIELDS) {
if (req.body[field] !== undefined) {
allowedUpdates[field] = req.body[field];
}
}

// Reject if no valid fields were provided
if (Object.keys(allowedUpdates).length === 0) {
return res.status(400).json({ error: "No valid fields to update" });
}

// SECURITY: Explicitly reject any privileged field attempts and log them
const BLOCKED_FIELDS = ["is_admin", "is_mentor", "points", "rating", "badges", "sessions_completed", "role", "permissions"];
const attemptedPrivilegeEscalation = BLOCKED_FIELDS.filter(f => req.body[f] !== undefined);
if (attemptedPrivilegeEscalation.length > 0) {
console.error(
`[SECURITY] #1851 Privilege escalation attempt blocked. User: ${authenticatedUserId}, ` +
`Target: ${userId}, Attempted fields: ${attemptedPrivilegeEscalation.join(", ")}`
);
return res.status(403).json({
error: "Permission denied: cannot modify restricted fields",
});
}

// Use service-role client for server-side update (bypasses RLS since we've validated authorization)
const supabaseUrl = process.env.SUPABASE_URL || process.env.VITE_SUPABASE_URL;
const serviceRoleKey = process.env.SUPABASE_SERVICE_ROLE_KEY;

if (!supabaseUrl || !serviceRoleKey) {
return res.status(500).json({ error: "Server configuration error" });
}

const supabase = createClient(supabaseUrl, serviceRoleKey);

const { data, error } = await supabase
.from("profiles")
.update(allowedUpdates)
.eq("id", userId)
.select("id, name, bio, skills, avatar_url, interests, teach_subjects, learn_subjects")
.single();

if (error) {
console.error("Profile update error:", error.message);
return res.status(500).json({ error: "Failed to update profile" });
}

res.json({ success: true, profile: data });
} catch (err) {
console.error("Profile update error:", err);
res.status(500).json({ error: "Internal server error" });
}
});

// User profile photo upload endpoint
router.post("/upload-photo", requireAuth, uploadProfilePhoto, async (req, res) => {
if (!req.file) {
Expand Down
14 changes: 14 additions & 0 deletions backend/validation/schemas.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,20 @@ export const aiSchemas = {
},
};

export const profileSchemas = {
updateProfile: {
body: z.object({
name: z.string().trim().max(50).optional(),
bio: z.string().trim().max(300).optional(),
skills: z.array(z.string().trim().max(50)).max(20).optional(),
avatar_url: z.string().trim().max(500).optional(),
interests: z.array(z.string().trim().max(50)).max(20).optional(),
teach_subjects: z.array(z.string().trim().max(50)).max(20).optional(),
learn_subjects: z.array(z.string().trim().max(50)).max(20).optional(),
}).strict(), // .strict() rejects any unknown keys — prevents mass assignment
},
};

export const matchSchemas = {
getRecommendedPartners: {
query: z.object({
Expand Down
44 changes: 30 additions & 14 deletions src/pages/EditProfile.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { motion } from "framer-motion";
import { User, FileText, Code, Save, ArrowLeft, Loader2 } from "lucide-react";
import { Button } from "@/components/ui/button";
import { Input } from "@/components/ui/input";
import { API_BASE_URL } from "@/config/api";

interface ProfileState {
name: string;
Expand Down Expand Up @@ -65,22 +66,37 @@ const EditProfile = () => {

setIsSaving(true);
try {
const { error } = await supabase
.from("profiles")
.update({
// SECURITY (#1851): Use server-side endpoint with field whitelisting
// instead of direct Supabase client call to prevent mass-assignment attacks.
const { data: { session } } = await supabase.auth.getSession();
if (!session?.access_token) {
toast.error("Your session has expired. Please log in again.");
return;
}

const skillsArray =
typeof profile.skills === "string"
? profile.skills.split(",").map((s) => s.trim()).filter(Boolean)
: profile.skills;

const res = await fetch(`${API_BASE_URL}/api/users/${userId}/profile`, {
method: "PUT",
headers: {
"Content-Type": "application/json",
Authorization: `Bearer ${session.access_token}`,
},
credentials: "include",
body: JSON.stringify({
name: profile.name,
bio: profile.bio,
skills:
typeof profile.skills === "string"
? profile.skills
.split(",")
.map((s) => s.trim())
.filter(Boolean)
: profile.skills,
})
.eq("id", userId as string);

if (error) throw error;
skills: skillsArray,
}),
});

if (!res.ok) {
const data = await res.json().catch(() => ({}));
throw new Error(data.error || "Failed to update profile");
}
toast.success("Profile updated successfully!");
navigate("/profile");
} catch (err: unknown) {
Expand Down
31 changes: 22 additions & 9 deletions src/pages/Profile.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { useEffect, useState } from "react";
import { toast } from "sonner";
import { supabase } from "@/integrations/supabase/client";
import { motion } from "framer-motion";
import { API_BASE_URL } from "@/config/api";

import { Camera, Save, Sparkles, User, Flame, Zap, Trophy, Lock, Settings } from "lucide-react";
import StreakStats from "@/components/StreakStats";
Expand Down Expand Up @@ -96,21 +97,33 @@ if (profile.bio.length > MAX_BIO_CHARS) {
toast.error(`Bio must be ${MAX_BIO_CHARS} characters or fewer.`);
return;
}
const { error } = await supabase
.from("profiles")
.update({
const { data: { session } } = await supabase.auth.getSession();
if (!session?.access_token) {
toast.error("Your session has expired. Please log in again.");
return;
}

const res = await fetch(`${API_BASE_URL}/api/users/${user.id}/profile`, {
method: "PUT",
headers: {
"Content-Type": "application/json",
Authorization: `Bearer ${session.access_token}`,
},
credentials: "include",
body: JSON.stringify({
name: profile.name,
bio: profile.bio,
skills: Array.isArray(profile.skills) ? profile.skills : profile.skills.split(",").map((s: string) => s.trim()).filter(Boolean),
avatar_url: profile.avatar_url,
})
.eq("id", user.id);
}),
});

if (error) {
toast.error("Failed to update profile: " + error.message);
} else {
toast.success("Profile updated successfully!");
if (!res.ok) {
const data = await res.json().catch(() => ({}));
throw new Error(data.error || "Failed to update profile");
}

toast.success("Profile updated successfully!");
} catch (err: any) {
toast.error("An unexpected error occurred: " + err.message);
} finally {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
-- 20260727000000_fix_mass_assignment_privilege_escalation.sql
-- Fix #1851: Privilege Escalation Vulnerability in User Roles Management
--
-- PROBLEM:
-- The existing "Users can update their own profile" policy (from 20260617000000)
-- protects gamification columns (is_mentor, points, rating, badges, etc.) but
-- does NOT protect the `is_admin` column. An attacker can send
-- `{ "is_admin": true }` via the Supabase client to self-grant admin
-- privileges, bypassing all authorization checks.
--
-- FIX:
-- 1. Drop and recreate the profiles UPDATE policy with `is_admin` in the
-- WITH CHECK clause.
-- 2. Add a trigger that prevents standard users from modifying `is_admin`
-- or `is_mentor` via any direct SQL path (defense-in-depth).

DROP POLICY IF EXISTS "Users can update their own profile" ON public.profiles;

CREATE POLICY "Users can update their own profile" ON public.profiles
FOR UPDATE TO authenticated
USING (auth.uid() = id)
WITH CHECK (
auth.uid() = id
-- Privileged columns that must NOT be changed by the user:
AND is_admin IS NOT DISTINCT FROM (SELECT is_admin FROM public.profiles WHERE id = auth.uid())
AND is_mentor IS NOT DISTINCT FROM (SELECT is_mentor FROM public.profiles WHERE id = auth.uid())
AND points IS NOT DISTINCT FROM (SELECT points FROM public.profiles WHERE id = auth.uid())
AND rating IS NOT DISTINCT FROM (SELECT rating FROM public.profiles WHERE id = auth.uid())
AND badges IS NOT DISTINCT FROM (SELECT badges FROM public.profiles WHERE id = auth.uid())
AND sessions_completed IS NOT DISTINCT FROM (SELECT sessions_completed FROM public.profiles WHERE id = auth.uid())
AND streak IS NOT DISTINCT FROM (SELECT streak FROM public.profiles WHERE id = auth.uid())
AND previous_streak IS NOT DISTINCT FROM (SELECT previous_streak FROM public.profiles WHERE id = auth.uid())
AND last_active IS NOT DISTINCT FROM (SELECT last_active FROM public.profiles WHERE id = auth.uid())
AND restoration_used_today IS NOT DISTINCT FROM (SELECT restoration_used_today FROM public.profiles WHERE id = auth.uid())
AND restoration_date IS NOT DISTINCT FROM (SELECT restoration_date FROM public.profiles WHERE id = auth.uid())
);

-- Defense-in-depth trigger: prevents ANY direct UPDATE from modifying
-- is_admin or is_mentor. Only SECURITY DEFINER functions (which run as
-- the function owner, not the session user) can bypass this by calling
-- ALTER TABLE ... DISABLE TRIGGER temporarily, or by using a dedicated
-- admin RPC that has the necessary privileges.
--
-- NOTE: SECURITY DEFINER functions (like gamification RPCs) execute as
-- the function owner (postgres), so session_user = 'postgres' for those
-- calls. Regular client calls have session_user = 'authenticated'.
CREATE OR REPLACE FUNCTION public.prevent_privilege_escalation()
RETURNS TRIGGER AS $$
BEGIN
-- Block self-promotion of is_admin
IF NEW.is_admin IS DISTINCT FROM OLD.is_admin THEN
RAISE EXCEPTION 'Permission denied: cannot modify is_admin column directly';
END IF;

-- Block self-promotion of is_mentor
IF NEW.is_mentor IS DISTINCT FROM OLD.is_mentor THEN
RAISE EXCEPTION 'Permission denied: cannot modify is_mentor column directly';
END IF;

RETURN NEW;
END;
$$ LANGUAGE plpgsql;

-- Only create trigger if it doesn't already exist
DO $$
BEGIN
IF NOT EXISTS (
SELECT 1 FROM pg_trigger WHERE tgname = 'trg_prevent_privilege_escalation'
) THEN
CREATE TRIGGER trg_prevent_privilege_escalation
BEFORE UPDATE ON public.profiles
FOR EACH ROW
EXECUTE FUNCTION public.prevent_privilege_escalation();
END IF;
END $$;

-- Add comment documenting the security fix
COMMENT ON POLICY "Users can update their own profile" ON public.profiles IS
'Fix #1851: Prevents mass assignment privilege escalation. is_admin, is_mentor, points, rating, badges, sessions_completed, streak, and other server-managed columns are locked via WITH CHECK.';
COMMENT ON FUNCTION public.prevent_privilege_escalation() IS
'Fix #1851: Defense-in-depth trigger preventing direct modification of is_admin and is_mentor columns. Only SECURITY DEFINER functions (service-role) can modify these.';