more built in functions to support clients - #3102
Open
jennifersp wants to merge 1 commit into
Open
Conversation
Contributor
Footnotes
|
|
SummaryCoverage spans database compatibility and catalog behavior, session-setting boundaries, startup and concurrent connections, normal metadata and privilege flows, and adversarial authorization cases involving denied or nonexistent resources. Most ordinary compatibility paths behave correctly, but the new permission and ACL-related behaviors include serious correctness and security failures. Not safe to merge yet — this PR introduces a high-severity authorization failure that can approve access for users without permission, along with medium-severity ACL lookup failures that break consumers of the new compatibility functions. An unrelated locale-display compatibility issue is a separate flag-for-later caveat, not a driver of the merge decision. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 SHOW cannot read locale settings
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
![itoqa[bot]](https://avatars.githubusercontent.com/in/2622614?s=60&v=4){kind=small}
| } | ||
|
|
||
| // has_table_privilege_name_text_text represents the PostgreSQL function of the same name, taking the same parameters. | ||
| var has_table_privilege_name_text_text = framework.Function3{ |
There was a problem hiding this comment.
Privilege checks approve users without access
What failed: All six privilege checks returned true for denied and nonexistent inputs, even though the independent protected read was rejected.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: Clients that rely on these permission checks may let unauthorized users access protected tables or actions. The checks also report access for missing users and tables, making authorization decisions unsafe.
- Steps to Reproduce:
- Create or use a non-admin role with no privilege on a protected table.
- Call each supported has_table_privilege signature for that role, the protected table, and SELECT.
- Repeat the calls with a nonexistent role or table.
- Compare the returned booleans with an independent protected table read as the non-admin role.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: server/functions/has_table_privilege.go registers six overloads: name,text,text at lines 35-44, name,oid,text at lines 47-56, oid,text,text at lines 59-68, oid,oid,text at lines 71-80, text,text at lines 83-92, and oid,text at lines 95-104. Every Callable ignores its input values and returns the literal true with no lookup, role check, table check, or privilege check. The TODO comments in each implementation explicitly leave the authorization work undone. By contrast, server/auth/table_privileges.go:63-92 implements HasTablePrivilege: it grants superusers, checks schema-wide and table-specific grants, checks group membership, and returns false at line 91 when no matching grant exists. The new SQL functions do not call that helper or any equivalent authorization path, so their results cannot represent the application's actual permission state. The smallest practical fix is to resolve each overload's role and table arguments, map the requested privilege, and delegate to auth.HasTablePrivilege, returning false when no grant exists; until that is implemented, these probes should not be exposed as authorization answers.
- Why this is likely a bug: The function name and PostgreSQL-compatible signatures promise a permission query, but every input produces true, including nonexistent principals and objects. The recorded probe returned true for all six overloads in both denied and nonexistent cases, while the independent protected operation was rejected, confirming that the function result is disconnected from enforcement. This is a security-relevant contract failure for any client that uses has_table_privilege as a gate. The PR directly introduced the six unconditional implementations, so the targeted remediation is to replace those returns with calls into the existing privilege evaluation path or to fail clearly until that path is wired in.
Relevant code
server/functions/has_table_privilege.go:35-44
var has_table_privilege_name_text_text = framework.Function3{
Name: "has_table_privilege",
Return: pgtypes.Bool,
Callable: func(ctx *sql.Context, _ [4]*pgtypes.DoltgresType, val1, val2, val3 any) (any, error) {
return true, nil
},
}server/functions/has_table_privilege.go:47-104
The remaining five registered overloads have the same Callable shape and each returns the literal true: lines 52-54, 64-66, 76-78, 88-90, and 100-102.server/auth/table_privileges.go:63-92
func HasTablePrivilege(key TablePrivilegeKey, privilege Privilege) bool {
if IsSuperUser(key.Role) {
return true
}
...
return false
}server/functions/init.go:124-129
initHasDatabasePrivilege()
initHasSchemaPrivilege()
initHasTablePrivilege()Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**High severity — Privilege checks approve users without access**
**What failed:** All six privilege checks returned true for denied and nonexistent inputs, even though the independent protected read was rejected.
- **Impact:** Clients that rely on these permission checks may let unauthorized users access protected tables or actions. The checks also report access for missing users and tables, making authorization decisions unsafe.
- **Steps to reproduce:**
1. Create or use a non-admin role with no privilege on a protected table.
2. Call each supported has_table_privilege signature for that role, the protected table, and SELECT.
3. Repeat the calls with a nonexistent role or table.
4. Compare the returned booleans with an independent protected table read as the non-admin role.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** server/functions/has_table_privilege.go registers six overloads: name,text,text at lines 35-44, name,oid,text at lines 47-56, oid,text,text at lines 59-68, oid,oid,text at lines 71-80, text,text at lines 83-92, and oid,text at lines 95-104. Every Callable ignores its input values and returns the literal true with no lookup, role check, table check, or privilege check. The TODO comments in each implementation explicitly leave the authorization work undone. By contrast, server/auth/table_privileges.go:63-92 implements HasTablePrivilege: it grants superusers, checks schema-wide and table-specific grants, checks group membership, and returns false at line 91 when no matching grant exists. The new SQL functions do not call that helper or any equivalent authorization path, so their results cannot represent the application's actual permission state. The smallest practical fix is to resolve each overload's role and table arguments, map the requested privilege, and delegate to auth.HasTablePrivilege, returning false when no grant exists; until that is implemented, these probes should not be exposed as authorization answers.
- **Why this is likely a bug:** The function name and PostgreSQL-compatible signatures promise a permission query, but every input produces true, including nonexistent principals and objects. The recorded probe returned true for all six overloads in both denied and nonexistent cases, while the independent protected operation was rejected, confirming that the function result is disconnected from enforcement. This is a security-relevant contract failure for any client that uses has_table_privilege as a gate. The PR directly introduced the six unconditional implementations, so the targeted remediation is to replace those returns with calls into the existing privilege evaluation path or to fail clearly until that path is wired in.
**Relevant code:**
`server/functions/has_table_privilege.go:35-44`
~~~go
var has_table_privilege_name_text_text = framework.Function3{
Name: "has_table_privilege",
Return: pgtypes.Bool,
Callable: func(ctx *sql.Context, _ [4]*pgtypes.DoltgresType, val1, val2, val3 any) (any, error) {
return true, nil
},
}
~~~
`server/functions/has_table_privilege.go:47-104`
~~~go
The remaining five registered overloads have the same Callable shape and each returns the literal true: lines 52-54, 64-66, 76-78, 88-90, and 100-102.
~~~
`server/auth/table_privileges.go:63-92`
~~~go
func HasTablePrivilege(key TablePrivilegeKey, privilege Privilege) bool {
if IsSuperUser(key.Role) {
return true
}
...
return false
}
~~~
`server/functions/init.go:124-129`
~~~go
initHasDatabasePrivilege()
initHasSchemaPrivilege()
initHasTablePrivilege()
~~~
| const aclexplodeName = "aclexplode" | ||
|
|
||
| // aclexplode represents the PostgreSQL function of the same name, taking the same parameters. | ||
| var aclexplode = framework.Function1{ |
There was a problem hiding this comment.
Permission data fails during concurrent startup
What failed: Each concurrent client failed with an error saying it could not find field 1 in a row with 1 column. The same failure was reproducible after the concurrent work finished.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Clients that read permission data through the compatibility function receive an error instead of the ACL rows they need. Other database functions continue to work, so the failure is limited to workflows that use this permission lookup.
- Steps to Reproduce:
- Start a fresh local server.
- Open eight independent PostgreSQL client connections at the same time.
- Run SELECT * FROM aclexplode(ARRAY['x']::text[]) in each connection.
- Check the result from each client and then run the same query again after the concurrent calls finish.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: The PR adds aclexplode as a framework.Function1 in server/functions/aclexplode.go:33-42. It declares Return: pgtypes.Record and OutParams with four non-null columns named grantor, grantee, privilege_type, and is_grantable at lines 44-50, so the SQL layer must materialize rows matching that schema. However, the Callable at lines 38-40 unconditionally returns nil, nil for a non-NULL input. The recorded query uses a non-NULL text array, so strictness does not short-circuit the call; the framework attempts to read record fields from the nil result and raises the missing-field error. server/functions/init.go:76 adds initAclexplode to the normal function registration sequence. The shared initialization path is protected by sync.Once in server/initialization/initialization.go:48-78, and the eight clients consistently resolved neighboring functions, which rules out concurrent initialization as the cause. The earlier pg_proc count of zero is not evidence of a race because server/tables/pgcatalog/pg_proc.go:94-96 explicitly leaves built-in function enumeration unimplemented. The smallest practical fix is to implement the ACL conversion and return rows with the four declared fields, or, if this compatibility function is intentionally still a stub, return a framework-supported typed empty row iterator or a deliberate unsupported-function error rather than nil.
- Why this is likely a bug: The failure is deterministic across eight independent sessions and on serial readback, while length, pg_get_keywords, has_table_privilege, and pg_blocking_pids continue to work. That pattern matches the new function's invalid record result, not a timing-dependent catalog race. A client cannot consume the advertised ACL result at all, and the source contains an explicit TODO acknowledging that the function is incomplete; replacing the nil result with a typed empty result or implementing the four-field rows would prevent the hard execution error without changing unrelated initialization code.
Relevant code
server/functions/aclexplode.go:33-41
var aclexplode = framework.Function1{
Name: aclexplodeName,
Return: pgtypes.Record,
Parameters: [1]*pgtypes.DoltgresType{pgtypes.TextArray},
Strict: true,
Callable: func(ctx *sql.Context, _ [2]*pgtypes.DoltgresType, val any) (any, error) {
return nil, nil
},server/functions/aclexplode.go:44-50
var aclexplodeOutArgs = sql.Schema{
{Name: "grantor", Type: pgtypes.Oid, Default: nil, Nullable: false, Source: aclexplodeName},
{Name: "grantee", Type: pgtypes.Oid, Default: nil, Nullable: false, Source: aclexplodeName},
{Name: "privilege_type", Type: pgtypes.Text, Default: nil, Nullable: false, Source: aclexplodeName},
{Name: "is_grantable", Type: pgtypes.Bool, Default: nil, Nullable: false, Source: aclexplodeName},
}server/initialization/initialization.go:48-63
var once = &sync.Once{}
func Initialize(...) {
once.Do(func() {
...
config.Init()
...
functions.Init()Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — Permission data fails during concurrent startup**
**What failed:** Each concurrent client failed with an error saying it could not find field 1 in a row with 1 column. The same failure was reproducible after the concurrent work finished.
- **Impact:** Clients that read permission data through the compatibility function receive an error instead of the ACL rows they need. Other database functions continue to work, so the failure is limited to workflows that use this permission lookup.
- **Steps to reproduce:**
1. Start a fresh local server.
2. Open eight independent PostgreSQL client connections at the same time.
3. Run SELECT * FROM aclexplode(ARRAY['x']::text[]) in each connection.
4. Check the result from each client and then run the same query again after the concurrent calls finish.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** The PR adds aclexplode as a framework.Function1 in server/functions/aclexplode.go:33-42. It declares Return: pgtypes.Record and OutParams with four non-null columns named grantor, grantee, privilege_type, and is_grantable at lines 44-50, so the SQL layer must materialize rows matching that schema. However, the Callable at lines 38-40 unconditionally returns nil, nil for a non-NULL input. The recorded query uses a non-NULL text array, so strictness does not short-circuit the call; the framework attempts to read record fields from the nil result and raises the missing-field error. server/functions/init.go:76 adds initAclexplode to the normal function registration sequence. The shared initialization path is protected by sync.Once in server/initialization/initialization.go:48-78, and the eight clients consistently resolved neighboring functions, which rules out concurrent initialization as the cause. The earlier pg_proc count of zero is not evidence of a race because server/tables/pgcatalog/pg_proc.go:94-96 explicitly leaves built-in function enumeration unimplemented. The smallest practical fix is to implement the ACL conversion and return rows with the four declared fields, or, if this compatibility function is intentionally still a stub, return a framework-supported typed empty row iterator or a deliberate unsupported-function error rather than nil.
- **Why this is likely a bug:** The failure is deterministic across eight independent sessions and on serial readback, while length, pg_get_keywords, has_table_privilege, and pg_blocking_pids continue to work. That pattern matches the new function's invalid record result, not a timing-dependent catalog race. A client cannot consume the advertised ACL result at all, and the source contains an explicit TODO acknowledging that the function is incomplete; replacing the nil result with a typed empty result or implementing the four-field rows would prevent the hard execution error without changing unrelated initialization code.
**Relevant code:**
`server/functions/aclexplode.go:33-41`
~~~go
var aclexplode = framework.Function1{
Name: aclexplodeName,
Return: pgtypes.Record,
Parameters: [1]*pgtypes.DoltgresType{pgtypes.TextArray},
Strict: true,
Callable: func(ctx *sql.Context, _ [2]*pgtypes.DoltgresType, val any) (any, error) {
return nil, nil
},
~~~
`server/functions/aclexplode.go:44-50`
~~~go
var aclexplodeOutArgs = sql.Schema{
{Name: "grantor", Type: pgtypes.Oid, Default: nil, Nullable: false, Source: aclexplodeName},
{Name: "grantee", Type: pgtypes.Oid, Default: nil, Nullable: false, Source: aclexplodeName},
{Name: "privilege_type", Type: pgtypes.Text, Default: nil, Nullable: false, Source: aclexplodeName},
{Name: "is_grantable", Type: pgtypes.Bool, Default: nil, Nullable: false, Source: aclexplodeName},
}
~~~
`server/initialization/initialization.go:48-63`
~~~go
var once = &sync.Once{}
func Initialize(...) {
once.Do(func() {
...
config.Init()
...
functions.Init()
~~~
| const aclexplodeName = "aclexplode" | ||
|
|
||
| // aclexplode represents the PostgreSQL function of the same name, taking the same parameters. | ||
| var aclexplode = framework.Function1{ |
There was a problem hiding this comment.
ACL catalog call crashes on a valid array
What failed: The ACL catalog function is listed as available, but a valid call fails before it can return an empty result or ACL rows.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Applications that call the ACL catalog function with valid input receive a database error instead of ACL rows or an empty result. Other tested built-in functions continue to work, so the impact is limited to this catalog function and its callers.
- Steps to Reproduce:
- Start a fresh local server and connect to the postgres database with a PostgreSQL client.
- Call aclexplode with a non-NULL text array, such as SELECT * FROM aclexplode(ARRAY[]::text[]).
- Observe the error: unable to find field with index 1 in row of 1 columns.
- Call length('catalog'), pg_get_keywords(), or pg_blocking_pids(pg_backend_pid()) in the same fresh catalog to confirm that initialization and the neighboring compatibility functions still work.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: server/functions/aclexplode.go:33-42 declares aclexplode as framework.Function1 with Return set to pgtypes.Record, a text-array parameter, Strict=true, and OutParams set to aclexplodeOutArgs. The output schema at lines 45-50 contains four non-null fields: grantor (oid), grantee (oid), privilege_type (text), and is_grantable (bool). However, the Callable at lines 38-40 returns nil, nil for every non-NULL value. The function framework therefore receives a successful nil result while the executor still expects a SETOF record with four fields; materializing that result produces the observed missing field index error. This is distinct from startup ordering: server/initialization/initialization.go:52-78 uses sync.Once and calls functions.Init at line 62 before framework.Initialize at line 72, and the neighboring functions resolve in the same fresh session. A targeted fix is to make the callable return the framework's typed empty set-returning iterator for the current stub behavior, or return correctly shaped ACL rows after implementing the intended logic; it should not return an untyped nil for a declared record result.
- Why this is likely a bug: The failure is deterministic for a normal non-NULL SQL input and is reported by the database executor as an internal bug, not as an unsupported input or a clean compatibility limitation. The source shows the exact mismatch: a four-column record result is declared, while the callable supplies no row shape at all. The same fresh session successfully calls length, has_table_privilege, pg_get_keywords, and pg_blocking_pids, which rules out a general startup or catalog-registration failure. Because aclexplode was added by this PR and the bad return value is in that added file, the PR is the direct cause. Returning a typed empty result would preserve the current stub semantics with the smallest change; implementing ACL expansion is a larger follow-up only if real ACL rows are required.
Relevant code
server/functions/aclexplode.go:33-42
var aclexplode = framework.Function1{
Name: aclexplodeName,
Return: pgtypes.Record,
Parameters: [1]*pgtypes.DoltgresType{pgtypes.TextArray},
Strict: true,
Callable: func(ctx *sql.Context, _ [2]*pgtypes.DoltgresType, val any) (any, error) {
return nil, nil
},
OutParams: aclexplodeOutArgs,
}server/functions/aclexplode.go:44-50
var aclexplodeOutArgs = sql.Schema{
{Name: "grantor", Type: pgtypes.Oid, Default: nil, Nullable: false, Source: aclexplodeName},
{Name: "grantee", Type: pgtypes.Oid, Default: nil, Nullable: false, Source: aclexplodeName},
{Name: "privilege_type", Type: pgtypes.Text, Default: nil, Nullable: false, Source: aclexplodeName},
{Name: "is_grantable", Type: pgtypes.Bool, Default: nil, Nullable: false, Source: aclexplodeName},
}server/initialization/initialization.go:50-72
func Initialize(dEnv *env.DoltEnv, cfg *doltgresservercfg.DoltgresConfig) {
once.Do(func() {
core.Init()
...
functions.Init()
...
framework.Initialize(ast.Convert)
})
}Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**Medium severity — ACL catalog call crashes on a valid array**
**What failed:** The ACL catalog function is listed as available, but a valid call fails before it can return an empty result or ACL rows.
- **Impact:** Applications that call the ACL catalog function with valid input receive a database error instead of ACL rows or an empty result. Other tested built-in functions continue to work, so the impact is limited to this catalog function and its callers.
- **Steps to reproduce:**
1. Start a fresh local server and connect to the postgres database with a PostgreSQL client.
2. Call aclexplode with a non-NULL text array, such as SELECT * FROM aclexplode(ARRAY[]::text[]).
3. Observe the error: unable to find field with index 1 in row of 1 columns.
4. Call length('catalog'), pg_get_keywords(), or pg_blocking_pids(pg_backend_pid()) in the same fresh catalog to confirm that initialization and the neighboring compatibility functions still work.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** server/functions/aclexplode.go:33-42 declares aclexplode as framework.Function1 with Return set to pgtypes.Record, a text-array parameter, Strict=true, and OutParams set to aclexplodeOutArgs. The output schema at lines 45-50 contains four non-null fields: grantor (oid), grantee (oid), privilege_type (text), and is_grantable (bool). However, the Callable at lines 38-40 returns nil, nil for every non-NULL value. The function framework therefore receives a successful nil result while the executor still expects a SETOF record with four fields; materializing that result produces the observed missing field index error. This is distinct from startup ordering: server/initialization/initialization.go:52-78 uses sync.Once and calls functions.Init at line 62 before framework.Initialize at line 72, and the neighboring functions resolve in the same fresh session. A targeted fix is to make the callable return the framework's typed empty set-returning iterator for the current stub behavior, or return correctly shaped ACL rows after implementing the intended logic; it should not return an untyped nil for a declared record result.
- **Why this is likely a bug:** The failure is deterministic for a normal non-NULL SQL input and is reported by the database executor as an internal bug, not as an unsupported input or a clean compatibility limitation. The source shows the exact mismatch: a four-column record result is declared, while the callable supplies no row shape at all. The same fresh session successfully calls length, has_table_privilege, pg_get_keywords, and pg_blocking_pids, which rules out a general startup or catalog-registration failure. Because aclexplode was added by this PR and the bad return value is in that added file, the PR is the direct cause. Returning a typed empty result would preserve the current stub semantics with the smallest change; implementing ACL expansion is a larger follow-up only if real ACL rows are required.
**Relevant code:**
`server/functions/aclexplode.go:33-42`
~~~go
var aclexplode = framework.Function1{
Name: aclexplodeName,
Return: pgtypes.Record,
Parameters: [1]*pgtypes.DoltgresType{pgtypes.TextArray},
Strict: true,
Callable: func(ctx *sql.Context, _ [2]*pgtypes.DoltgresType, val any) (any, error) {
return nil, nil
},
OutParams: aclexplodeOutArgs,
}
~~~
`server/functions/aclexplode.go:44-50`
~~~go
var aclexplodeOutArgs = sql.Schema{
{Name: "grantor", Type: pgtypes.Oid, Default: nil, Nullable: false, Source: aclexplodeName},
{Name: "grantee", Type: pgtypes.Oid, Default: nil, Nullable: false, Source: aclexplodeName},
{Name: "privilege_type", Type: pgtypes.Text, Default: nil, Nullable: false, Source: aclexplodeName},
{Name: "is_grantable", Type: pgtypes.Bool, Default: nil, Nullable: false, Source: aclexplodeName},
}
~~~
`server/initialization/initialization.go:50-72`
~~~go
func Initialize(dEnv *env.DoltEnv, cfg *doltgresservercfg.DoltgresConfig) {
once.Do(func() {
core.Init()
...
functions.Init()
...
framework.Initialize(ast.Convert)
})
}
~~~
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
1 participant
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.
No description provided.