fix(dialect): escape driver.Valuer error messages in AppendError

dialect/append.go

@@ -7,9 +7,20 @@ import (
 	"github.com/uptrace/bun/internal"
 )
 
-func AppendError(b []byte, err error) []byte {
+// StringEscaper appends s to b as a properly quoted and escaped SQL string
+// literal. schema.Dialect satisfies this interface.
+type StringEscaper interface {
+	AppendString(b []byte, s string) []byte
+}
+
+// AppendError appends err inside a "?!(...)" marker so a formatting failure
+// is visible in the generated query. err.Error() is untrusted: it can come
+// from a driver.Valuer, JSON/msgpack encoder, etc., so it must go through
+// escaper instead of being appended raw, or a crafted message could break
+// out of the marker and inject SQL.
+func AppendError(b []byte, escaper StringEscaper, err error) []byte {
 	b = append(b, "?!("...)
-	b = append(b, err.Error()...)
+	b = escaper.AppendString(b, err.Error())
 	b = append(b, ')')
 	return b
 }

dialect/append_test.go

@@ -0,0 +1,56 @@
+package dialect
+
+import (
+	"errors"
+	"strings"
+	"testing"
+)
+
+// fakeEscaper stands in for schema.Dialect.AppendString (which dialect
+// cannot import without creating an import cycle): quote the string and
+// double embedded single quotes, the same as the real SQL dialects.
+type fakeEscaper struct{}
+
+func (fakeEscaper) AppendString(b []byte, s string) []byte {
+	b = append(b, '\'')
+	for _, r := range s {
+		if r == '\'' {
+			b = append(b, '\'', '\'')
+			continue
+		}
+		b = append(b, string(r)...)
+	}
+	b = append(b, '\'')
+	return b
+}
+
+// Regression test for the driver.Valuer error-message SQL injection (bun
+// issue #1307): err.Error() used to be appended to the "?!(...)" marker raw,
+// so a crafted message could close the marker and inject arbitrary SQL.
+func TestAppendError(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want string
+	}{
+		{"plain", errors.New("boom"), `?!('boom')`},
+		// Closing the marker with ")" no longer works once the message is a
+		// quoted string literal.
+		{"paren breakout attempt", errors.New(`1)); DROP TABLE t; --`), `?!('1)); DROP TABLE t; --')`},
+		// A quote in the message must be doubled, not left to close the
+		// string literal early.
+		{"embedded quote", errors.New(`it's bad`), `?!('it''s bad')`},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := string(AppendError(nil, fakeEscaper{}, tt.err))
+			if got != tt.want {
+				t.Fatalf("AppendError(%v) = %q, want %q", tt.err, got, tt.want)
+			}
+			if strings.Count(got, "?!(") != 1 {
+				t.Fatalf("AppendError(%v) = %q, want exactly one formatting error marker", tt.err, got)
+			}
+		})
+	}
+}

dialect/pgdialect/append.go

@@ -55,7 +55,7 @@ func (d *Dialect) hstoreAppender(typ reflect.Type) schema.AppenderFunc {
 
 	return func(gen schema.QueryGen, b []byte, v reflect.Value) []byte {
 		err := fmt.Errorf("bun: Hstore(unsupported %s)", v.Type())
-		return dialect.AppendError(b, err)
+		return dialect.AppendError(b, gen.Dialect(), err)
 	}
 }
 

dialect/pgdialect/array.go

@@ -178,9 +178,9 @@ func appendBytesElemValue(gen schema.QueryGen, b []byte, v reflect.Value) []byte
 func arrayAppendDriverValue(gen schema.QueryGen, b []byte, v reflect.Value) []byte {
 	iface, err := v.Interface().(driver.Valuer).Value()
 	if err != nil {
-		return dialect.AppendError(b, err)
+		return dialect.AppendError(b, gen.Dialect(), err)
 	}
-	return appendElem(b, iface)
+	return appendElem(b, gen.Dialect(), iface)
 }
 
 func appendStringSliceValue(gen schema.QueryGen, b []byte, v reflect.Value) []byte {

dialect/pgdialect/elem.go

@@ -11,7 +11,7 @@ import (
 	"github.com/uptrace/bun/dialect"
 )
 
-func appendElem(buf []byte, val any) []byte {
+func appendElem(buf []byte, escaper dialect.StringEscaper, val any) []byte {
 	switch val := val.(type) {
 	case int64:
 		return strconv.AppendInt(buf, val, 10)
@@ -32,12 +32,12 @@ func appendElem(buf []byte, val any) []byte {
 		val2, err := val.Value()
 		if err != nil {
 			err := fmt.Errorf("pgdialect: can't append elem value: %w", err)
-			return dialect.AppendError(buf, err)
+			return dialect.AppendError(buf, escaper, err)
 		}
-		return appendElem(buf, val2)
+		return appendElem(buf, escaper, val2)
 	default:
 		err := fmt.Errorf("pgdialect: can't append elem %T", val)
-		return dialect.AppendError(buf, err)
+		return dialect.AppendError(buf, escaper, err)
 	}
 }
 

dialect/pgdialect/elem_test.go

@@ -0,0 +1,68 @@
+package pgdialect
+
+import (
+	"database/sql/driver"
+	"errors"
+	"strings"
+	"testing"
+
+	"github.com/uptrace/bun/schema"
+)
+
+// badValuer reproduces the driver.Valuer error-message SQL injection (bun
+// issue #1307) for array/range elements, whose escaping goes through
+// appendElem instead of schema's appendDriverValue.
+type badValuer struct{}
+
+func (badValuer) Value() (driver.Value, error) {
+	return nil, errors.New(`x'); DROP TABLE t; --`)
+}
+
+func TestAppendElem_ValuerErrorIsEscaped(t *testing.T) {
+	got := string(appendElem(nil, pgDialect, badValuer{}))
+
+	if !strings.Contains(got, "?!(") {
+		t.Fatalf("appendElem(badValuer) = %q, want a formatting error marker", got)
+	}
+	// The quote in the payload must be doubled, not left to close the
+	// string literal (and therefore the marker) early.
+	if strings.Contains(got, "x');") {
+		t.Fatalf("appendElem(badValuer) = %q, quote was not escaped", got)
+	}
+	if !strings.Contains(got, "x'');") {
+		t.Fatalf("appendElem(badValuer) = %q, want escaped payload", got)
+	}
+}
+
+func TestAppendElem_UnsupportedTypeErrorIsEscaped(t *testing.T) {
+	got := string(appendElem(nil, pgDialect, struct{ X string }{}))
+	if !strings.Contains(got, "?!(") || !strings.Contains(got, "struct { X string }") {
+		t.Fatalf("appendElem(unsupported) = %q", got)
+	}
+}
+
+// A plain value is unaffected by the error-escaping path.
+func TestAppendElem_NoError(t *testing.T) {
+	got := string(appendElem(nil, pgDialect, int64(42)))
+	if got != "42" {
+		t.Fatalf("appendElem(int64) = %q, want %q", got, "42")
+	}
+}
+
+// Range.AppendQuery must thread the real dialect through to appendElem so a
+// driver.Valuer error in a range bound gets escaped the same way.
+func TestRangeAppendQuery_ValuerErrorIsEscaped(t *testing.T) {
+	r := NewRange[driver.Valuer](badValuer{}, badValuer{})
+	gen := schema.NewQueryGen(pgDialect)
+
+	got, err := r.AppendQuery(gen, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if strings.Contains(string(got), "x');") {
+		t.Fatalf("Range.AppendQuery(badValuer) = %q, quote was not escaped", got)
+	}
+	if !strings.Contains(string(got), "x'');") {
+		t.Fatalf("Range.AppendQuery(badValuer) = %q, want escaped payload", got)
+	}
+}

dialect/pgdialect/range.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"time"
 
+	"github.com/uptrace/bun/dialect"
 	"github.com/uptrace/bun/internal"
 	"github.com/uptrace/bun/schema"
 )
@@ -128,14 +129,14 @@ func (r *Range[T]) Scan(raw any) (err error) {
 
 var _ schema.QueryAppender = (*Range[any])(nil)
 
-func (r Range[T]) AppendQuery(_ schema.QueryGen, buf []byte) ([]byte, error) {
+func (r Range[T]) AppendQuery(gen schema.QueryGen, buf []byte) ([]byte, error) {
 	buf = append(buf, '\'')
-	buf = appendRange(buf, r)
+	buf = appendRange(buf, gen.Dialect(), r)
 	buf = append(buf, '\'')
 	return buf, nil
 }
 
-func appendRange[T any](buf []byte, r Range[T]) []byte {
+func appendRange[T any](buf []byte, escaper dialect.StringEscaper, r Range[T]) []byte {
 	if r.IsEmpty() {
 		buf = append(buf, []byte("empty")...)
 		return buf
@@ -147,13 +148,13 @@ func appendRange[T any](buf []byte, r Range[T]) []byte {
 		buf = append(buf, byte(RangeBoundExclusiveLeft))
 	} else {
 		buf = append(buf, byte(r.LowerBound))
-		buf = appendElem(buf, r.Lower)
+		buf = appendElem(buf, escaper, r.Lower)
 	}
 	buf = append(buf, ',')
 	if r.UpperBound == RangeBoundUnset {
 		buf = append(buf, byte(RangeBoundExclusiveRight))
 	} else {
-		buf = appendElem(buf, r.Upper)
+		buf = appendElem(buf, escaper, r.Upper)
 		buf = append(buf, byte(r.UpperBound))
 	}
 	return buf
@@ -170,14 +171,14 @@ func (m *MultiRange[T]) IsZero() bool {
 	return m.Len() == 0
 }
 
-func (m MultiRange[T]) AppendQuery(_ schema.QueryGen, buf []byte) ([]byte, error) {
+func (m MultiRange[T]) AppendQuery(gen schema.QueryGen, buf []byte) ([]byte, error) {
 	if m == nil {
 		return append(buf, []byte("'{}'")...), nil
 	}
 	rs := ([]Range[T])(m)
 	buf = append(buf, '\'', '{')
 	for _, r := range rs {
-		buf = appendRange(buf, r)
+		buf = appendRange(buf, gen.Dialect(), r)
 		buf = append(buf, ',')
 	}
 	if len(rs) > 0 {

schema/append_value.go

@@ -232,7 +232,7 @@ func AppendStringValue(gen QueryGen, b []byte, v reflect.Value) []byte {
 func AppendJSONValue(gen QueryGen, b []byte, v reflect.Value) []byte {
 	bb, err := bunjson.Marshal(v.Interface())
 	if err != nil {
-		return dialect.AppendError(b, err)
+		return dialect.AppendError(b, gen.Dialect(), err)
 	}
 
 	if len(bb) > 0 && bb[len(bb)-1] == '\n' {
@@ -271,10 +271,10 @@ func appendQueryAppenderValue(gen QueryGen, b []byte, v reflect.Value) []byte {
 func appendDriverValue(gen QueryGen, b []byte, v reflect.Value) []byte {
 	value, err := v.Interface().(driver.Valuer).Value()
 	if err != nil {
-		return dialect.AppendError(b, err)
+		return dialect.AppendError(b, gen.Dialect(), err)
 	}
 	if _, ok := value.(driver.Valuer); ok {
-		return dialect.AppendError(b, fmt.Errorf("driver.Valuer returns unsupported type %T", value))
+		return dialect.AppendError(b, gen.Dialect(), fmt.Errorf("driver.Valuer returns unsupported type %T", value))
 	}
 	return gen.Append(b, value)
 }
@@ -283,7 +283,7 @@ func addrAppender(fn AppenderFunc) AppenderFunc {
 	return func(gen QueryGen, b []byte, v reflect.Value) []byte {
 		if !v.CanAddr() {
 			err := fmt.Errorf("bun: Append(nonaddressable %T)", v.Interface())
-			return dialect.AppendError(b, err)
+			return dialect.AppendError(b, gen.Dialect(), err)
 		}
 		return fn(gen, b, v.Addr())
 	}
@@ -297,11 +297,11 @@ func appendMsgpack(gen QueryGen, b []byte, v reflect.Value) []byte {
 
 	enc.Reset(hexEnc)
 	if err := enc.EncodeValue(v); err != nil {
-		return dialect.AppendError(b, err)
+		return dialect.AppendError(b, gen.Dialect(), err)
 	}
 
 	if err := hexEnc.Close(); err != nil {
-		return dialect.AppendError(b, err)
+		return dialect.AppendError(b, gen.Dialect(), err)
 	}
 
 	return hexEnc.Bytes()
@@ -310,7 +310,7 @@ func appendMsgpack(gen QueryGen, b []byte, v reflect.Value) []byte {
 func AppendQueryAppender(gen QueryGen, b []byte, app QueryAppender) []byte {
 	bb, err := app.AppendQuery(gen, b)
 	if err != nil {
-		return dialect.AppendError(b, err)
+		return dialect.AppendError(b, gen.Dialect(), err)
 	}
 	return bb
 }

schema/append_value_test.go

@@ -0,0 +1,41 @@
+package schema
+
+import (
+	"database/sql"
+	"database/sql/driver"
+	"errors"
+	"reflect"
+	"testing"
+)
+
+// badValuer reproduces the driver.Valuer error-message SQL injection (bun
+// issue #1307): Value() returns an error whose message is crafted to close
+// the "?!(...)" marker and append arbitrary SQL.
+type badValuer struct{}
+
+func (badValuer) Value() (driver.Value, error) {
+	return nil, errors.New(`pwned'); INSERT INTO pwned VALUES (999); --`)
+}
+
+func TestAppendDriverValue_ErrorIsEscaped(t *testing.T) {
+	gen := NewQueryGen(newNopDialect())
+	got := string(appendDriverValue(gen, nil, reflect.ValueOf(badValuer{})))
+
+	// The embedded quote must be doubled, keeping the injected text inside
+	// the string literal instead of letting it break out.
+	want := `?!('pwned''); INSERT INTO pwned VALUES (999); --')`
+	if got != want {
+		t.Fatalf("appendDriverValue(badValuer) = %q, want %q", got, want)
+	}
+}
+
+// A driver.Valuer that succeeds must be unaffected by the error-escaping path.
+func TestAppendDriverValue_NoError(t *testing.T) {
+	gen := NewQueryGen(newNopDialect())
+	got := string(appendDriverValue(gen, nil, reflect.ValueOf(sql.NullString{String: "ok", Valid: true})))
+
+	want := `'ok'`
+	if got != want {
+		t.Fatalf("appendDriverValue(NullString) = %q, want %q", got, want)
+	}
+}

schema/dialect.go

@@ -69,13 +69,16 @@ func (BaseDialect) AppendTime(b []byte, tm time.Time) []byte {
 	return b
 }
 
-func (BaseDialect) AppendString(b []byte, s string) []byte {
+func (d BaseDialect) AppendString(b []byte, s string) []byte {
 	b = append(b, '\'')
 	for _, r := range s {
 		if r == '\000' {
 			// Fail closed instead of silently dropping the NUL, which would
-			// persist a value different from the one that was validated.
-			return dialect.AppendError(b, errStringNul)
+			// persist a value different from the one that was validated. The
+			// marker text is a fixed constant (no attacker-controlled bytes),
+			// but it still goes through AppendString for a consistently
+			// quoted/escaped "?!(...)" marker.
+			return dialect.AppendError(b, d, errStringNul)
 		}
 
 		if r == '\'' {

schema/querygen.go

@@ -220,7 +220,7 @@ func (gen QueryGen) appendArg(b []byte, arg any) []byte {
 	case QueryAppender:
 		bb, err := arg.AppendQuery(gen, b)
 		if err != nil {
-			return dialect.AppendError(b, err)
+			return dialect.AppendError(b, gen.Dialect(), err)
 		}
 		return bb
 	default: