Skip to content

Commit 0eb6853

Browse files
koenbollenkoenbollen
committed
Introduce new, required, access control options
Previously this library has been insecure by default, this change will make one of the new access control options required.
1 parent 9061a61 commit 0eb6853

File tree

9 files changed

+245
-46
lines changed

9 files changed

+245
-46
lines changed

‎README.md

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,12 @@ import (
3232

3333
func main() {
3434
// Create a converter with options:
35+
// - WithAllowAllColumns: allow all columns to be filtered.
3536
// - WithArrayDriver: to convert arrays to the correct driver type, required when using lib/pq
36-
converter := filter.NewConverter(filter.WithArrayDriver(pq.Array))
37+
converter, err := filter.NewConverter(filter.WithAllowAllColumns(), filter.WithArrayDriver(pq.Array))
38+
if err != nil {
39+
// handle error
40+
}
3741

3842
// Convert a filter query to a WHERE clause and values:
3943
input := []byte(`{"title": "Jurassic Park"}`)

‎examples/basic_test.go

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,10 @@ import (
99

1010
func ExampleNewConverter() {
1111
// Remeber to use `filter.WithArrayDriver(pg.Array)` when using github.com/lib/pq
12-
converter := filter.NewConverter(filter.WithNestedJSONB("meta", "created_at", "updated_at"))
12+
converter, err := filter.NewConverter(filter.WithNestedJSONB("meta", "created_at", "updated_at"))
13+
if err != nil {
14+
// handle error
15+
}
1316

1417
mongoFilterQuery := `{
1518
"name": "John",
@@ -30,7 +33,10 @@ func ExampleNewConverter() {
3033
}
3134

3235
func ExampleNewConverter_emptyfilter() {
33-
converter := filter.NewConverter(filter.WithEmptyCondition("TRUE")) // The default is FALSE if you don't change it.
36+
converter, err := filter.NewConverter(filter.WithAllowAllColumns(), filter.WithEmptyCondition("TRUE")) // The default is FALSE if you don't change it.
37+
if err != nil {
38+
// handle error
39+
}
3440

3541
mongoFilterQuery := `{}`
3642
conditions, _, err := converter.Convert([]byte(mongoFilterQuery), 1)
@@ -44,7 +50,10 @@ func ExampleNewConverter_emptyfilter() {
4450
}
4551

4652
func ExampleNewConverter_nonIsolatedConditions() {
47-
converter := filter.NewConverter()
53+
converter, err := filter.NewConverter(filter.WithAllowAllColumns())
54+
if err != nil {
55+
// handle error
56+
}
4857

4958
mongoFilterQuery := `{
5059
"$or": [

‎examples/readme_test.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,10 @@ import (
88

99
func ExampleNewConverter_readme() {
1010
// Remeber to use `filter.WithArrayDriver(pg.Array)` when using github.com/lib/pq
11-
converter := filter.NewConverter(filter.WithNestedJSONB("meta", "created_at", "updated_at"))
11+
converter, err := filter.NewConverter(filter.WithNestedJSONB("meta", "created_at", "updated_at"))
12+
if err != nil {
13+
// handle error
14+
}
1215

1316
mongoFilterQuery := `{
1417
"$and": [

‎filter/converter.go

Lines changed: 40 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,12 @@ const defaultPlaceholderName = "__filter_placeholder"
3030

3131
// Converter converts MongoDB filter queries to SQL conditions and values. Use [filter.NewConverter] to create a new instance.
3232
type Converter struct {
33-
nestedColumn string
34-
nestedExemptions []string
35-
arrayDriver func(a any) interface {
33+
allowAllColumns bool
34+
allowedColumns []string
35+
disallowedColumns []string
36+
nestedColumn string
37+
nestedExemptions []string
38+
arrayDriver func(a any) interface {
3639
driver.Valuer
3740
sql.Scanner
3841
}
@@ -45,16 +48,23 @@ type Converter struct {
4548
// NewConverter creates a new [Converter] with optional nested JSONB field mapping.
4649
//
4750
// Note: When using https://github.com/lib/pq, the [filter.WithArrayDriver] should be set to pq.Array.
48-
func NewConverter(options ...Option) *Converter {
51+
func NewConverter(options ...Option) (*Converter, error) {
4952
converter := &Converter{
5053
// don't set defaults, use the once.Do in #Convert()
5154
}
55+
seenAccessOption := false
5256
for _, option := range options {
53-
if option != nil {
54-
option(converter)
57+
if option.f != nil {
58+
option.f(converter)
5559
}
60+
if option.isAccessOption {
61+
seenAccessOption = true
62+
}
63+
}
64+
if !seenAccessOption {
65+
return nil, ErrNoAccessOption
5666
}
57-
return converter
67+
return converter, nil
5868
}
5969

6070
// Convert converts a MongoDB filter query into SQL conditions and values.
@@ -165,6 +175,9 @@ func (c *Converter) convertFilter(filter map[string]any, paramIndex int) (string
165175
if !isValidPostgresIdentifier(key) {
166176
return "", nil, fmt.Errorf("invalid column name: %s", key)
167177
}
178+
if !c.isColumnAllowed(key) {
179+
return "", nil, ColumnNotAllowedError{Column: key}
180+
}
168181

169182
switch v := value.(type) {
170183
case map[string]any:
@@ -346,6 +359,26 @@ func (c *Converter) columnName(column string) string {
346359
return fmt.Sprintf(`%q->>'%s'`, c.nestedColumn, column)
347360
}
348361

362+
func (c *Converter) isColumnAllowed(column string) bool {
363+
for _, disallowed := range c.disallowedColumns {
364+
if disallowed == column {
365+
return false
366+
}
367+
}
368+
if c.allowAllColumns {
369+
return true
370+
}
371+
if c.nestedColumn != "" {
372+
return true
373+
}
374+
for _, allowed := range c.allowedColumns {
375+
if allowed == column {
376+
return true
377+
}
378+
}
379+
return false
380+
}
381+
349382
func (c *Converter) isNestedColumn(column string) bool {
350383
if c.nestedColumn == "" {
351384
return false

‎filter/converter_test.go

Lines changed: 110 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,10 @@ import (
1111

1212
func ExampleNewConverter() {
1313
// Remeber to use `filter.WithArrayDriver(pg.Array)` when using github.com/lib/pq
14-
converter := filter.NewConverter(filter.WithNestedJSONB("meta", "created_at", "updated_at"))
14+
converter, err := filter.NewConverter(filter.WithNestedJSONB("meta", "created_at", "updated_at"))
15+
if err != nil {
16+
// handle error
17+
}
1518

1619
mongoFilterQuery := `{
1720
"name": "John",
@@ -33,7 +36,7 @@ func ExampleNewConverter() {
3336
func TestConverter_Convert(t *testing.T) {
3437
tests := []struct {
3538
name string
36-
option filter.Option
39+
option []filter.Option
3740
input string
3841
conditions string
3942
values []any
@@ -73,15 +76,15 @@ func TestConverter_Convert(t *testing.T) {
7376
},
7477
{
7578
"nested jsonb single value",
76-
filter.WithNestedJSONB("meta"),
79+
[]filter.Option{filter.WithNestedJSONB("meta")},
7780
`{"name": "John"}`,
7881
`("meta"->>'name' = $1)`,
7982
[]any{"John"},
8083
nil,
8184
},
8285
{
8386
"nested jsonb multi value",
84-
filter.WithNestedJSONB("meta", "created_at", "updated_at"),
87+
[]filter.Option{filter.WithNestedJSONB("meta", "created_at", "updated_at")},
8588
`{"created_at": {"$gte": "2020-01-01T00:00:00Z"}, "name": "John", "role": "admin"}`,
8689
`(("created_at" >= $1) AND ("meta"->>'name' = $2) AND ("meta"->>'role' = $3))`,
8790
[]any{"2020-01-01T00:00:00Z", "John", "admin"},
@@ -296,7 +299,7 @@ func TestConverter_Convert(t *testing.T) {
296299
},
297300
{
298301
"null jsonb column",
299-
filter.WithNestedJSONB("meta"),
302+
[]filter.Option{filter.WithNestedJSONB("meta")},
300303
`{"name": null}`,
301304
`(jsonb_path_match(meta, 'exists($.name)') AND "meta"->>'name' IS NULL)`,
302305
nil,
@@ -312,15 +315,15 @@ func TestConverter_Convert(t *testing.T) {
312315
},
313316
{
314317
"not $exists jsonb column",
315-
filter.WithNestedJSONB("meta"),
318+
[]filter.Option{filter.WithNestedJSONB("meta")},
316319
`{"name": {"$exists": false}}`,
317320
`(NOT jsonb_path_match(meta, 'exists($.name)'))`,
318321
nil,
319322
nil,
320323
},
321324
{
322325
"$exists jsonb column",
323-
filter.WithNestedJSONB("meta"),
326+
[]filter.Option{filter.WithNestedJSONB("meta")},
324327
`{"name": {"$exists": true}}`,
325328
`(jsonb_path_match(meta, 'exists($.name)'))`,
326329
nil,
@@ -344,31 +347,31 @@ func TestConverter_Convert(t *testing.T) {
344347
},
345348
{
346349
"$elemMatch on jsonb column",
347-
filter.WithNestedJSONB("meta"),
350+
[]filter.Option{filter.WithNestedJSONB("meta")},
348351
`{"name": {"$elemMatch": {"$eq": "John"}}}`,
349352
`EXISTS (SELECT 1 FROM jsonb_array_elements("meta"->'name') AS __filter_placeholder WHERE ("__filter_placeholder"::text = $1))`,
350353
[]any{"John"},
351354
nil,
352355
},
353356
{
354357
"$elemMatch with $gt",
355-
filter.WithPlaceholderName("__placeholder"),
358+
[]filter.Option{filter.WithAllowAllColumns(), filter.WithPlaceholderName("__placeholder")},
356359
`{"age": {"$elemMatch": {"$gt": 18}}}`,
357360
`EXISTS (SELECT 1 FROM unnest("age") AS __placeholder WHERE ("__placeholder"::text > $1))`,
358361
[]any{float64(18)},
359362
nil,
360363
},
361364
{
362365
"numeric comparison bug with jsonb column",
363-
filter.WithNestedJSONB("meta"),
366+
[]filter.Option{filter.WithNestedJSONB("meta")},
364367
`{"foo": {"$gt": 0}}`,
365368
`(("meta"->>'foo')::numeric > $1)`,
366369
[]any{float64(0)},
367370
nil,
368371
},
369372
{
370373
"numeric comparison against null with jsonb column",
371-
filter.WithNestedJSONB("meta"),
374+
[]filter.Option{filter.WithNestedJSONB("meta")},
372375
`{"foo": {"$gt": null}}`,
373376
`("meta"->>'foo' > $1)`,
374377
[]any{nil},
@@ -392,23 +395,23 @@ func TestConverter_Convert(t *testing.T) {
392395
},
393396
{
394397
"compare two jsonb fields",
395-
filter.WithNestedJSONB("meta"),
398+
[]filter.Option{filter.WithNestedJSONB("meta")},
396399
`{"foo": {"$eq": {"$field": "bar"}}}`,
397400
`("meta"->>'foo' = "meta"->>'bar')`,
398401
nil,
399402
nil,
400403
},
401404
{
402405
"compare two jsonb fields with numeric comparison",
403-
filter.WithNestedJSONB("meta"),
406+
[]filter.Option{filter.WithNestedJSONB("meta")},
404407
`{"foo": {"$lt": {"$field": "bar"}}}`,
405408
`(("meta"->>'foo')::numeric < ("meta"->>'bar')::numeric)`,
406409
nil,
407410
nil,
408411
},
409412
{
410413
"compare two fields with simple expression",
411-
filter.WithNestedJSONB("meta", "foo"),
414+
[]filter.Option{filter.WithNestedJSONB("meta", "foo")},
412415
`{"foo": {"$field": "bar"}}`,
413416
`("foo" = "meta"->>'bar')`,
414417
nil,
@@ -426,7 +429,13 @@ func TestConverter_Convert(t *testing.T) {
426429

427430
for _, tt := range tests {
428431
t.Run(tt.name, func(t *testing.T) {
429-
c := filter.NewConverter(tt.option)
432+
if tt.option == nil {
433+
tt.option = []filter.Option{filter.WithAllowAllColumns()}
434+
}
435+
c, err := filter.NewConverter(tt.option...)
436+
if err != nil {
437+
t.Fatal(err)
438+
}
430439
conditions, values, err := c.Convert([]byte(tt.input), 1)
431440
if err != nil && (tt.err == nil || err.Error() != tt.err.Error()) {
432441
t.Errorf("Converter.Convert() error = %v, wantErr %v", err, tt.err)
@@ -447,7 +456,7 @@ func TestConverter_Convert(t *testing.T) {
447456
}
448457

449458
func TestConverter_Convert_startAtParameterIndex(t *testing.T) {
450-
c := filter.NewConverter()
459+
c, _ := filter.NewConverter(filter.WithAllowAllColumns())
451460
conditions, values, err := c.Convert([]byte(`{"name": "John", "password": "secret"}`), 10)
452461
if err != nil {
453462
t.Fatal(err)
@@ -476,7 +485,7 @@ func TestConverter_Convert_startAtParameterIndex(t *testing.T) {
476485
}
477486

478487
func TestConverter_WithEmptyCondition(t *testing.T) {
479-
c := filter.NewConverter(filter.WithEmptyCondition("TRUE"))
488+
c, _ := filter.NewConverter(filter.WithAllowAllColumns(), filter.WithEmptyCondition("TRUE"))
480489
conditions, values, err := c.Convert([]byte(`{}`), 1)
481490
if err != nil {
482491
t.Fatal(err)
@@ -490,6 +499,8 @@ func TestConverter_WithEmptyCondition(t *testing.T) {
490499
}
491500

492501
func TestConverter_NoConstructor(t *testing.T) {
502+
t.Skip() // this is currently not supported since we introduced the access control options
503+
493504
c := &filter.Converter{}
494505
conditions, values, err := c.Convert([]byte(`{"name": "John"}`), 1)
495506
if err != nil {
@@ -524,3 +535,85 @@ func TestConverter_CopyReference(t *testing.T) {
524535
t.Errorf("Converter.Convert() conditions = %v, want %v", conditions, want)
525536
}
526537
}
538+
539+
func TestConverter_RequireAccessControl(t *testing.T) {
540+
if _, err := filter.NewConverter(); err != filter.ErrNoAccessOption {
541+
t.Errorf("NewConverter() error = %v, want %v", err, filter.ErrNoAccessOption)
542+
}
543+
if _, err := filter.NewConverter(filter.WithPlaceholderName("___test___")); err != filter.ErrNoAccessOption {
544+
t.Errorf("NewConverter() error = %v, want %v", err, filter.ErrNoAccessOption)
545+
}
546+
if _, err := filter.NewConverter(filter.WithAllowAllColumns()); err != nil {
547+
t.Errorf("NewConverter() error = %v, want no error", err)
548+
}
549+
if _, err := filter.NewConverter(filter.WithAllowColumns("name", "map")); err != nil {
550+
t.Errorf("NewConverter() error = %v, want no error", err)
551+
}
552+
if _, err := filter.NewConverter(filter.WithAllowColumns()); err != nil {
553+
t.Errorf("NewConverter() error = %v, want no error", err)
554+
}
555+
if _, err := filter.NewConverter(filter.WithNestedJSONB("meta", "created_at", "updated_at")); err != nil {
556+
t.Errorf("NewConverter() error = %v, want no error", err)
557+
}
558+
if _, err := filter.NewConverter(filter.WithDisallowColumns("password")); err != nil {
559+
t.Errorf("NewConverter() error = %v, want no error", err)
560+
}
561+
}
562+
563+
func TestConverter_AccessControl(t *testing.T) {
564+
f := func(in string, wantErr error, options ...filter.Option) func(t *testing.T) {
565+
t.Helper()
566+
return func(t *testing.T) {
567+
t.Helper()
568+
c := &filter.Converter{}
569+
if options != nil {
570+
c, _ = filter.NewConverter(options...)
571+
// requirement of access control is tested above.
572+
}
573+
q, _, err := c.Convert([]byte(in), 1)
574+
t.Log(in, "->", q, err)
575+
if wantErr == nil && err != nil {
576+
t.Fatalf("no error returned, expected error: %v", err)
577+
} else if wantErr != nil && err == nil {
578+
t.Fatalf("expected error: %v", wantErr)
579+
} else if wantErr != nil && wantErr.Error() != err.Error() {
580+
t.Fatalf("error mismatch: %v != %v", err, wantErr)
581+
}
582+
}
583+
}
584+
585+
no := func(c string) error { return filter.ColumnNotAllowedError{Column: c} }
586+
587+
t.Run("allow all, single root field",
588+
f(`{"name":"John"}`, nil, filter.WithAllowAllColumns()))
589+
t.Run("allow name, single allowed root field",
590+
f(`{"name":"John"}`, nil, filter.WithAllowColumns("name")))
591+
t.Run("allow name, single disallowed root field",
592+
f(`{"password":"hacks"}`, no("password"), filter.WithAllowColumns("name")))
593+
t.Run("allowed meta, single allowed nested field",
594+
f(`{"map":"de_dust"}`, nil, filter.WithNestedJSONB("meta", "created_at")))
595+
t.Run("allowed nested excemption, single allowed field",
596+
f(`{"created_at":"de_dust"}`, nil, filter.WithNestedJSONB("meta", "created_at")))
597+
t.Run("multi allow, single allowed root field",
598+
f(`{"name":"John"}`, nil, filter.WithAllowColumns("name", "email")))
599+
t.Run("multi allow, two allowed root fields",
600+
f(`{"name":"John", "email":"[email protected]"}`, nil, filter.WithAllowColumns("name", "email")))
601+
t.Run("multi allow, mixes access",
602+
f(`{"name":"John", "password":"hacks"}`, no("password"), filter.WithAllowColumns("name", "email")))
603+
t.Run("multi allow, mixes access",
604+
f(`{"name":"John", "password":"hacks"}`, no("password"), filter.WithAllowColumns("name", "email")))
605+
t.Run("allowed basic $and",
606+
f(`{"$and": [{"name": "John"}, {"version": 3}]}`, nil, filter.WithAllowColumns("name", "version")))
607+
t.Run("disallowed basic $and",
608+
f(`{"$and": [{"name": "John"}, {"version": 3}]}`, no("version"), filter.WithAllowColumns("name")))
609+
t.Run("allow all but one",
610+
f(`{"name": "John"}`, nil, filter.WithAllowAllColumns(), filter.WithDisallowColumns("password")))
611+
t.Run("allow all but one, failing",
612+
f(`{"$and": [{"name": "John"}, {"password": "hacks"}]}`, no("password"), filter.WithAllowAllColumns(), filter.WithDisallowColumns("password")))
613+
t.Run("nested but disallow password, allow exception",
614+
f(`{"created_at": "1"}`, nil, filter.WithNestedJSONB("meta", "created_at"), filter.WithDisallowColumns("password")))
615+
t.Run("nested but disallow password, allow nested",
616+
f(`{"map": "de_dust"}`, nil, filter.WithNestedJSONB("meta", "created_at"), filter.WithDisallowColumns("password")))
617+
t.Run("nested but disallow password, disallow",
618+
f(`{"password": "hacks"}`, no("password"), filter.WithNestedJSONB("meta", "created_at"), filter.WithDisallowColumns("password")))
619+
}

0 commit comments

Comments
 (0)