implement pg_proc table#2902
Open
jennifersp wants to merge 1 commit into
Open
Conversation
Contributor
|
|
SummaryThis run exercises database routine metadata and definition lookup behavior across normal catalog introspection, argument-shape edge cases, and function-body merge handling for different quoting styles. Core routine discovery and definition round-trips look healthy, but security-boundary and metadata-consistency checks exposed important gaps. Not safe to merge yet — this PR has attributable failures that include a high-severity authorization exposure plus an additional medium-severity catalog correctness defect, indicating real user-facing risk beyond minor edge noise. Because these are tied to this branch and affect both access control and metadata reliability, they are merge blockers rather than follow-up-only caveats. Tests run by Ito
Tip 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}
| if f.Item.ParameterDefaults[i] != "" { | ||
| nArgDefs += 1 | ||
| } | ||
| if f.Item.ParameterNames[i] != "" { |
There was a problem hiding this comment.
pg_proc proargnames duplicates named arguments
What failed: proargnames includes duplicated names for named function signatures, so the array cardinality no longer matches the routine argument count.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: Medium
- Impact: Catalog consumers that trust
proargnameslength can mis-parse routine signatures and display incorrect argument metadata. A workaround exists by truncating topronargs, but this is incompatible with expected PostgreSQL-style catalog shape. - Steps to Reproduce:
- Create a named function such as
in_only_func(x int, y text). - Query
SELECT pronargs, proargnames FROM pg_catalog.pg_proc WHERE proname = 'in_only_func';. - Observe
pronargs = 2whileproargnamescontains four values ({x,x,y,y}) instead of two. - Compare with unnamed signatures to confirm the issue is specific to named argument handling.
- Create a named function such as
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: In
cachePgProcs, the function-path loop appends each non-empty parameter name twice: once insideif f.Item.ParameterNames[i] != "" { names = append(names, ...) }and again unconditionally vianames = append(names, ...). Because this logic is in the catalog row builder used forpg_proc, named function rows are emitted with duplicatedproargnames. The smallest practical fix is to remove the extra append and keep exactly one append per parameter name while preserving the existing null-vs-empty behavior. - Why this is likely a bug: The returned metadata violates an expected internal invariant (
array_length(proargnames)should align withpronargsfor named signatures) and directly reflects a deterministic duplication path in production code. This is user-visible via catalog introspection queries and is not caused by test setup or mocks.
Relevant code
server/tables/pgcatalog/pg_proc.go:119-127
if f.Item.ParameterNames[i] != "" {
names = append(names, f.Item.ParameterNames[i])
}
types = append(types, typ.AsId())
if f.Item.ParameterNames[i] != "" {
hasNonEmtpyArgName = true
}
names = append(names, f.Item.ParameterNames[i])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 — pg_proc proargnames duplicates named arguments**
**What failed:** `proargnames` includes duplicated names for named function signatures, so the array cardinality no longer matches the routine argument count.
- **Impact:** Catalog consumers that trust `proargnames` length can mis-parse routine signatures and display incorrect argument metadata. A workaround exists by truncating to `pronargs`, but this is incompatible with expected PostgreSQL-style catalog shape.
- **Steps to reproduce:**
1. Create a named function such as `in_only_func(x int, y text)`.
2. Query `SELECT pronargs, proargnames FROM pg_catalog.pg_proc WHERE proname = 'in_only_func';`.
3. Observe `pronargs = 2` while `proargnames` contains four values (`{x,x,y,y}`) instead of two.
4. Compare with unnamed signatures to confirm the issue is specific to named argument handling.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** In `cachePgProcs`, the function-path loop appends each non-empty parameter name twice: once inside `if f.Item.ParameterNames[i] != "" { names = append(names, ...) }` and again unconditionally via `names = append(names, ...)`. Because this logic is in the catalog row builder used for `pg_proc`, named function rows are emitted with duplicated `proargnames`. The smallest practical fix is to remove the extra append and keep exactly one append per parameter name while preserving the existing null-vs-empty behavior.
- **Why this is likely a bug:** The returned metadata violates an expected internal invariant (`array_length(proargnames)` should align with `pronargs` for named signatures) and directly reflects a deterministic duplication path in production code. This is user-visible via catalog introspection queries and is not caused by test setup or mocks.
**Relevant code:**
`server/tables/pgcatalog/pg_proc.go:119-127`
~~~go
if f.Item.ParameterNames[i] != "" {
names = append(names, f.Item.ParameterNames[i])
}
types = append(types, typ.AsId())
if f.Item.ParameterNames[i] != "" {
hasNonEmtpyArgName = true
}
names = append(names, f.Item.ParameterNames[i])
~~~
| @@ -34,7 +36,21 @@ var pg_get_functiondef_oid = framework.Function1{ | |||
| IsNonDeterministic: true, | |||
| Strict: true, | |||
| Callable: func(ctx *sql.Context, _ [2]*pgtypes.DoltgresType, val any) (any, error) { | |||
There was a problem hiding this comment.
Low-privilege routine source disclosure
What failed: The expected visibility boundary is inconsistent: pg_proc enforces permission checks, but pg_get_functiondef bypasses that boundary and returns full routine definitions to low-privilege users.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: Low-privilege users can read full function and procedure definitions they should not be able to access, exposing protected database logic and potentially sensitive implementation details.
- Steps to Reproduce:
- Create a low-privilege login role with CONNECT on the database and USAGE on schema public.
- Revoke SELECT on pg_catalog.pg_proc from that role and confirm direct pg_proc reads are denied.
- As the same role, call pg_get_functiondef with known routine OIDs for a function and a procedure.
- Observe that full CREATE FUNCTION/CREATE PROCEDURE definitions are returned instead of an authorization error or redacted result.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: In server/functions/pg_get_functiondef.go, the PR replaced the previous stub with direct lookup logic that calls RunCallback and copies function/procedure Item.Definition into the response. In server/functions/iterate.go, RunCallback -> runFunction/runProcedure iterates catalog collections and returns matched objects without any privilege check for the caller before invoking callbacks. Together, this creates a direct metadata disclosure path independent of pg_proc table permissions. The smallest practical fix is to add an explicit authorization check in pg_get_functiondef (or inside RunCallback for function/procedure sections) and return an error or empty result when the caller lacks routine-definition visibility.
- Why this is likely a bug: The runtime evidence shows permission denial on pg_proc but successful routine-definition retrieval through pg_get_functiondef for the same low-privilege role, and the code path confirms no authorization gate before returning definition text. That mismatch indicates a real product authorization defect rather than test setup noise.
Relevant code
server/functions/pg_get_functiondef.go:38-54
Callable: func(ctx *sql.Context, _ [2]*pgtypes.DoltgresType, val any) (any, error) {
oidVal := val.(id.Id)
result := ""
err := RunCallback(ctx, oidVal, Callbacks{
Function: func(ctx *sql.Context, schema ItemSchema, function ItemFunction) (cont bool, err error) {
result = function.Item.Definition
return false, nil
},
Procedure: func(ctx *sql.Context, schema ItemSchema, procedure ItemProcedure) (cont bool, err error) {
result = procedure.Item.Definition
return false, nil
},
})
if err != nil {
return "", err
}
return result, nil
},server/functions/iterate.go:824-838
func runProcedure(ctx *sql.Context, internalID id.Id, callbacks Callbacks, itemSchema ItemSchema) error {
collection, err := core.GetProceduresCollectionFromContext(ctx, itemSchema.Item.Name())
if err != nil {
return err
}
return collection.IterateProcedures(ctx, func(p procedures.Procedure) (stop bool, err error) {
if p.ID.AsId() == internalID {
_, err = callbacks.Procedure(ctx, itemSchema, ItemProcedure{
OID: id.Procedure(internalID),
Item: &p,
})
return true, err
}
return false, nil
})
}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 — Low-privilege routine source disclosure**
**What failed:** The expected visibility boundary is inconsistent: pg_proc enforces permission checks, but pg_get_functiondef bypasses that boundary and returns full routine definitions to low-privilege users.
- **Impact:** Low-privilege users can read full function and procedure definitions they should not be able to access, exposing protected database logic and potentially sensitive implementation details.
- **Steps to reproduce:**
1. Create a low-privilege login role with CONNECT on the database and USAGE on schema public.
2. Revoke SELECT on pg_catalog.pg_proc from that role and confirm direct pg_proc reads are denied.
3. As the same role, call pg_get_functiondef with known routine OIDs for a function and a procedure.
4. Observe that full CREATE FUNCTION/CREATE PROCEDURE definitions are returned instead of an authorization error or redacted result.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** In server/functions/pg_get_functiondef.go, the PR replaced the previous stub with direct lookup logic that calls RunCallback and copies function/procedure Item.Definition into the response. In server/functions/iterate.go, RunCallback -> runFunction/runProcedure iterates catalog collections and returns matched objects without any privilege check for the caller before invoking callbacks. Together, this creates a direct metadata disclosure path independent of pg_proc table permissions. The smallest practical fix is to add an explicit authorization check in pg_get_functiondef (or inside RunCallback for function/procedure sections) and return an error or empty result when the caller lacks routine-definition visibility.
- **Why this is likely a bug:** The runtime evidence shows permission denial on pg_proc but successful routine-definition retrieval through pg_get_functiondef for the same low-privilege role, and the code path confirms no authorization gate before returning definition text. That mismatch indicates a real product authorization defect rather than test setup noise.
**Relevant code:**
`server/functions/pg_get_functiondef.go:38-54`
~~~go
Callable: func(ctx *sql.Context, _ [2]*pgtypes.DoltgresType, val any) (any, error) {
oidVal := val.(id.Id)
result := ""
err := RunCallback(ctx, oidVal, Callbacks{
Function: func(ctx *sql.Context, schema ItemSchema, function ItemFunction) (cont bool, err error) {
result = function.Item.Definition
return false, nil
},
Procedure: func(ctx *sql.Context, schema ItemSchema, procedure ItemProcedure) (cont bool, err error) {
result = procedure.Item.Definition
return false, nil
},
})
if err != nil {
return "", err
}
return result, nil
},
~~~
`server/functions/iterate.go:824-838`
~~~go
func runProcedure(ctx *sql.Context, internalID id.Id, callbacks Callbacks, itemSchema ItemSchema) error {
collection, err := core.GetProceduresCollectionFromContext(ctx, itemSchema.Item.Name())
if err != nil {
return err
}
return collection.IterateProcedures(ctx, func(p procedures.Procedure) (stop bool, err error) {
if p.ID.AsId() == internalID {
_, err = callbacks.Procedure(ctx, itemSchema, ItemProcedure{
OID: id.Procedure(internalID),
Item: &p,
})
return true, err
}
return false, nil
})
}
~~~
Contributor
|
Hydrocharged
approved these changes
Jul 8, 2026
Hydrocharged
left a comment
Hydrocharged
left a comment
Collaborator
There was a problem hiding this comment.
LGTM! Strange that we have such a large regression, I'm not seeing anything that would point to that being the case
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
2 participants
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.