From be94af386ca518f2fe06602a4119700e7548cd0a Mon Sep 17 00:00:00 2001 From: George K Date: Tue, 10 Feb 2026 16:17:29 -0800 Subject: [PATCH] chore(coderd/database): enforce workspace ACL JSON object constraints (#22019) The constraints prevent faulty code from saving 'null' as JSON and breaking the `workspaces_expanded` view. --- coderd/database/check_constraint.go | 2 + coderd/database/dump.sql | 4 +- ...7_workspace_acl_object_constraint.down.sql | 3 + ...417_workspace_acl_object_constraint.up.sql | 9 +++ ...pre_workspace_acl_object_constraint.up.sql | 35 +++++++++++ coderd/database/querier_test.go | 59 +++++++++++++++++++ 6 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 coderd/database/migrations/000417_workspace_acl_object_constraint.down.sql create mode 100644 coderd/database/migrations/000417_workspace_acl_object_constraint.up.sql create mode 100644 coderd/database/migrations/testdata/fixtures/000416_pre_workspace_acl_object_constraint.up.sql diff --git a/coderd/database/check_constraint.go b/coderd/database/check_constraint.go index ae22af7815..8a261b8603 100644 --- a/coderd/database/check_constraint.go +++ b/coderd/database/check_constraint.go @@ -17,4 +17,6 @@ const ( CheckTelemetryLockEventTypeConstraint CheckConstraint = "telemetry_lock_event_type_constraint" // telemetry_locks CheckValidationMonotonicOrder CheckConstraint = "validation_monotonic_order" // template_version_parameters CheckUsageEventTypeCheck CheckConstraint = "usage_event_type_check" // usage_events + CheckGroupAclIsObject CheckConstraint = "group_acl_is_object" // workspaces + CheckUserAclIsObject CheckConstraint = "user_acl_is_object" // workspaces ) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 868a8dc26c..b7d41e9058 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -2736,7 +2736,9 @@ CREATE TABLE workspaces ( favorite boolean DEFAULT false NOT NULL, next_start_at timestamp with time zone, group_acl jsonb DEFAULT '{}'::jsonb NOT NULL, - user_acl jsonb DEFAULT '{}'::jsonb NOT NULL + user_acl jsonb DEFAULT '{}'::jsonb NOT NULL, + CONSTRAINT group_acl_is_object CHECK ((jsonb_typeof(group_acl) = 'object'::text)), + CONSTRAINT user_acl_is_object CHECK ((jsonb_typeof(user_acl) = 'object'::text)) ); COMMENT ON COLUMN workspaces.favorite IS 'Favorite is true if the workspace owner has favorited the workspace.'; diff --git a/coderd/database/migrations/000417_workspace_acl_object_constraint.down.sql b/coderd/database/migrations/000417_workspace_acl_object_constraint.down.sql new file mode 100644 index 0000000000..ceccd55da6 --- /dev/null +++ b/coderd/database/migrations/000417_workspace_acl_object_constraint.down.sql @@ -0,0 +1,3 @@ +ALTER TABLE workspaces + DROP CONSTRAINT IF EXISTS group_acl_is_object, + DROP CONSTRAINT IF EXISTS user_acl_is_object; diff --git a/coderd/database/migrations/000417_workspace_acl_object_constraint.up.sql b/coderd/database/migrations/000417_workspace_acl_object_constraint.up.sql new file mode 100644 index 0000000000..58f8cc6d63 --- /dev/null +++ b/coderd/database/migrations/000417_workspace_acl_object_constraint.up.sql @@ -0,0 +1,9 @@ +-- Add constraints that reject 'null'::jsonb for group and user ACLs +-- because they would break the new workspace_expanded view. + +UPDATE workspaces SET group_acl = '{}'::jsonb WHERE group_acl = 'null'::jsonb; +UPDATE workspaces SET user_acl = '{}'::jsonb WHERE user_acl = 'null'::jsonb; + +ALTER TABLE workspaces + ADD CONSTRAINT group_acl_is_object CHECK (jsonb_typeof(group_acl) = 'object'), + ADD CONSTRAINT user_acl_is_object CHECK (jsonb_typeof(user_acl) = 'object'); diff --git a/coderd/database/migrations/testdata/fixtures/000416_pre_workspace_acl_object_constraint.up.sql b/coderd/database/migrations/testdata/fixtures/000416_pre_workspace_acl_object_constraint.up.sql new file mode 100644 index 0000000000..f7d9d23da6 --- /dev/null +++ b/coderd/database/migrations/testdata/fixtures/000416_pre_workspace_acl_object_constraint.up.sql @@ -0,0 +1,35 @@ +-- Fixture for migration 000417_workspace_acl_object_constraint. +-- Inserts a workspace with 'null'::json ACLs to ensure the migration +-- correctly normalizes such values. + +INSERT INTO workspaces ( + id, + created_at, + updated_at, + owner_id, + organization_id, + template_id, + deleted, + name, + last_used_at, + automatic_updates, + favorite, + group_acl, + user_acl +) +VALUES ( + '6f6fdbee-4c18-4a5c-8a8d-9b811c9f0a28', + '2024-02-10 00:00:00+00', + '2024-02-10 00:00:00+00', + '30095c71-380b-457a-8995-97b8ee6e5307', + 'bb640d07-ca8a-4869-b6bc-ae61ebb2fda1', + '4cc1f466-f326-477e-8762-9d0c6781fc56', + false, + 'acl-null-workspace', + '0001-01-01 00:00:00+00', + 'never', + false, + 'null'::jsonb, + 'null'::jsonb +) +ON CONFLICT DO NOTHING; diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 967907b507..4ebb12bf5c 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -6765,6 +6765,65 @@ func TestWorkspaceBuildDeadlineConstraint(t *testing.T) { } } +func TestWorkspaceACLObjectConstraint(t *testing.T) { + t.Parallel() + + db, _ := dbtestutil.NewDB(t) + org := dbgen.Organization(t, db, database.Organization{}) + user := dbgen.User(t, db, database.User{}) + template := dbgen.Template(t, db, database.Template{ + CreatedBy: user.ID, + OrganizationID: org.ID, + }) + workspace := dbgen.Workspace(t, db, database.WorkspaceTable{ + OwnerID: user.ID, + TemplateID: template.ID, + Deleted: false, + }) + + t.Run("GroupACLNull", func(t *testing.T) { + t.Parallel() + + var nilACL database.WorkspaceACL + + ctx := testutil.Context(t, testutil.WaitLong) + err := db.UpdateWorkspaceACLByID(ctx, database.UpdateWorkspaceACLByIDParams{ + ID: workspace.ID, + GroupACL: nilACL, + UserACL: database.WorkspaceACL{}, + }) + require.Error(t, err) + require.True(t, database.IsCheckViolation(err, database.CheckGroupAclIsObject)) + }) + + t.Run("UserACLNull", func(t *testing.T) { + t.Parallel() + + var nilACL database.WorkspaceACL + + ctx := testutil.Context(t, testutil.WaitLong) + err := db.UpdateWorkspaceACLByID(ctx, database.UpdateWorkspaceACLByIDParams{ + ID: workspace.ID, + GroupACL: database.WorkspaceACL{}, + UserACL: nilACL, + }) + require.Error(t, err) + require.True(t, database.IsCheckViolation(err, database.CheckUserAclIsObject)) + }) + + t.Run("ValidEmptyObjects", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + err := db.UpdateWorkspaceACLByID(ctx, database.UpdateWorkspaceACLByIDParams{ + ID: workspace.ID, + GroupACL: database.WorkspaceACL{}, + UserACL: database.WorkspaceACL{}, + }) + require.NoError(t, err) + }) +} + // TestGetLatestWorkspaceBuildsByWorkspaceIDs populates the database with // workspaces and builds. It then tests that // GetLatestWorkspaceBuildsByWorkspaceIDs returns the latest build for some