Conversation
|
Updates to Preview Branch (eng-1426-bug-in-permission-functions) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| permissions_ := max( | ||
| my_permissions_in_space(space_id_), | ||
| COALESCE(local_account.permissions, | ||
| CASE WHEN local_account.space_editor THEN 'editor' ELSE 'reader' END)); |
There was a problem hiding this comment.
🔴 max() used as two-argument scalar function but it's an aggregate function in PostgreSQL
The code uses max(value1, value2) as if it were a two-argument scalar comparison function, but in PostgreSQL max() is an aggregate function that accepts only a single argument. This will cause a runtime error when upsert_account_in_space is called for a user with a non-null dg_account.
Root Cause and Impact
At packages/database/supabase/schemas/account.sql:219-222 and the corresponding migration file at line 35-38:
permissions_ := max(
my_permissions_in_space(space_id_),
COALESCE(local_account.permissions,
CASE WHEN local_account.space_editor THEN 'editor' ELSE 'reader' END));PostgreSQL's max() is an aggregate function (like SELECT max(column) FROM table) and does not accept two scalar arguments. Calling max(a, b) will produce an error like: function max(spaceaccesspermissions, spaceaccesspermissions) does not exist. There is no custom two-argument max function defined anywhere in the codebase.
The correct function for comparing two scalar values is GREATEST(a, b).
Impact: Every call to upsert_account_in_space (and by extension create_account_in_space, upsert_accounts_in_space) for a user with a linked dg_account (i.e., user_uid IS NOT NULL) will fail with a SQL error, completely breaking account creation/upsert for registered users.
| permissions_ := max( | |
| my_permissions_in_space(space_id_), | |
| COALESCE(local_account.permissions, | |
| CASE WHEN local_account.space_editor THEN 'editor' ELSE 'reader' END)); | |
| permissions_ := GREATEST( | |
| my_permissions_in_space(space_id_), | |
| COALESCE(local_account.permissions, | |
| CASE WHEN local_account.space_editor THEN 'editor' ELSE 'reader' END)); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| permissions_ := max( | ||
| my_permissions_in_space(space_id_), | ||
| COALESCE(local_account.permissions, | ||
| CASE WHEN local_account.space_editor THEN 'editor' ELSE 'reader' END)); |
There was a problem hiding this comment.
🔴 max() used as two-argument scalar function in migration file (same bug as BUG-0001)
The migration file has the same max() vs GREATEST() bug as the schema file.
Root Cause
At packages/database/supabase/migrations/20260213155959_upsert_user_permissions.sql:35-38:
permissions_ := max(
my_permissions_in_space(space_id_),
COALESCE(local_account.permissions,
CASE WHEN local_account.space_editor THEN 'editor' ELSE 'reader' END));Same issue as BUG-0001 — max() is an aggregate function in PostgreSQL, not a two-argument scalar comparison. This migration will fail when applied.
Impact: The migration itself will succeed (it's just a CREATE OR REPLACE FUNCTION), but any subsequent call to upsert_account_in_space for a user with user_uid IS NOT NULL will fail at runtime.
| permissions_ := max( | |
| my_permissions_in_space(space_id_), | |
| COALESCE(local_account.permissions, | |
| CASE WHEN local_account.space_editor THEN 'editor' ELSE 'reader' END)); | |
| permissions_ := GREATEST( | |
| my_permissions_in_space(space_id_), | |
| COALESCE(local_account.permissions, | |
| CASE WHEN local_account.space_editor THEN 'editor' ELSE 'reader' END)); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| permissions_ := max( | ||
| my_permissions_in_space(space_id_), | ||
| COALESCE(local_account.permissions, | ||
| CASE WHEN local_account.space_editor THEN 'editor' ELSE 'reader' END)); |
There was a problem hiding this comment.
🔴 my_permissions_in_space checks the calling user's permissions instead of the target user's existing permissions
The max/GREATEST logic compares the calling user's permissions with the requested permissions, but the intent (based on the old code) is to not downgrade the target user's existing permissions.
Detailed Explanation
The old code at packages/database/supabase/migrations/20260124215014_PartialSpaceAccess.sql:40-42 used sa.editor (the existing value in the conflicting row) to preserve the target user's current permissions:
DO UPDATE SET permissions = CASE
WHEN COALESCE(local_account.space_editor, sa.editor, true) THEN 'editor'
ELSE 'reader' END;The new code at packages/database/supabase/schemas/account.sql:219-222 replaces this with:
permissions_ := max(
my_permissions_in_space(space_id_),
COALESCE(local_account.permissions, ...));my_permissions_in_space(space_id_) uses auth.uid() which returns the calling user's UID, not the target user's (user_uid). This means the comparison is against the wrong user's permissions.
Scenario: If user A (a reader) calls upsert_account_in_space for user B (currently an editor) with permissions='reader', the result would be GREATEST('reader', 'reader') = 'reader', which downgrades user B from editor to reader. The old code would have preserved user B's editor status.
Additionally, upsert_account_in_space is SECURITY DEFINER, so it runs as the function owner (postgres). When it calls my_permissions_in_space, auth.uid() returns the JWT-based calling user's UID. But the target user (user_uid) may be a completely different user whose existing permissions should be preserved.
Impact: Users' permissions can be inadvertently downgraded when another user with lower permissions triggers an account upsert.
Prompt for agents
The `my_permissions_in_space(space_id_)` function returns the calling user's (auth.uid()) permissions, but the intent is to preserve the target user's existing permissions so they aren't downgraded. Instead of calling `my_permissions_in_space`, you should query the target user's existing permissions directly. For example, replace the GREATEST/max call with something like:
SELECT permissions INTO permissions_ FROM public."SpaceAccess" WHERE space_id = space_id_ AND account_uid = user_uid;
permissions_ := GREATEST(
permissions_,
COALESCE(local_account.permissions,
CASE WHEN local_account.space_editor THEN 'editor' ELSE 'reader' END));
This change needs to be made in both:
1. packages/database/supabase/schemas/account.sql lines 219-222
2. packages/database/supabase/migrations/20260213155959_upsert_user_permissions.sql lines 35-38
Was this helpful? React with 👍 or 👎 to provide feedback.
maparent
force-pushed
the
eng-1426-bug-in-permission-functions
branch
from
23b9348 to
10e2212
Compare
| permissions_ := max( | ||
| my_permissions_in_space(space_id_), | ||
| COALESCE(local_account.permissions, | ||
| CASE WHEN local_account.space_editor THEN 'editor' ELSE 'reader' END)); |
There was a problem hiding this comment.
🔴 GREATEST() grants permissions higher than caller's own, violating stated security intent
Even after fixing max() to GREATEST(), the logic computes the higher of the caller's permissions and the requested permissions. The PR description states the intent is to "make sure that one cannot give permissions higher than one's own," which requires LEAST(), not GREATEST().
Detailed Explanation
The enum ordering is 'partial' < 'reader' < 'editor' (defined at packages/database/supabase/schemas/account.sql:99-103). Using GREATEST(caller_perms, requested_perms) means:
- If caller has
'reader'and requests'editor'for another user →GREATEST('reader', 'editor')='editor'→ grants editor access despite caller only being a reader.
The correct logic to enforce "cannot give permissions higher than one's own" is:
permissions_ := LEAST(
my_permissions_in_space(space_id_),
COALESCE(local_account.permissions, ...));This would cap the granted permissions at the caller's own level.
Impact: A user with reader permissions could escalate another user's permissions to editor, which is a privilege escalation vulnerability. This affects both the schema file (packages/database/supabase/schemas/account.sql:219-222) and the migration (packages/database/supabase/migrations/20260213155959_upsert_user_permissions.sql:35-38).
Prompt for agents
In both packages/database/supabase/schemas/account.sql (lines 219-222) and packages/database/supabase/migrations/20260213155959_upsert_user_permissions.sql (lines 35-38), change GREATEST (or the current broken max) to LEAST to enforce the security constraint that a caller cannot grant permissions higher than their own:
permissions_ := LEAST(
my_permissions_in_space(space_id_),
COALESCE(local_account.permissions,
CASE WHEN local_account.space_editor THEN 'editor' ELSE 'reader' END));
This ensures the granted permissions are capped at the caller's own permission level in the space.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
https://linear.app/discourse-graphs/issue/ENG-1426/bug-in-permission-functions
Pointed out by Devin in the group-test branch: there was still an issue with using the obsolete editor field of the SpaceAccess structure in that code.
Surprisingly the bug did not make the function fail, probably because the value is rarely set.
Corrected, and generalized the use of permissions as an input parameter to create_account_in_space.
Also, make sure that one cannot give permissions higher than one's own this way.