[no-release-notes] Account for dolt dsess refactor#2809
Open
macneale4 wants to merge 1 commit into
Open
Conversation
Contributor
|
Ito Test Report ❌8 test cases ran. 1 failed, 2 additional findings, 5 passed. Across 8 test cases, 5 passed and 3 failed, showing that session/database resolution and provider lookup error propagation generally behaved correctly (including SHOW SCHEMAS fallback, OID helper short-circuiting, OID introspection consistency, and clean mixed-version startup abort diagnostics) but the run still identified significant product issues. The most important confirmed defects were a high-severity startup break from unresolved migrated doltcore/dsess imports against the pinned Dolt module version (introduced by this PR), a high-severity authorization leak where pg_catalog.pg_database exposes unauthorized database names, and a medium-severity regclass bug where search_path first-match precedence is reversed and can resolve to later-schema objects. ❌ Failed (1)
|
| Category | Summary | Screenshot |
|---|---|---|
| Catalog | Provider lookup errors are surfaced correctly; prior failure came from querying a valid DB context instead of the missing DB target. |  |
| Database | OID-backed introspection stayed consistent with pg_database visibility after correct in-database setup. |  |
| Provider | Mixed-version startup failed with explicit compatibility diagnostics and no partial running process. |  |
| Session | SHOW SCHEMAS succeeded for the connected database and returned user and system schemas. |  |
| Session | Invalid DOLTGRES_DB startup still used stale_missing_db, so CREATE SEQUENCE success did not indicate a provider lookup bug. |  |
ℹ️ Additional Findings (2)
These findings are unrelated to the current changes but were observed during testing.
| Category | Summary | Screenshot |
|---|---|---|
| Catalog | 🟠 regclass resolution can return a later-schema match instead of honoring first-match search_path precedence. |  |
| Database |  |
🟠 regclass search_path precedence is reversed
- What failed: The resolved relation tracks a later scanned schema instead of the first schema in search_path; expected behavior is first-match precedence.
- Impact: Metadata and object-resolution flows that rely on
regclasscan target the wrong object when names collide across schemas. Users can sometimes work around it by schema-qualifying names, but default introspection behavior is incorrect. - Steps to reproduce:
- Create the same relation name in two schemas, for example s1.shared_rel and s2.shared_rel.
- Set search_path to s1,s2 and run SELECT 'shared_rel'::regclass.
- Reverse search_path to s2,s1 and run the same cast again to compare the resolved relation/OID.
- Stub / mock context: Authentication checks were bypassed for this run by forcing
EnableAuthentication = falseinserver/authentication_scram.goso local SQL sessions could execute without SCRAM validation. - Code analysis: regclassin stops its callback when it finds a match, but iteration helpers collapse that stop into nil and the outer schema loop continues scanning. Because later callbacks can overwrite resultOid, first-match semantics are lost.
- Why this is likely a bug: The production iterator control flow makes a found-match stop signal non-terminal at schema scope, which directly explains the reversed precedence behavior.
Relevant code:
server/functions/regclass.go (lines 91-126)
err = IterateDatabase(ctx, database, Callbacks{
Table: func(ctx *sql.Context, schema ItemSchema, table ItemTable) (cont bool, err error) {
if table.Item.Name() == relationName {
resultOid = table.OID.AsId()
return false, nil
}
return true, nil
},
View: func(ctx *sql.Context, schema ItemSchema, view ItemView) (cont bool, err error) {
if view.Item.Name == relationName {
resultOid = view.OID.AsId()
return false, nil
}
return true, nil
},
SearchSchemas: searchSchemas,
})server/functions/iterate.go (lines 225-240)
err := iterateSequences(ctx, callbacks, sequenceMap, schema, itemSchema)
if err != nil {
return err
}
if callbacks.iteratesOverTables() {
tableNames, err := schema.GetTableNames(ctx)
if err != nil {
return err
}
if err = iterateTables(ctx, callbacks, itemSchema, tableNames); err != nil {
return err
}
}server/functions/iterate.go (lines 367-371)
if callbacks.Table != nil {
if cont, err := callbacks.Table(ctx, itemSchema, itemTable); err != nil {
return err
} else if !cont {
return nil
}
}⚠️ Catalog enumeration leaks unauthorized database names
- What failed: The catalog query exposes database names that should be hidden by database-level authorization; expected behavior is that unauthorized databases are excluded.
- Impact: Users with constrained access can enumerate database names outside their allowed scope, leaking metadata across tenants/projects. This weakens authorization boundaries for a core catalog endpoint and can aid further targeted probing.
- Steps to reproduce:
- Create one in-scope database and one out-of-scope database, then revoke CONNECT on the out-of-scope database for a limited role.
- Connect as the limited role and run SELECT datname FROM pg_catalog.pg_database.
- Observe that the out-of-scope database name is still returned in the result set.
- Stub / mock context: SCRAM sign-in was bypassed to allow deterministic local role sessions, but the leakage check itself used real database role grants/revokes and a real pg_database query path with no mocked catalog output.
- Code analysis: I inspected the pg_database table handler and authorization handler paths. The row iterator enumerates all catalog databases from the provider and only filters system DB names, with no per-user privilege check, and the database authorization hook is still a TODO returning nil.
- Why this is likely a bug: Production code currently builds the pg_database result set without enforcing database authorization, so unauthorized names can be returned by design.
Relevant code:
server/tables/pgcatalog/pg_database.go (lines 49-62)
func (p PgDatabaseHandler) RowIter(ctx *sql.Context, partition sql.Partition) (sql.RowIter, error) {
// TODO: Should the catalog be passed to RowIter like it is for the information_schema tables RowIter?
doltSession := dsess.DSessFromSess(ctx.Session)
c := sqle.NewDefault(doltSession.GenericProvider()).Analyzer.Catalog
databases := c.AllDatabases(ctx)
dbs := make([]sql.Database, 0, len(databases))
for _, db := range databases {
name := db.Name()
if name == "information_schema" || name == "pg_catalog" || name == "performance_schema" {
continue
}
dbs = append(dbs, db)server/auth/auth_handler.go (lines 248-257)
func (h *AuthorizationHandler) CheckDatabase(ctx *sql.Context, aqs sql.AuthorizationQueryState, dbName string) error {
if aqs == nil {
aqs = h.NewQueryState(ctx)
}
state := aqs.(AuthorizationQueryState)
if state.err != nil {
return state.err
}
// TODO: implement this
return nil
}Commit: 33854d6
Tell us how we did: Give Ito Feedback
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.
Depends on: dolthub/dolt#11162