Skip to content

Fix drop_label privilege check for non-superusers (unrecognized class ID) - #2497

Open
eeshwarg wants to merge 1 commit into
apache:masterfrom
eeshwarg:fix/drop-label-object-ownercheck
Open

Fix drop_label privilege check for non-superusers (unrecognized class ID)#2497
eeshwarg wants to merge 1 commit into
apache:masterfrom
eeshwarg:fix/drop-label-object-ownercheck

Conversation

@eeshwarg

@eeshwarg eeshwarg commented Aug 4, 2026

Copy link
Copy Markdown

Problem

Any non-superuser calling drop_label('graph', 'label') (or dropping a vertex/edge label) fails with:

ERROR:  unrecognized class ID: <oid>

Superusers are unaffected.

Root cause

In src/backend/commands/label_commands.c, range_var_callback_for_remove_relation() calls the PG16+ object_ownercheck() with the wrong argument order. The API is:

bool object_ownercheck(Oid classid, Oid objectid, Oid roleid);

AGE passed rel_oid as classid (should be RelationRelationId) and a namespace OID as objectid (should be rel_oid). Since classid isn't a real catalog OID, the lookup hits the default case in get_object_property and raises unrecognized class ID. Superusers escape because object_ownercheck early-returns via superuser_arg(roleid) before the classid lookup.

This was introduced in the PG16 port: before PG16 the code used the correct 2-arg pg_class_ownercheck(rel_oid, GetUserId()). The same defect is present on master, PG16, PG17, and PG18; PG15 and earlier are unaffected. This PR fixes master.

Fix

-    if (!object_ownercheck(rel_oid, get_rel_namespace(rel_oid), GetUserId()))
+    if (!object_ownercheck(RelationRelationId, rel_oid, GetUserId()))

This matches upstream PostgreSQL's own RangeVarCallbackForDropRelation usage. RelationRelationId is already available via existing includes (catalog/pg_class_d.h); no new include needed.

Testing

  • Added a regression test in regress/sql/security.sql: a NOSUPERUSER role creates (and therefore owns) a graph and label, then successfully runs drop_label. Verified it fails with ERROR: unrecognized class ID on the unpatched build and passes after the fix.
  • make installcheck full suite: 42/42 tests pass (PostgreSQL 18).

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

range_var_callback_for_remove_relation() called object_ownercheck() with
the wrong argument order for the PG16+ API
object_ownercheck(classid, objectid, roleid). It passed rel_oid as the
classid and a namespace OID as the objectid, so the classid lookup hit
the default case and raised "unrecognized class ID" for any
non-superuser dropping a graph label. Superusers were unaffected because
object_ownercheck() early-returns via superuser_arg().

Pass RelationRelationId as the classid and rel_oid as the objectid,
matching upstream PostgreSQL's RangeVarCallbackForDropRelation.

Add a regression test in security.sql covering a NOSUPERUSER role that
owns a graph/label and successfully runs drop_label.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@jrgemignani
jrgemignani requested review from MuhammadTahaNaveed and gregfelice and a lite review from Copilot August 4, 2026 18:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Fixes a PostgreSQL 16+ ownership check bug that caused non-superusers to fail drop_label() (and related label drops) with ERROR: unrecognized class ID, by passing the correct (classid, objectid, roleid) arguments to object_ownercheck() and adding a regression test to prevent regressions.

Changes:

  • Corrected object_ownercheck() argument order in range_var_callback_for_remove_relation().
  • Added a non-superuser regression scenario covering create graph/label and successful drop_label.
  • Updated regression expected output accordingly.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/backend/commands/label_commands.c Fixes ownership check by calling object_ownercheck(RelationRelationId, rel_oid, GetUserId()).
regress/sql/security.sql Adds regression test ensuring non-superuser owners can successfully drop_label().
regress/expected/security.out Captures expected output for the new regression test block.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread regress/sql/security.sql
Comment on lines +1468 to +1469
SELECT format('GRANT CREATE ON DATABASE %I TO age_nonsuper', current_database())
\gexec

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

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.

2 participants