From 27af564c846de652d952197c3f2e48708be09786 Mon Sep 17 00:00:00 2001 From: Peter Dotchev Date: Mon, 4 Nov 2019 11:43:23 +0200 Subject: [PATCH] Fix panic: index out of range (#346) * Fix panic: index out of range * Validate that right operand is provided --- pkg/query/selection.go | 12 ++++--- pkg/query/selection_test.go | 49 ++++++++++++++++++++++++-- storage/postgres/query_builder_test.go | 18 +++++----- storage/postgres/where_clause_tree.go | 4 ++- 4 files changed, 66 insertions(+), 17 deletions(-) diff --git a/pkg/query/selection.go b/pkg/query/selection.go index 44614dcd2..ed1ce6c4a 100644 --- a/pkg/query/selection.go +++ b/pkg/query/selection.go @@ -18,6 +18,7 @@ package query import ( "context" + "errors" "fmt" "sort" "strconv" @@ -137,11 +138,15 @@ func NewCriterion(leftOp string, operator Operator, rightOp []string, criteriaTy // Validate the criterion fields func (c Criterion) Validate() error { + if len(c.RightOp) == 0 { + return errors.New("missing right operand") + } + if c.Type == ResultQuery { if c.LeftOp == Limit { limit, err := strconv.Atoi(c.RightOp[0]) if err != nil { - return fmt.Errorf("could not cast string to int: %s", err.Error()) + return fmt.Errorf("could not convert string to int: %s", err.Error()) } if limit < 0 { return &util.UnsupportedQueryError{Message: fmt.Sprintf("limit (%d) is invalid. Limit should be positive number", limit)} @@ -149,11 +154,8 @@ func (c Criterion) Validate() error { } if c.LeftOp == OrderBy { - if len(c.RightOp) < 1 { - return &util.UnsupportedQueryError{Message: "order by result expects field and order type, but has none"} - } if len(c.RightOp) < 2 { - return &util.UnsupportedQueryError{Message: fmt.Sprintf(`order by result for field "%s" expects order type, but has none`, c.RightOp[0])} + return &util.UnsupportedQueryError{Message: "order by result expects field name and order type"} } } diff --git a/pkg/query/selection_test.go b/pkg/query/selection_test.go index 6b5f27f36..79d20549b 100644 --- a/pkg/query/selection_test.go +++ b/pkg/query/selection_test.go @@ -21,9 +21,11 @@ import ( "fmt" "strings" - . "github.com/Peripli/service-manager/pkg/query" . "github.com/onsi/ginkgo" + . "github.com/onsi/ginkgo/extensions/table" . "github.com/onsi/gomega" + + . "github.com/Peripli/service-manager/pkg/query" ) var _ = Describe("Selection", func() { @@ -54,7 +56,7 @@ var _ = Describe("Selection", func() { addInvalidCriterion(ByField(LessThanOrEqualOperator, "leftOp", "non-numeric")) }) Specify("Right operand containing new line", func() { - addInvalidCriterion(ByField(EqualsOperator, "leftOp", `value with + addInvalidCriterion(ByField(EqualsOperator, "leftOp", `value with new line`)) }) Specify("Left operand with query separator", func() { @@ -291,4 +293,47 @@ new line`)) }) } }) + + DescribeTable("Validate Criterion", + func(c Criterion, expectedErr ...string) { + err := c.Validate() + if len(expectedErr) == 0 { + Expect(err).To(BeNil()) + } else { + Expect(err).NotTo(BeNil()) + Expect(err.Error()).To(ContainSubstring(expectedErr[0])) + } + }, + Entry("Empty right operand is not allowed", + ByField(InOperator, "left"), + "missing right operand"), + Entry("Limit that is not a number is not allowed", + NewCriterion(Limit, NoOperator, []string{"not a number"}, ResultQuery), + "could not convert string to int"), + Entry("Negative limit is not allowed", + LimitResultBy(-1), + "should be positive"), + Entry("Valid limit is allowed", + LimitResultBy(10)), + Entry("Missing order type is not allowed", + NewCriterion(OrderBy, NoOperator, []string{"field"}, ResultQuery), + "expects field name and order type"), + Entry("Valid order by is allowed", + OrderResultBy("field", AscOrder)), + Entry("Multiple right operands are not allowed for univariate operators", + ByField(EqualsOperator, "left", "right1", "right2"), + "single value operation"), + Entry("Operators working with nil are not allowed for label queries", + ByLabel(EqualsOrNilOperator, "left", "right"), + "only for field queries"), + Entry("Non-numeric operand is not allowed by comparison operators", + ByField(LessThanOperator, "left", "not numeric"), + "not numeric or datetime"), + Entry("Separator word 'and' is not allowed to appear in left operand", + ByField(EqualsOperator, "band", "music"), + "separator and is not allowed"), + Entry("New line character is not allowed in right operand", + ByField(EqualsOperator, "left", "one\ntwo"), + "forbidden new line character"), + ) }) diff --git a/storage/postgres/query_builder_test.go b/storage/postgres/query_builder_test.go index 65231f6af..f9ab0642a 100644 --- a/storage/postgres/query_builder_test.go +++ b/storage/postgres/query_builder_test.go @@ -193,7 +193,7 @@ ORDER BY id DESC, created_at ASC;`))) }). List(ctx) Expect(err).Should(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring(`order by result for field "id" expects order type, but has none`)) + Expect(err.Error()).To(ContainSubstring("order by result expects field name and order type")) }) }) @@ -207,7 +207,7 @@ ORDER BY id DESC, created_at ASC;`))) }). List(ctx) Expect(err).Should(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("order by result expects field and order type, but has none")) + Expect(err.Error()).To(ContainSubstring("missing right operand")) }) }) }) @@ -390,8 +390,8 @@ FROM visibilities ;`))) Count(ctx) Expect(err).ShouldNot(HaveOccurred()) Expect(executedQuery).Should(Equal(trim(` -SELECT COUNT(DISTINCT visibilities.id) -FROM visibilities +SELECT COUNT(DISTINCT visibilities.id) +FROM visibilities LIMIT ?;`))) Expect(queryArgs).To(HaveLen(1)) Expect(queryArgs[0]).Should(Equal("10")) @@ -508,7 +508,7 @@ WHERE visibilities.id = t.id ;`))) Expect(executedQuery).Should(Equal(trim(` DELETE -FROM visibilities +FROM visibilities WHERE visibilities.id::text = ? ;`))) Expect(queryArgs).To(HaveLen(1)) Expect(queryArgs[0]).Should(Equal("1")) @@ -529,8 +529,8 @@ WHERE visibilities.id::text = ? ;`))) Expect(err).ToNot(HaveOccurred()) Expect(executedQuery).To(Equal(trim(` -DELETE -FROM visibilities +DELETE +FROM visibilities RETURNING id, service_plan_id;`))) }) @@ -539,8 +539,8 @@ RETURNING id, service_plan_id;`))) Expect(err).ToNot(HaveOccurred()) Expect(executedQuery).To(Equal(trim(` -DELETE -FROM visibilities +DELETE +FROM visibilities RETURNING *;`))) }) diff --git a/storage/postgres/where_clause_tree.go b/storage/postgres/where_clause_tree.go index 3c82e2947..94988d578 100644 --- a/storage/postgres/where_clause_tree.go +++ b/storage/postgres/where_clause_tree.go @@ -99,10 +99,12 @@ func criterionSQL(c query.Criterion, dbTags []tagType, tableAlias string) (strin func buildRightOp(operator query.Operator, rightOp []string) (string, interface{}) { rightOpBindVar := "?" - var rhs interface{} = rightOp[0] + var rhs interface{} if operator.Type() == query.MultivariateOperator { rightOpBindVar = "(?)" rhs = rightOp + } else { + rhs = rightOp[0] } return rightOpBindVar, rhs }