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 numberDiff line numberDiff 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

@coderabbitaicoderabbitaiBotJun 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/bashset -euo pipefail
# 1) List SQL files under src/migrationsecho"== SQL files =="
fd -t f -e sql src/migrations ||true# 2) Show all occurrences of the policy names across the repoechoecho"== Occurrences of course_update =="
rg -n --hidden --no-ignore -S "course_update".||trueechoecho"== Occurrences of professor_update =="
rg -n --hidden --no-ignore -S "professor_update".||true# 3) Search for policy update statements around bootstrap/drop/recreate schema markersechoecho"== 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)echoecho"== 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/bashset -euo pipefail
echo"== SQL files anywhere with migrations in path =="
find . -type f -name '*.sql' -path '*migrations*'| sed 's|^\./||'| sort
echoecho"== All DROP/ALTER/CREATE POLICY statements across repo =="
rg -n --hidden --no-ignore -S "DROP POLICY|ALTER POLICY|CREATE POLICY".| head -n 200
echoecho"== 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
echoecho"== migration.sql header (first 60 lines) =="
nl -ba src/migrations/migration.sql | sed -n '1,60p'echoecho"== 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/bashset -euo pipefail
echo"== file stats =="
wc -l src/migrations/migration.sql
echoecho"== first 120 lines =="
cat -n src/migrations/migration.sql | sed -n '1,120p'echoecho"== lines 330-460 (policy area) =="
cat -n src/migrations/migration.sql | sed -n '330,460p'echoecho"== 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 ||trueechoecho"== 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/bashset -euo pipefail
echo"== Directories named migrations (max depth 4) =="
find . -maxdepth 4 -type d -iname '*migrations*'| sed 's|^\./||'| sort
echoecho"== SQL files with 'migration' in filename (full repo) =="
find . -type f -iname '*migration*.sql'| sed 's|^\./||'| sort
echoecho"== 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 Pathimport resql = 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/bashset -euo pipefail
FILE="src/migrations/migration.sql"# Show the policy area around 395-420echo"=== 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)echoecho"=== 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/professorsechoecho"=== 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/bashset -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"echoecho"=== Trigger function definitions area (lines 80-280) ==="
awk 'NR>=80 && NR<=280 {printf "%d:%s\n", NR, $0}'"$FILE"echoecho"=== Locations: CREATE TRIGGER and ON reviews ==="
rg -n "CREATE TRIGGER|ON reviews""$FILE"||trueechoecho"=== Locations: UPDATE courses / UPDATE professors ==="
rg -n "UPDATE\s+courses|UPDATE\s+professors""$FILE"||trueechoecho"=== Function headers (repeat) ==="
python3 - <<'PY'from pathlib import Pathimport resql = 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/bashset -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"echoecho"=== Trigger statements for rating/votes/flag/sanitize (lines 290-360) ==="
awk 'NR>=290 && NR<=360 {printf "%d:%s\n", NR, $0}'"$FILE"echoecho"=== 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/bashset -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"||trueechoecho"=== Search for ALTER TABLE ... ENABLE ROW LEVEL SECURITY ==="
rg -n "ALTER TABLE .* ENABLE ROW LEVEL SECURITY""$FILE"||trueechoecho"=== 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