diff --git a/src/common/models/project.go b/src/common/models/project.go index 3bf34a7a5..88ea2b8fc 100644 --- a/src/common/models/project.go +++ b/src/common/models/project.go @@ -24,6 +24,7 @@ import ( "github.com/astaxie/beego/orm" "github.com/goharbor/harbor/src/pkg/quota/types" "github.com/goharbor/harbor/src/replication/model" + "github.com/lib/pq" ) const ( @@ -142,8 +143,13 @@ func (p *Project) FilterByPublic(ctx context.Context, qs orm.QuerySeter, key str } // FilterByOwner returns orm.QuerySeter with owner filter -func (p *Project) FilterByOwner(ctx context.Context, qs orm.QuerySeter, key string, value string) orm.QuerySeter { - return qs.FilterRaw("owner_id", fmt.Sprintf("IN (SELECT user_id FROM harbor_user WHERE username = '%s')", value)) +func (p *Project) FilterByOwner(ctx context.Context, qs orm.QuerySeter, key string, value interface{}) orm.QuerySeter { + username, ok := value.(string) + if !ok { + return qs + } + + return qs.FilterRaw("owner_id", fmt.Sprintf("IN (SELECT user_id FROM harbor_user WHERE username = %s)", pq.QuoteLiteral(username))) } // FilterByMember returns orm.QuerySeter with member filter @@ -152,10 +158,9 @@ func (p *Project) FilterByMember(ctx context.Context, qs orm.QuerySeter, key str if !ok { return qs } - subQuery := `SELECT project_id FROM project_member pm, harbor_user u WHERE pm.entity_id = u.user_id AND pm.entity_type = 'u' AND u.username = '%s'` - subQuery = fmt.Sprintf(subQuery, escape(query.Name)) + subQuery := fmt.Sprintf(`SELECT project_id FROM project_member WHERE entity_id = %d AND entity_type = 'u'`, query.UserID) if query.Role > 0 { - subQuery = fmt.Sprintf("%s AND pm.role = %d", subQuery, query.Role) + subQuery = fmt.Sprintf("%s AND role = %d", subQuery, query.Role) } if query.WithPublic { @@ -187,16 +192,6 @@ func isTrue(i interface{}) bool { } } -func escape(str string) string { - // TODO: remove this function when resolve the cycle import between lib/orm, common/dao and common/models. - // We need to move the function to registry the database from common/dao to lib/database, - // then lib/orm no need to depend the PrepareTestForPostgresSQL method in the common/dao - str = strings.Replace(str, `\`, `\\`, -1) - str = strings.Replace(str, `%`, `\%`, -1) - str = strings.Replace(str, `_`, `\_`, -1) - return str -} - // ProjectQueryParam can be used to set query parameters when listing projects. // The query condition will be set in the query if its corresponding field // is not nil. Leave it empty if you don't want to apply this condition. @@ -220,6 +215,7 @@ type ProjectQueryParam struct { // MemberQuery filter by member's username and role type MemberQuery struct { + UserID int // the user id Name string // the username of member Role int // the role of the member has to the project GroupIDs []int // the group ID of current user belongs to diff --git a/src/lib/orm/query.go b/src/lib/orm/query.go index d1e642db3..db82155a7 100644 --- a/src/lib/orm/query.go +++ b/src/lib/orm/query.go @@ -24,7 +24,6 @@ import ( "github.com/astaxie/beego/orm" "github.com/goharbor/harbor/src/common/dao" "github.com/goharbor/harbor/src/lib/errors" - "github.com/goharbor/harbor/src/lib/log" "github.com/goharbor/harbor/src/lib/q" ) @@ -53,7 +52,7 @@ func ParamPlaceholderForIn(n int) string { // Currently, it supports two ways to generate the query setter, the first one is to generate by the fields of the model, // and the second one is to generate by the methods their name begins with `FilterBy` of the model. // e.g. for the following model the queriable fields are : -// "Field2", "customized_field2", "Field3", "field3", "Field4" (or "field4") and "Field5" (or "field5"). +// "Field2", "customized_field2", "Field3", "field3" and "Field4" (or "field4"). // type Foo struct{ // Field1 string `orm:"-"` // Field2 string `orm:"column(customized_field2)"` @@ -64,11 +63,6 @@ func ParamPlaceholderForIn(n int) string { // // The value is the raw value of key in q.Query // return qs // } -// -// func (f *Foo) FilterByField5(ctx context.Context, qs orm.QuerySeter, key, value string) orm.QuerySeter { -// // The value string is the value of key in q.Query which is escaped by `Escape` -// return qs -// } func QuerySetter(ctx context.Context, model interface{}, query *q.Query, ignoredCols ...string) (orm.QuerySeter, error) { val := reflect.ValueOf(model) if val.Kind() != reflect.Ptr { @@ -177,11 +171,6 @@ func queryByMethod(ctx context.Context, qs orm.QuerySeter, key string, value int switch method := mv.Interface().(type) { case func(context.Context, orm.QuerySeter, string, interface{}) orm.QuerySeter: return method(ctx, qs, key, value) - case func(context.Context, orm.QuerySeter, string, string) orm.QuerySeter: - if str, ok := value.(string); ok { - return method(ctx, qs, key, Escape(str)) - } - log.Warningf("expected string type for the value of method %s, but actual is %T", methodName, value) default: return qs } diff --git a/src/pkg/project/dao/dao_test.go b/src/pkg/project/dao/dao_test.go index 50f29fa70..bb9c367b0 100644 --- a/src/pkg/project/dao/dao_test.go +++ b/src/pkg/project/dao/dao_test.go @@ -57,7 +57,7 @@ func (suite *DaoTestSuite) WithUser(f func(int64, string), usernames ...string) suite.Nil(o.Raw(sql, username, username, email).QueryRow(&userID)) defer func() { - o.Raw("UPDATE harbor_user SET deleted=True WHERE user_id = ?", userID).Exec() + o.Raw("UPDATE harbor_user SET deleted=True, username=concat_ws('#', username, user_id), email=concat_ws('#', email, user_id) WHERE user_id = ?", userID).Exec() }() f(userID, username) @@ -244,26 +244,62 @@ func (suite *DaoTestSuite) TestListByOwner() { suite.Nil(err) suite.Len(projects, 0) } + + { + // single quotes in owner + suite.WithUser(func(userID int64, username string) { + project := &models.Project{ + Name: "project-owner-name-include-single-quotes", + OwnerID: int(userID), + } + projectID, err := suite.dao.Create(orm.Context(), project) + suite.Nil(err) + + defer suite.dao.Delete(orm.Context(), projectID) + + projects, err := suite.dao.List(orm.Context(), q.New(q.KeyWords{"owner": username})) + suite.Nil(err) + suite.Len(projects, 1) + }, "owner include single quotes ' in it") + } + + { + // sql inject + suite.WithUser(func(userID int64, username string) { + project := &models.Project{ + Name: "project-sql-inject", + OwnerID: int(userID), + } + projectID, err := suite.dao.Create(orm.Context(), project) + suite.Nil(err) + + defer suite.dao.Delete(orm.Context(), projectID) + + projects, err := suite.dao.List(orm.Context(), q.New(q.KeyWords{"owner": username})) + suite.Nil(err) + suite.Len(projects, 1) + }, "'owner' OR 1=1") + } } func (suite *DaoTestSuite) TestListByMember() { { // project admin - projects, err := suite.dao.List(orm.Context(), q.New(q.KeyWords{"member": &models.MemberQuery{Name: "admin", Role: common.RoleProjectAdmin}})) + projects, err := suite.dao.List(orm.Context(), q.New(q.KeyWords{"member": &models.MemberQuery{UserID: 1, Role: common.RoleProjectAdmin}})) suite.Nil(err) suite.Len(projects, 1) } { // guest - projects, err := suite.dao.List(orm.Context(), q.New(q.KeyWords{"member": &models.MemberQuery{Name: "admin", Role: common.RoleGuest}})) + projects, err := suite.dao.List(orm.Context(), q.New(q.KeyWords{"member": &models.MemberQuery{UserID: 1, Role: common.RoleGuest}})) suite.Nil(err) suite.Len(projects, 0) } { // guest with public projects - projects, err := suite.dao.List(orm.Context(), q.New(q.KeyWords{"member": &models.MemberQuery{Name: "admin", Role: common.RoleGuest, WithPublic: true}})) + projects, err := suite.dao.List(orm.Context(), q.New(q.KeyWords{"member": &models.MemberQuery{UserID: 1, Role: common.RoleGuest, WithPublic: true}})) suite.Nil(err) suite.Len(projects, 1) } @@ -291,7 +327,7 @@ func (suite *DaoTestSuite) TestListByMember() { defer o.Raw("DELETE FROM project_member WHERE id = ?", pid) memberQuery := &models.MemberQuery{ - Name: "admin", + UserID: 1, Role: common.RoleProjectAdmin, GroupIDs: []int{int(groupID)}, } diff --git a/src/server/v2.0/handler/project.go b/src/server/v2.0/handler/project.go index 225a0f2b6..6caacc82a 100644 --- a/src/server/v2.0/handler/project.go +++ b/src/server/v2.0/handler/project.go @@ -375,7 +375,7 @@ func (a *projectAPI) ListProjects(ctx context.Context, params operation.ListProj if l, ok := secCtx.(*local.SecurityContext); ok { currentUser := l.User() member := &project.MemberQuery{ - Name: currentUser.Username, + UserID: currentUser.UserID, GroupIDs: currentUser.GroupIDs, }