Skip to content

security(rls): restrict courses/professors UPDATE to admin #75

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
roshanraj9136 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

security(rls): restrict courses/professors UPDATE to admin #75

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions src/migrations/migration.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ CREATE POLICY course_insert ON courses
FOR INSERT WITH CHECK (is_admin());

CREATE POLICY course_update ON courses
FOR UPDATE USING (true) WITH CHECK (true);
FOR UPDATE USING (is_admin()) WITH CHECK (is_admin());
Comment on lines 395 to +396

@coderabbitai coderabbitai Bot Jun 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
fd -t f -e sql src/migrations
rg -n -C2 'DROP POLICY|ALTER POLICY|CREATE POLICY course_update|CREATE POLICY professor_update' src/migrations

Repository: OpenLake/RateMyCourse

Length of output: 1058

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) List SQL files under src/migrations
echo "== SQL files =="
fd -t f -e sql src/migrations || true

# 2) Show all occurrences of the policy names across the repo
echo
echo "== Occurrences of course_update =="
rg -n --hidden --no-ignore -S "course_update" . || true

echo
echo "== Occurrences of professor_update =="
rg -n --hidden --no-ignore -S "professor_update" . || true

# 3) Search for policy update statements around bootstrap/drop/recreate schema markers
echo
echo "== Policy-related statements in migrations files =="
fd -t f -e sql src/migrations | xargs -r rg -n --hidden --no-ignore -S "DROP POLICY|ALTER POLICY|CREATE POLICY" || true

# 4) Identify migration framework hints (optional, lightweight)
echo
echo "== Migration framework hints (best-effort) =="
rg -n --hidden --no-ignore -S "flyway|goose|liquibase|knex|sequelize|prisma|migrate|migration" src/migrations . || true

Repository: OpenLake/RateMyCourse

Length of output: 1500

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== SQL files anywhere with migrations in path =="
find . -type f -name '*.sql' -path '*migrations*' | sed 's|^\./||' | sort

echo
echo "== All DROP/ALTER/CREATE POLICY statements across repo =="
rg -n --hidden --no-ignore -S "DROP POLICY|ALTER POLICY|CREATE POLICY" . | head -n 200

echo
echo "== CREATE POLICY statements that use is_admin() =="
rg -n --hidden --no-ignore -S "CREATE POLICY" src/migrations/migration.sql
rg -n --hidden --no-ignore -S "is_admin\\(\\)" src/migrations/migration.sql

echo
echo "== migration.sql header (first 60 lines) =="
nl -ba src/migrations/migration.sql | sed -n '1,60p'

echo
echo "== migration.sql policy area (lines 360-430) =="
nl -ba src/migrations/migration.sql | sed -n '360,430p'

Repository: OpenLake/RateMyCourse

Length of output: 4399

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== file stats =="
wc -l src/migrations/migration.sql
echo

echo "== first 120 lines =="
cat -n src/migrations/migration.sql | sed -n '1,120p'
echo

echo "== lines 330-460 (policy area) =="
cat -n src/migrations/migration.sql | sed -n '330,460p'
echo

echo "== does migration.sql contain any 'DROP TABLE'/'CREATE TABLE' schema drop? =="
rg -n --no-ignore --hidden -S "DROP TABLE|CREATE TABLE|DROP SCHEMA|CREATE SCHEMA" src/migrations/migration.sql || true

echo
echo "== does it drop/recreate policies earlier? =="
rg -n --no-ignore --hidden -S "DROP POLICY|CREATE POLICY" src/migrations/migration.sql | head -n 60

Repository: OpenLake/RateMyCourse

Length of output: 11715

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Directories named migrations (max depth 4) =="
find . -maxdepth 4 -type d -iname '*migrations*' | sed 's|^\./||' | sort

echo
echo "== SQL files with 'migration' in filename (full repo) =="
find . -type f -iname '*migration*.sql' | sed 's|^\./||' | sort

echo
echo "== SQL files in root-level migrations dirs (supabase, db, etc.) =="
find . -type f -name '*.sql' -path '*migrations*' | sed 's|^\./||' | sort

Repository: OpenLake/RateMyCourse

Length of output: 313

Add a forward migration to patch existing DB RLS policies (course_update/professor_update)

src/migrations/migration.sql is a bootstrap script that drops/recreates tables; it also defines course_update (395-396) and professor_update (408-409), but there is no separate forward migration to update policies on already-deployed databases. Without an in-repo forward migration, stale vulnerable policies may remain active.

-- New forward migration (example)
DROP POLICY IF EXISTS course_update ON courses;
CREATE POLICY course_update ON courses
  FOR UPDATE USING (is_admin()) WITH CHECK (is_admin());

DROP POLICY IF EXISTS professor_update ON professors;
CREATE POLICY professor_update ON professors
  FOR UPDATE USING (is_admin()) WITH CHECK (is_admin());
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/migrations/migration.sql` around lines 395 - 396, The migration bootstrap
defines RLS policies course_update and professor_update but there is no forward
migration to update those policies on existing DBs; add a new forward SQL
migration that drops each policy if it exists and recreates it with the secure
definition (DROP POLICY IF EXISTS course_update ON courses; CREATE POLICY
course_update ON courses FOR UPDATE USING (is_admin()) WITH CHECK (is_admin());
and similarly for professor_update on the professors table) so deployed
databases get the patched policies applied; include both DROP and CREATE
statements in the new migration file and ensure it is ordered after existing
schema migrations.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
python - <<'PY'
from pathlib import Path
import re

sql = Path("src/migrations/migration.sql").read_text()

for fn in ("update_course_ratings", "update_professor_ratings"):
    m = re.search(
        rf"CREATE OR REPLACE FUNCTION\s+{fn}\s*\(\)\s*RETURNS\s+TRIGGER(?P<header>.*?)AS\s+\$\$",
        sql,
        flags=re.IGNORECASE | re.DOTALL,
    )
    if not m:
        print(f"{fn}: NOT FOUND")
        continue
    header = " ".join(m.group("header").split()).upper()
    print(f"{fn}: SECURITY DEFINER present? {'SECURITY DEFINER' in header}")
PY

Repository: OpenLake/RateMyCourse

Length of output: 178

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="src/migrations/migration.sql"

# Show the policy area around 395-420
echo "=== RLS policy area (lines 380-430) ==="
nl -ba "$FILE" | sed -n '380,430p'

# Show trigger function definitions area where review comment claims they are defined (lines 120-260)
echo
echo "=== Trigger function definitions area (lines 100-280) ==="
nl -ba "$FILE" | sed -n '100,280p'

# Extract direct evidence of triggers on reviews and updates to courses/professors
echo
echo "=== Find triggers on reviews and updates to courses/professors ==="
rg -n "CREATE TRIGGER|ON reviews" "$FILE"
echo
rg -n "UPDATE\s+courses|UPDATE\s+professors" "$FILE" | head -n 50

Repository: OpenLake/RateMyCourse

Length of output: 166

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="src/migrations/migration.sql"

echo "=== RLS policy area (lines 360-430) ==="
awk 'NR>=360 && NR<=430 {printf "%d:%s\n", NR, $0}' "$FILE"

echo
echo "=== Trigger function definitions area (lines 80-280) ==="
awk 'NR>=80 && NR<=280 {printf "%d:%s\n", NR, $0}' "$FILE"

echo
echo "=== Locations: CREATE TRIGGER and ON reviews ==="
rg -n "CREATE TRIGGER|ON reviews" "$FILE" || true

echo
echo "=== Locations: UPDATE courses / UPDATE professors ==="
rg -n "UPDATE\s+courses|UPDATE\s+professors" "$FILE" || true

echo
echo "=== Function headers (repeat) ==="
python3 - <<'PY'
from pathlib import Path
import re
sql = Path("src/migrations/migration.sql").read_text()
for fn in ("update_course_ratings", "update_professor_ratings"):
    m = re.search(rf"CREATE OR REPLACE FUNCTION\s+{fn}\s*\(\)\s*RETURNS\s+TRIGGER(?P<header>.*?)AS\s+\$\$",
                  sql, flags=re.IGNORECASE|re.DOTALL)
    if not m:
        print(f"{fn}: NOT FOUND")
        continue
    header = " ".join(m.group("header").split()).upper()
    print(f"{fn}: SECURITY DEFINER present? {'SECURITY DEFINER' in header}")
    # Print a compact header excerpt
    print(f"{fn}: header excerpt: {header[:220]}...")
PY

Repository: OpenLake/RateMyCourse

Length of output: 11146

🏁 Script executed:

#!/bin/bash
set -euo pipefail
FILE="src/migrations/migration.sql"

echo "=== Review policies area (lines 420-470) ==="
awk 'NR>=420 && NR<=470 {printf "%d:%s\n", NR, $0}' "$FILE"

echo
echo "=== Trigger statements for rating/votes/flag/sanitize (lines 290-360) ==="
awk 'NR>=290 && NR<=360 {printf "%d:%s\n", NR, $0}' "$FILE"

echo
echo "=== Target table column names check (spot relevant updates) ==="
rg -n "overall_rating|difficulty_rating|workload_rating|review_count|knowledge_rating|teaching_rating|approachability_rating" "$FILE" | head -n 50

Repository: OpenLake/RateMyCourse

Length of output: 5979

🏁 Script executed:

#!/bin/bash
set -euo pipefail
FILE="src/migrations/migration.sql"

echo "=== Search for FORCE ROW LEVEL SECURITY / BYPASSRLS / OWNER TO ==="
rg -n "FORCE ROW LEVEL SECURITY|BYPASSRLS|OWNER TO" "$FILE" || true

echo
echo "=== Search for ALTER TABLE ... ENABLE ROW LEVEL SECURITY ==="
rg -n "ALTER TABLE .* ENABLE ROW LEVEL SECURITY" "$FILE" || true

echo
echo "=== Quick context: course/professor policies (just in case) ==="
rg -n "CREATE POLICY (course_update|professor_update) " "$FILE" -n -C 2

Repository: OpenLake/RateMyCourse

Length of output: 947

RLS breakage: reviews triggers update courses/professors, but those UPDATE policies are admin-only while trigger functions aren’t SECURITY DEFINER

  • course_update / professor_update restrict UPDATE courses / UPDATE professors to is_admin() (lines 395-396, 408-409).
  • update_course_ratings / update_professor_ratings run UPDATE courses / UPDATE professors from AFTER INSERT OR UPDATE OR DELETE ON reviews triggers, but both functions are defined without SECURITY DEFINER, so non-admin review writes can be rolled back by RLS when the trigger-side UPDATE is evaluated.
🔧 Proposed fix 
-CREATE OR REPLACE FUNCTION update_course_ratings() 
-RETURNS TRIGGER AS $$
+CREATE OR REPLACE FUNCTION update_course_ratings() 
+RETURNS TRIGGER
+LANGUAGE plpgsql
+SECURITY DEFINER
+SET search_path = public, pg_temp
+AS $$
@@
-END;
-$$ LANGUAGE plpgsql;
+END;
+$$;

-CREATE OR REPLACE FUNCTION update_professor_ratings() 
-RETURNS TRIGGER AS $$
+CREATE OR REPLACE FUNCTION update_professor_ratings() 
+RETURNS TRIGGER
+LANGUAGE plpgsql
+SECURITY DEFINER
+SET search_path = public, pg_temp
+AS $$
@@
-END;
-$$ LANGUAGE plpgsql;
+END;
+$$;
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/migrations/migration.sql` around lines 395 - 396, The RLS breakage arises
because the trigger functions update_course_ratings and update_professor_ratings
perform UPDATEs on courses/professors while those UPDATE policies (course_update
and professor_update) require is_admin(), but the functions are not SECURITY
DEFINER so non-admin review writes get blocked; fix by modifying the trigger
function definitions in the migration to declare them as SECURITY DEFINER (add
the SECURITY DEFINER clause to update_course_ratings and
update_professor_ratings), ensure those functions are owned by an administrative
role that satisfies is_admin() (or adjust their owner accordingly after
creation), and set a safe search_path inside the function to avoid privilege
escalation. This change keeps the existing course_update and professor_update
policies intact while allowing the trigger-side UPDATEs to run with admin
privileges.


CREATE POLICY course_delete ON courses
FOR DELETE USING (is_admin());
Expand All @@ -406,7 +406,7 @@ CREATE POLICY professor_insert ON professors
FOR INSERT WITH CHECK (is_admin());

CREATE POLICY professor_update ON professors
FOR UPDATE USING (true) WITH CHECK (true);
FOR UPDATE USING (is_admin()) WITH CHECK (is_admin());

CREATE POLICY professor_delete ON professors
FOR DELETE USING (is_admin());
Expand Down