From 7935a2084cdded16401a0acbbd27e5bfd377859e Mon Sep 17 00:00:00 2001
From: Wenkai Yin <yinw@vmware.com>
Date: Wed, 24 Feb 2021 14:11:40 +0800
Subject: [PATCH] Fix sql issue in artifact dao

Fix sql issue in artifact dao

Signed-off-by: Wenkai Yin <yinw@vmware.com>
---
 src/lib/orm/orm.go          | 29 +++++++++++++++++++++++++++++
 src/pkg/artifact/dao/dao.go | 32 ++++++++++++++++++++------------
 2 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/src/lib/orm/orm.go b/src/lib/orm/orm.go
index 91a0eb19f..855d559e1 100644
--- a/src/lib/orm/orm.go
+++ b/src/lib/orm/orm.go
@@ -17,6 +17,9 @@ package orm
 import (
 	"context"
 	"errors"
+	"fmt"
+	"strconv"
+	"strings"
 
 	"github.com/astaxie/beego/orm"
 	"github.com/goharbor/harbor/src/lib/log"
@@ -77,3 +80,29 @@ func WithTransaction(f func(ctx context.Context) error) func(ctx context.Context
 		return nil
 	}
 }
+
+// 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
+}
diff --git a/src/pkg/artifact/dao/dao.go b/src/pkg/artifact/dao/dao.go
index 9c9100bac..3a3347726 100644
--- a/src/pkg/artifact/dao/dao.go
+++ b/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 (
-		SELECT DISTINCT art.id FROM artifact art
-		JOIN tag ON art.id=tag.artifact_id
-		WHERE tag.name LIKE '%%%s%%')`, orm.Escape(f.Value))
-		qs = qs.FilterRaw("id", sql)
+		// get the id list first to avoid the sql injection
+		inClause, err := orm.CreateInClause(ctx, `SELECT DISTINCT art.id FROM artifact art
+			JOIN tag ON art.id=tag.artifact_id
+			WHERE tag.name LIKE ?`, "%"+orm.Escape(f.Value)+"%")
+		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
-		JOIN tag ON art.id=tag.artifact_id
-		WHERE tag.name = '%s')`, orm.Escape(s))
-		qs = qs.FilterRaw("id", sql)
+
+		// get the id list first to avoid the sql injection
+		inClause, err := orm.CreateInClause(ctx, `SELECT DISTINCT art.id FROM artifact art
+			JOIN tag ON art.id=tag.artifact_id
+			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 ")))