Merge pull request #14208 from ywk253100/210208_sql

Fix sql issue for querying tasks

src/lib/orm/orm.go

@@ -18,6 +18,8 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strconv"
+	"strings"
 
 	"github.com/astaxie/beego/orm"
 	"github.com/goharbor/harbor/src/lib/log"
@@ -146,3 +148,29 @@ func ReadOrCreate(ctx context.Context, md interface{}, col1 string, cols ...stri
 
 	return
 }
+
+// CreateInClause creates an IN clause with the provided sql and args to avoid the sql injection
+// The sql should return the ID list with the specific condition(e.g. select id from table1 where column1=?)
+// The sql runs as a prepare statement with the "?" be populated rather than concat string directly
+// The returning in clause is a string like "IN (id1, id2, id3, ...)"
+func CreateInClause(ctx context.Context, sql string, args ...interface{}) (string, error) {
+	ormer, err := FromContext(ctx)
+	if err != nil {
+		return "", err
+	}
+	ids := []int64{}
+	if _, err = ormer.Raw(sql, args...).QueryRows(&ids); err != nil {
+		return "", err
+	}
+	// no matching, append -1 as the id
+	if len(ids) == 0 {
+		ids = append(ids, -1)
+	}
+	var idStrs []string
+	for _, id := range ids {
+		idStrs = append(idStrs, strconv.FormatInt(id, 10))
+	}
+	// there is no too many arguments issue like https://github.com/goharbor/harbor/issues/12269
+	// when concat the in clause directly
+	return fmt.Sprintf(`IN (%s)`, strings.Join(idStrs, ",")), nil
+}

src/pkg/artifact/dao/dao.go

@@ -261,7 +261,7 @@ func querySetter(ctx context.Context, query *q.Query) (beegoorm.QuerySeter, erro
 	if err != nil {
 		return nil, err
 	}
-	qs, err = setTagQuery(qs, query)
+	qs, err = setTagQuery(ctx, qs, query)
 	if err != nil {
 		return nil, err
 	}
@@ -296,7 +296,7 @@ func setBaseQuery(qs beegoorm.QuerySeter, query *q.Query) (beegoorm.QuerySeter,
 }
 
 // handle query string: q=tags=value q=tags=~value
-func setTagQuery(qs beegoorm.QuerySeter, query *q.Query) (beegoorm.QuerySeter, error) {
+func setTagQuery(ctx context.Context, qs beegoorm.QuerySeter, query *q.Query) (beegoorm.QuerySeter, error) {
 	if query == nil || len(query.Keywords) == 0 {
 		return qs, nil
 	}
@@ -311,11 +311,14 @@ func setTagQuery(qs beegoorm.QuerySeter, query *q.Query) (beegoorm.QuerySeter, e
 	// fuzzy match
 	f, ok := tags.(*q.FuzzyMatchValue)
 	if ok {
-		sql := fmt.Sprintf(`IN (
+		// get the id list first to avoid the sql injection
-		SELECT DISTINCT art.id FROM artifact art
+		inClause, err := orm.CreateInClause(ctx, `SELECT DISTINCT art.id FROM artifact art
-		JOIN tag ON art.id=tag.artifact_id
+			JOIN tag ON art.id=tag.artifact_id
-		WHERE tag.name LIKE '%%%s%%')`, orm.Escape(f.Value))
+			WHERE tag.name LIKE ?`, "%"+orm.Escape(f.Value)+"%")
-		qs = qs.FilterRaw("id", sql)
+		if err != nil {
+			return nil, err
+		}
+		qs = qs.FilterRaw("id", inClause)
 		return qs, nil
 	}
 	// exact match:
@@ -332,11 +335,15 @@ func setTagQuery(qs beegoorm.QuerySeter, query *q.Query) (beegoorm.QuerySeter, e
 			qs = qs.FilterRaw("id", untagged)
 			return qs, nil
 		}
-		sql := fmt.Sprintf(`IN (
+
-		SELECT DISTINCT art.id FROM artifact art
+		// get the id list first to avoid the sql injection
-		JOIN tag ON art.id=tag.artifact_id
+		inClause, err := orm.CreateInClause(ctx, `SELECT DISTINCT art.id FROM artifact art
-		WHERE tag.name = '%s')`, s)
+			JOIN tag ON art.id=tag.artifact_id
-		qs = qs.FilterRaw("id", sql)
+			WHERE tag.name = ?`, s)
+		if err != nil {
+			return nil, err
+		}
+		qs = qs.FilterRaw("id", inClause)
 		return qs, nil
 	}
 	return qs, errors.New(nil).WithCode(errors.BadRequestCode).
@@ -367,6 +374,7 @@ func setLabelQuery(qs beegoorm.QuerySeter, query *q.Query) (beegoorm.QuerySeter,
 			return qs, errors.New(nil).WithCode(errors.BadRequestCode).
 				WithMessage(`the value of "labels" query can only be integer list with intersetion relationship`)
 		}
+		// param "labelID" is integer, no need to sanitize
 		collections = append(collections, fmt.Sprintf(`SELECT artifact_id FROM label_reference WHERE label_id=%d`, labelID))
 	}
 	qs = qs.FilterRaw("id", fmt.Sprintf(`IN (%s)`, strings.Join(collections, " INTERSECT ")))

src/pkg/task/dao/execution.go

@@ -315,16 +315,30 @@ func (e *executionDAO) querySetter(ctx context.Context, query *q.Query) (orm.Que
 
 	// append the filter for "extra attrs"
 	if query != nil && len(query.Keywords) > 0 {
-		for key, value := range query.Keywords {
+		var (
+			key       string
+			keyPrefix string
+			value     interface{}
+		)
+		for key, value = range query.Keywords {
 			if strings.HasPrefix(key, "ExtraAttrs.") {
-				qs = qs.FilterRaw("id", fmt.Sprintf("in (select id from execution where extra_attrs->>'%s'='%s')", strings.TrimPrefix(key, "ExtraAttrs."), value))
+				keyPrefix = "ExtraAttrs."
 				break
 			}
 			if strings.HasPrefix(key, "extra_attrs.") {
-				qs = qs.FilterRaw("id", fmt.Sprintf("in (select id from execution where extra_attrs->>'%s'='%s')", strings.TrimPrefix(key, "extra_attrs."), value))
+				keyPrefix = "extra_attrs."
 				break
 			}
 		}
+		if len(keyPrefix) == 0 {
+			return qs, nil
+		}
+		inClause, err := orm.CreateInClause(ctx, "select id from execution where extra_attrs->>?=?",
+			strings.TrimPrefix(key, keyPrefix), value)
+		if err != nil {
+			return nil, err
+		}
+		qs = qs.FilterRaw("id", inClause)
 	}
 
 	return qs, nil

src/pkg/task/dao/task.go

@@ -16,7 +16,6 @@ package dao
 
 import (
 	"context"
-	"fmt"
 	"strings"
 	"time"
 
@@ -216,16 +215,30 @@ func (t *taskDAO) querySetter(ctx context.Context, query *q.Query) (orm.QuerySet
 
 	// append the filter for "extra attrs"
 	if query != nil && len(query.Keywords) > 0 {
-		for key, value := range query.Keywords {
+		var (
+			key       string
+			keyPrefix string
+			value     interface{}
+		)
+		for key, value = range query.Keywords {
 			if strings.HasPrefix(key, "ExtraAttrs.") {
-				qs = qs.FilterRaw("id", fmt.Sprintf("in (select id from task where extra_attrs->>'%s'='%s')", strings.TrimPrefix(key, "ExtraAttrs."), value))
+				keyPrefix = "ExtraAttrs."
 				break
 			}
 			if strings.HasPrefix(key, "extra_attrs.") {
-				qs = qs.FilterRaw("id", fmt.Sprintf("in (select id from task where extra_attrs->>'%s'='%s')", strings.TrimPrefix(key, "extra_attrs."), value))
+				keyPrefix = "extra_attrs."
 				break
 			}
 		}
+		if len(keyPrefix) == 0 {
+			return qs, nil
+		}
+		inClause, err := orm.CreateInClause(ctx, "select id from task where extra_attrs->>? = ?",
+			strings.TrimPrefix(key, keyPrefix), value)
+		if err != nil {
+			return nil, err
+		}
+		qs = qs.FilterRaw("id", inClause)
 	}
 
 	return qs, nil