Skip to content

[auto-bump] [no-release-notes] dependency by tbantle22#2711

Closed
coffeegoddd wants to merge 1 commit into
mainfrom
tbantle22-01cf2f95
Closed

[auto-bump] [no-release-notes] dependency by tbantle22#2711
coffeegoddd wants to merge 1 commit into
mainfrom
tbantle22-01cf2f95

Conversation

@coffeegoddd
Copy link
Copy Markdown
Contributor

An Automated Dependency Version Bump PR 👑

Initial Changes

The changes contained in this PR were produced by `go get`ing the dependency.

```bash
go get github.com/dolthub/[dependency]/go@[commit]
```

@github-actions
Copy link
Copy Markdown
Contributor

Main PR
covering_index_scan_postgres 1345.14/s 1305.37/s -3.0%
index_join_postgres 198.91/s 198.59/s -0.2%
index_join_scan_postgres 209.59/s 209.42/s -0.1%
index_scan_postgres 12.18/s 12.18/s 0.0%
oltp_point_select 2362.46/s 2350.93/s -0.5%
oltp_read_only 1888.18/s 1891.58/s +0.1%
select_random_points 132.62/s 131.76/s -0.7%
select_random_ranges 1089.25/s 1079.33/s -1.0%
table_scan_postgres 11.87/s 11.95/s +0.6%
types_table_scan_postgres 5.47/s 5.46/s -0.2%

@itoqa
Copy link
Copy Markdown

itoqa Bot commented May 13, 2026

Ito Test Report ✅

10 test cases ran. 2 additional findings, 8 passed.

Across 10 executed test cases, 8 passed and 2 failed, confirming successful bootstrap startup/query readiness, TLS pgwire handshake behavior (including database fallback to username), authorization enforcement, adapter initialization, and key replication happy paths including restart checkpoint recovery. The major issues were both in logical replication: a High-severity SQL apply-construction flaw with unquoted identifiers and malformed predicates that can produce unsafe or broken statements, and a Critical crash path where out-of-order DML with unknown relation IDs triggers log.Fatalf and terminates the replica; several runs bypassed SCRAM authentication, so these results validate functional paths rather than credential enforcement.

✅ Passed (8)
Category Summary Screenshot
Adapter First SELECT * FROM dolt.status after restart succeeded with expected adapted columns. N/A
Bootstrap Fresh startup created postgres and SELECT 1 succeeded via psql. BOOTSTRAP-1
Handshake Configured doltgres with TLS cert/key, connected with SSL-required pgwire client, and verified authenticated usable session by querying current_user successfully. N/A
Handshake Using a raw pgwire startup packet that omitted the database parameter, connection succeeded over TLS and subsequent query confirmed usable session with current_database() resolving to postgres. N/A
Permissions Unauthorized table SELECT was correctly denied for a role without SELECT privileges. PERMISSIONS-1
Permissions Execute after privilege revocation was correctly denied in the prepared-statement flow. PERMISSIONS-2
Replication Replica applied INSERT/UPDATE/DELETE correctly (row 301 updated, row 101 deleted). REPLICATION-1
Replication After forced stop and restart, replay resumed cleanly with complete row range 500-520. REPLICATION-4
ℹ️ Additional Findings (2)

These findings are unrelated to the current changes but were observed during testing.

Category Summary Screenshot
Replication ⚠️ Replication apply builds SQL from unquoted identifiers and malformed predicates, causing unsafe/malformed statements. REPLICATION-2
Replication 🚨 Out-of-order DML for unknown relation ID calls log.Fatalf and terminates the replica process. REPLICATION-3
⚠️ Replication apply SQL builder is unsafe
  • What failed: The apply path interpolates relation and column identifiers directly into SQL and contains malformed predicate assembly, so malformed/adversarial metadata can produce unsafe or syntactically broken statements instead of a clean safe reject.
  • Impact: A crafted replication stream can trigger malformed or unintended SQL in the apply path, creating a security and data integrity risk on replicas. Replication can also enter retry/error loops instead of cleanly rejecting invalid metadata.
  • Steps to reproduce:
    1. Configure logical replication between primary and replica with a controlled publication.
    2. Send crafted relation and column identifiers through replication metadata.
    3. Observe generated apply SQL behavior and replica error handling.
  • Stub / mock context: SCRAM authentication was intentionally bypassed for local connections in server/authentication_scram.go to keep replication setup deterministic, and the runtime image used the prebuilt doltgres binary path during this run.
  • Code analysis: I inspected the replication apply implementation in server/logrepl/replication.go and found raw identifier interpolation in INSERT/UPDATE/DELETE query builders plus predicate-construction defects (updateStr separator written in the WHERE branch and comma-joined delete predicates).
  • Why this is likely a bug: The production apply code composes executable SQL from unquoted replication identifiers and inconsistent predicate builders, which directly explains syntax-error behavior and unsafe statement-construction risk without relying on test-only assumptions.

Relevant code:

server/logrepl/replication.go (lines 613-637)

colName := rel.Columns[idx].Name
columnStr.WriteString(colName)

switch col.DataType {
case 'n': // null
    valuesStr.WriteString("NULL")
case 't': // text

    // We have to round-trip the data through the encodings to get an accurate text rep back
    val, err := decodeTextColumnData(state.typeMap, col.Data, rel.Columns[idx].DataType)
    if err != nil {
        log.Fatalln("error decoding column data:", err)
    }
    colData, err := encodeColumnData(state.typeMap, val, rel.Columns[idx].DataType)
    if err != nil {
        return false, err
    }
    valuesStr.WriteString(colData)
}
err = r.replicateQuery(state.replicaConn, fmt.Sprintf("INSERT INTO %s.%s (%s) VALUES (%s)", rel.Namespace, rel.RelationName, columnStr.String(), valuesStr.String()))

server/logrepl/replication.go (lines 678-692)

// TODO: quote column names?
if colFlags == 0 {
    if updateStr.Len() > 0 {
        updateStr.WriteString(", ")
    }
    updateStr.WriteString(fmt.Sprintf("%s = %v", colName, stringVal))
} else {
    if whereStr.Len() > 0 {
        updateStr.WriteString(", ")
    }
    whereStr.WriteString(fmt.Sprintf("%s = %v", colName, stringVal))
}
err = r.replicateQuery(state.replicaConn, fmt.Sprintf("UPDATE %s.%s SET %s%s", rel.Namespace, rel.RelationName, updateStr.String(), whereClause(whereStr)))

server/logrepl/replication.go (lines 735-742)

if whereStr.Len() > 0 {
    whereStr.WriteString(", ")
}
whereStr.WriteString(fmt.Sprintf("%s = %v", colName, stringVal))

err = r.replicateQuery(state.replicaConn, fmt.Sprintf("DELETE FROM %s.%s WHERE %s", rel.Namespace, rel.RelationName, whereStr.String()))
🚨 Out-of-order replication DML crashes process
  • What failed: Unknown relation IDs in DML paths trigger log.Fatalf, which terminates the process rather than rejecting/quarantining the bad stream segment and continuing service.
  • Impact: A single out-of-order replication message can crash the replica process and interrupt availability until manual restart. This makes replication fragile under protocol disorder or malicious upstream input.
  • Steps to reproduce:
    1. Connect replication to a controllable stream source.
    2. Deliver INSERT, UPDATE, or DELETE frames that reference an unseen relation ID before the relation message.
    3. Observe process behavior and confirm whether the replica stays available and can recover.
  • Stub / mock context: SCRAM authentication was intentionally bypassed for local connections in server/authentication_scram.go to keep replication setup deterministic, and the runtime image used the prebuilt doltgres binary path during this run.
  • Code analysis: I reviewed the insert, update, and delete replication handlers in server/logrepl/replication.go; each unknown relation branch uses log.Fatalf, so the failure mode is process termination instead of resilient error handling.
  • Why this is likely a bug: Fatal process exits on malformed/out-of-order replication input violate the expected resilient behavior and are caused by explicit production crash paths in the handler.

Relevant code:

server/logrepl/replication.go (lines 600-603)

rel, ok := state.relations[logicalMsg.RelationID]
if !ok {
    log.Fatalf("unknown relation ID %d", logicalMsg.RelationID)
}

server/logrepl/replication.go (lines 648-651)

rel, ok := state.relations[logicalMsg.RelationID]
if !ok {
    log.Fatalf("unknown relation ID %d", logicalMsg.RelationID)
}

server/logrepl/replication.go (lines 703-705)

rel, ok := state.relations[logicalMsg.RelationID]
if !ok {
    log.Fatalf("unknown relation ID %d", logicalMsg.RelationID)
}

Commit: cbcf41f

View Full Run


Tell us how we did: Give Ito Feedback

@github-actions
Copy link
Copy Markdown
Contributor

Main PR
Total 42090 42090
Successful 18017 18017
Failures 24073 24073
Partial Successes1 5372 5372
Main PR
Successful 42.8059% 42.8059%
Failures 57.1941% 57.1941%

Footnotes

  1. These are tests that we're marking as Successful, however they do not match the expected output in some way. This is due to small differences, such as different wording on the error messages, or the column names being incorrect while the data itself is correct.

@github-actions
Copy link
Copy Markdown
Contributor

This PR has been superseded by #2713

@github-actions github-actions Bot closed this May 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants