security(rls): restrict courses/professors UPDATE to admin#75
Open
roshanraj9136 wants to merge 1 commit into
Open
security(rls): restrict courses/professors UPDATE to admin#75roshanraj9136 wants to merge 1 commit into
roshanraj9136 wants to merge 1 commit into
Conversation
The UPDATE policies on courses and professors used USING (true) WITH CHECK (true), allowing any authenticated user to modify any course or professor record (name, ratings, department, etc.). The comment said 'allow trigger updates' but triggers defined with SECURITY DEFINER bypass RLS entirely, so the permissive policy only benefited attackers. Restrict to is_admin() to match INSERT and DELETE policies.
|
@roshanraj9136 is attempting to deploy a commit to the OpenLake_Website Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis migration tightens Row-Level Security for ChangesRLS Security Tightening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/migrations/migration.sql`:
- Around line 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.
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39b4e4ef-3c26-4dab-9811-ae248464423b
📒 Files selected for processing (1)
src/migrations/migration.sql
Comment on lines
395
to
+396
| CREATE POLICY course_update ON courses | ||
| FOR UPDATE USING (true) WITH CHECK (true); | ||
| FOR UPDATE USING (is_admin()) WITH CHECK (is_admin()); |
There was a problem hiding this comment.
🧩 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/migrationsRepository: 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 . || trueRepository: 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 60Repository: 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|^\./||' | sortRepository: 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.
🧩 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}")
PYRepository: 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 50Repository: 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]}...")
PYRepository: 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 50Repository: 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 2Repository: 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_updaterestrictUPDATE courses/UPDATE professorstois_admin()(lines 395-396, 408-409).update_course_ratings/update_professor_ratingsrunUPDATE courses/UPDATE professorsfromAFTER INSERT OR UPDATE OR DELETE ON reviewstriggers, but both functions are defined withoutSECURITY 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Vulnerability
The RLS UPDATE policies on
coursesandprofessorstables are:This means any authenticated user can update any course or professor record — change names, ratings, departments, emails, etc. The original comment says 'allow trigger updates', but triggers defined with
SECURITY DEFINERbypass RLS entirely, so this permissive policy only benefits attackers.Impact
A malicious user with a valid session could:
Fix
Restrict UPDATE to
is_admin(), matching the existing INSERT and DELETE policies:Important
This fixes the migration file. If the database is already deployed, you also need to run this against the live DB:
File changed (1)
src/migrations/migration.sql— 2 lines changedSummary by CodeRabbit
Release Notes