Skip to content

fix: restrict team RLS policy to members only#117

Open
FOX-KNIGHT wants to merge 3 commits intoAOSSIE-Org:mainfrom
FOX-KNIGHT:fix/secure-teams-rls
Open

fix: restrict team RLS policy to members only#117
FOX-KNIGHT wants to merge 3 commits intoAOSSIE-Org:mainfrom
FOX-KNIGHT:fix/secure-teams-rls

Conversation

@FOX-KNIGHT
Copy link

@FOX-KNIGHT FOX-KNIGHT commented Jan 3, 2026

Closes #116

📝 Description

This PR addresses a Critical Security Vulnerability in sqls/02_user_auth_policies.sql.
The previous Row-Level Security (RLS) policy for the teams table was configured with a wildcard TRUE condition, which allowed any authenticated user to query and view all teams in the database, including private team_codes and admin emails.

This fix restricts access so that users can only view teams they are explicitly members of.

🔧 Changes Made

  • Modified sqls/02_user_auth_policies.sql:
    • Removed the insecure USING (TRUE) policy.
    • Added a subquery check: USING (id IN (SELECT team_id FROM users WHERE id = auth.uid())).

✅ Verified App Execution

Validating the build integrity after security patches.

✅ Checklist

  • I have read the contributing guidelines.
  • I have added tests that prove my fix is effective.
  • I have added necessary documentation (if applicable).
  • Any dependent changes have been merged and published in downstream modules.

Summary by CodeRabbit

  • Security

    • Team data access is now restricted so users can only view their own team records.
  • Chores

    • Removed a packaged logo asset, so that image is no longer included in the app build.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 3, 2026

📝 Walkthrough

Walkthrough

Removed the ELL-ena-logo/ asset entry from pubspec.yaml, and tightened the Row-Level Security policy on teams in sqls/02_user_auth_policies.sql so SELECT is limited to teams the authenticated user belongs to.

Changes

Cohort / File(s) Summary
Asset configuration
pubspec.yaml
Removed the ELL-ena-logo/ entry from the Flutter assets: list (no new assets added).
Security policy
sqls/02_user_auth_policies.sql
Updated RLS policy "Team members can view their team" on teams FOR SELECT USING clause from TRUE to id IN (SELECT team_id FROM users WHERE id = auth.uid()), restricting reads to a user's own team(s).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • SharkyBytes

Poem

🐰 I nibble lines of code and cheer,
The logo path has hopped away,
Permissions snug, teams held near,
I thump my foot — safe work today! 🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR includes an out-of-scope change: removal of the ELL-ena-logo asset from pubspec.yaml is unrelated to the RLS security fix in the linked issue #116. Remove the pubspec.yaml change or address it in a separate PR focused on asset management, keeping this PR focused solely on the security fix.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: restricting the team RLS policy to members only, which directly addresses the core fix in the pull request.
Linked Issues check ✅ Passed The PR implements the exact fix recommended in issue #116: replacing the insecure USING (TRUE) policy with a membership check that restricts team access to users who belong to the team.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
sqls/02_user_auth_policies.sql (1)

25-27: Consider using the user_teams view for consistency.

The policy queries the users table directly, while the existing "Users can view team members" policy (lines 46-48) uses the user_teams view defined at line 14. Using the view consistently across all policies would improve maintainability.

🔎 Proposed refactor using user_teams view
 CREATE POLICY "Team members can view their team" 
   ON teams FOR SELECT 
   USING (
     id IN (
-      SELECT team_id FROM users WHERE id = auth.uid()
+      SELECT team_id FROM user_teams WHERE id = auth.uid()
     )
   );
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 235a88b and 105ab2e.

📒 Files selected for processing (2)
  • pubspec.yaml
  • sqls/02_user_auth_policies.sql
🔇 Additional comments (2)
sqls/02_user_auth_policies.sql (2)

34-36: Verify permissive INSERT policies and GRANT ALL permissions are intentional.

While the SELECT policy was tightened to restrict team visibility, the INSERT policies (lines 36, 57) allow any authenticated user to create teams and users with WITH CHECK (TRUE). Additionally, GRANT ALL (lines 62-63) provides full permissions to both anonymous and authenticated users.

Please confirm this permission model is intentional and aligns with your security requirements, especially given the effort to restrict SELECT access.

Consider whether:

  • Team creation should be restricted to specific roles or conditions
  • User creation should validate team membership or require authorization
  • More granular grants (SELECT, INSERT, UPDATE, DELETE) would be more appropriate than GRANT ALL

Also applies to: 55-57, 62-63


25-27: Security fix correctly implements membership check.

The policy at lines 25-27 successfully restricts team visibility to only teams where the authenticated user is a member. The subquery pattern id IN (SELECT team_id FROM users WHERE id = auth.uid()) correctly prevents the vulnerability described in issue #116.

The required indexes already exist in the schema: idx_users_team_id on users(team_id) and the primary key index on users(id).

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
pubspec.yaml (1)

95-95: Good cleanup - addresses previous feedback.

The removal of the ELL-ena-logo/ asset entry correctly addresses the prior review concern about bundling unrelated changes with the security fix. The launcher icon configuration (lines 130, 132) can still reference these files since they're processed at build time, not runtime.

The blank line could optionally be removed for cleaner formatting.

🔎 Optional: Remove blank line
  assets:
    - .env
-
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 105ab2e and 47da578.

📒 Files selected for processing (1)
  • pubspec.yaml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: SharkyBytes
Repo: AOSSIE-Org/Ell-ena PR: 3
File: lib/screens/auth/signup_screen.dart:98-106
Timestamp: 2025-07-03T00:01:13.873Z
Learning: In the Ell-ena Flutter project, the team is keeping authentication validation simple during development (e.g., 6-character minimum passwords) for testing convenience, with plans to enforce stronger security rules later when integrating full authentication flows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: Authenticated users can list all teams due to wildcard RLS

1 participant