From e06bef9d9b1d1d8f247b83876221d93fd4bdd96d Mon Sep 17 00:00:00 2001 From: mahir Date: Fri, 31 Dec 2021 17:34:09 +0100 Subject: [PATCH] Fixed a major security hole: controllers updated --- README.md | 188 ++++++++++++++++++++------------ controller/auth.go | 14 ++- controller/post.go | 57 ++++++---- controller/user.go | 53 ++++++--- database/migrate/autoMigrate.go | 2 +- database/model/auth.go | 10 +- service/auth.go | 2 +- 7 files changed, 213 insertions(+), 113 deletions(-) diff --git a/README.md b/README.md index 8b12cc01..5f553939 100644 --- a/README.md +++ b/README.md @@ -11,32 +11,44 @@ GoREST is a starter kit, written in [Golang][11] with [Gin framework][12], for rapid prototyping and developing a RESTful API. The source code is released under the [MIT license][13] and is free for any personal or commercial project. +## Updates +v1.3.1 [Dec 31 - 2021] -## Updates +- During the login process, if the provided email is not found, + API should handle it properly +- A user must not be able to modify resources related to other users + (controllers have been updated) v1.3.0 [Dec 28 - 2021] + - refactored config files to reduce cyclomatic complexity - organized instance variables v1.2.7 [Dec 27 - 2021] + - REDIS database driver and test endpoints added - removed ineffectual assignments - check errors during binding of incoming JSON v1.2.6 [Dec 26 - 2021] + - fixed security vulnerability [CWE-190][71] and [CWE-681][72] v1.2.5 [Dec 25 - 2021] + - new endpoint added for refreshing JWT tokens v1.2.4 [Aug 02 - 2021] + - middleware added: `logrus` + `sentry.io` v1.2.3 [Jul 31 - 2021] + - Route handlers modified to meet the requirements of doing unit test v1.2.2 [Jul 29 - 2021] + - Replaced `github.com/dgrijalva/jwt-go` with `github.com/golang-jwt/jwt` Package `github.com/dgrijalva/jwt-go <= v3.2.0` allows attackers to bypass @@ -45,24 +57,26 @@ intended access restrictions in situations with []string{} for m["aud"] More on this: https://github.com/advisories/GHSA-w73w-5m7g-f7qc v1.2.1 [Jun 19 - 2021] + - `SHA-256` is replaced by `Argon2id` for password hashing v1.2.0 [Jun 17 - 2021] + - `GORM` updated from `v1` to `v2` Projects developed based on `GORM v1` must checkout at `v1.1.3` v1.1 [Jan 03 - 2021] + - **PostgreSQL** and **SQLite3** drivers are included - `charset` updated from `utf8` to `utf8mb4` in order to fully support UTF-8 -encoding for MySQL database + encoding for MySQL database v1.0 [Dec 26 - 2020] + - [JWT][14] based authentication is implemented using [dgrijalva/jwt-go][15] - `One-to-one`, `one-to-many`, and `many-to-many` models are introduced - - ## Database Support GoREST uses [GORM][21] as its ORM. GORM supports **SQLite3**, **MySQL**, @@ -72,8 +86,6 @@ In GoREST, **MySQL**, **PostgreSQL** and **SQLite3** drivers are included. Anyone experienced in **Microsoft SQL Server** is welcome to contribute to the project by including **SQL Server** driver and testing all the features of GoREST. - - ## Demo For demonstration, a test instance can be accessed [here][31] from a web @@ -81,6 +93,7 @@ browser. For API development, it is recommended to use [Postman][32] or any other similar tool. Accessible endpoints of the test instance: + - https://goapi.pilinux.me/api/v1/users - https://goapi.pilinux.me/api/v1/users/:id - https://goapi.pilinux.me/api/v1/posts @@ -91,136 +104,185 @@ To prevent abuse, only HTTP `GET` requests are accepted by the demo server. - - ## Setup and start the production-ready app - Install a relational database (MySQL or PostgreSQL) - Set up an environment to compile the Go codes (a [quick tutorial][41] -for any Debian based OS) + for any Debian based OS) - Install `git` - Clone the project `git clone https://github.com/piLinux/GoREST.git` - At the root of the cloned repository -[`cd $GOPATH/src/github.com/pilinux/gorest`], execute `go build` to fetch all -the dependencies + [`cd $GOPATH/src/github.com/pilinux/gorest`], execute `go build` to fetch all + the dependencies - Edit `.env.sample` file and save it as `.env` file at the root of the -project `$GOPATH/src/github.com/pilinux/gorest` + project `$GOPATH/src/github.com/pilinux/gorest` - Edit the `.env.sample` file located at -`$GOPATH/src/github.com/pilinux/gorest/database/migrate` and save it as `.env` + `$GOPATH/src/github.com/pilinux/gorest/database/migrate` and save it as `.env` - Inside `$GOPATH/src/github.com/pilinux/gorest/database/migrate`, run -`go run autoMigrate.go` to migrate the database + `go run autoMigrate.go` to migrate the database - Comment the line `setPkFk()` in `autoMigrate.go` file if the driver is not **MySQL**. - [Check issue: 7][42] + [Check issue: 7][42] - At `$GOPATH/src/github.com/pilinux/gorest`, run `./gorest` to launch the app **Note For SQLite3:** + - `DBUSER`, `DBPASS`, `DBHOST` and `DBPORT` environment variables -should be left unchanged. + should be left unchanged. - `DBNAME` must contain the full path and the database file name; i.e, + ``` /user/location/database.db ``` To the following endpoints `GET`, `POST`, `PUT` and `DELETE` requests can be sent: -- http://localhost:port/api/v1/register - - `POST` [create new account] +### Register + +http://localhost:port/api/v1/register + +- `POST` [create new account] + ``` { "Email":"...@example.com", "Password":"..." } ``` -- http://localhost:port/api/v1/login - - `POST` [generate new JWT] + +### Login + +http://localhost:port/api/v1/login + +- `POST` [generate new JWT] + ``` { "Email":"...@example.com", "Password":"..." } ``` -- http://localhost:port/api/v1/refresh - - `POST` [generate new JWT] + +### Refresh JWT + +http://localhost:port/api/v1/refresh + +- `POST` [generate new JWT] + ``` { "RefreshJWT":"use_existing_valid_refresh_token" } ``` -- http://localhost:port/api/v1/users - - `GET` [get list of all registered users along with their hobbies and posts] - - `POST` [add user info to the database, requires JWT for verification] + +### User profile + +http://localhost:port/api/v1/users + +- `GET` [get list of all registered users along with their hobbies and posts] +- `POST` [add user info to the database, requires JWT for verification] + ``` { "FirstName": "...", "LastName": "..." } ``` - - `PUT` [edit user info, requires JWT for verification] + +- `PUT` [edit user info, requires JWT for verification] + ``` { "FirstName": "...", "LastName": "..." } ``` -- http://localhost:port/api/v1/users/:id - - `GET` [fetch hobbies and posts belonged to a specific user] -- http://localhost:port/api/v1/users/hobbies - - `PUT` [add a new hobby, requires JWT for verification] + +### Hobbies of a user + +http://localhost:port/api/v1/users/:id + +- `GET` [fetch hobbies and posts belonged to a specific user] + +http://localhost:port/api/v1/users/hobbies + +- `PUT` [add a new hobby, requires JWT for verification] + ``` { "Hobby": "..." } ``` -- http://localhost:port/api/v1/posts - - `GET` [fetch all published posts] - - `POST` [create a new post, requires JWT for verification] + +### Posts + +http://localhost:port/api/v1/posts + +- `GET` [fetch all published posts] +- `POST` [create a new post, requires JWT for verification] + ``` { "Title": "...", "Body": "... ..." } ``` -- http://localhost:port/api/v1/posts/:id - - `GET` [fetch a specific post] - - `PUT` [edit a specific post, requires JWT for verification] + +##### Any specific post + +http://localhost:port/api/v1/posts/:id + +- `GET` [fetch a specific post] +- `PUT` [edit a specific post, requires JWT for verification] + ``` { "Title": "...", "Body": "... ..." } ``` - - `DELETE` [delete a specific post, requires JWT for verification] -- http://localhost:port/api/v1/hobbies - - `GET` [fetch all hobbies created by all users] +- `DELETE` [delete a specific post, requires JWT for verification] -### For REDIS +### List of hobbies available in the database + +http://localhost:port/api/v1/hobbies + +- `GET` [fetch all hobbies created by all users] + +## For REDIS - Set environment variable `ACTIVATE_REDIS=yes` - Set `key:value` pair - `POST` http://localhost:port/api/v1/playground/redis_create + ``` { "Key": "test1", "Value": "v1" } ``` + - Fetch `key:value` pair - `GET` http://localhost:port/api/v1/playground/redis_read + ``` { "Key": "test1" } ``` + - Delete `key:value` pair - `DELETE` http://localhost:port/api/v1/playground/redis_delete + ``` { "Key": "test1" } ``` + - Set hashes with key - `POST` http://localhost:port/api/v1/playground/redis_create_hash + ``` { "Key": "test2", @@ -233,44 +295,44 @@ To the following endpoints `GET`, `POST`, `PUT` and `DELETE` requests can be sen } } ``` + - Fetch hashes by key - `GET` http://localhost:port/api/v1/playground/redis_read_hash + ``` { "Key": "test2" } ``` + - Delete a key - `DELETE` http://localhost:port/api/v1/playground/redis_delete_hash + ``` { "Key": "test2" } ``` - - ## Flow diagram ![Flow.Diagram][05] - - ## Features - GoREST uses [Gin][12] as the main framework, [GORM][21] as the ORM and -[GoDotEnv][51] for environment configuration + [GoDotEnv][51] for environment configuration - [golang-jwt/jwt][16] is used for JWT authentication - [sentry.io][17] error tracker and performance monitor is enabled by default -as a hook inside `logrus`. They are included as middleware which can be -disabled by omitting + as a hook inside `logrus`. They are included as middleware which can be + disabled by omitting ``` router.Use(middleware.SentryCapture(configure.Logger.SentryDsn)) ``` -- All codes are written and organized following a straightforward and -easy-to-understand approach +- All codes are written and organized following a straightforward and + easy-to-understand approach - For **Logger** and **Recovery**, Gin's in-built middlewares are used ``` @@ -288,14 +350,10 @@ router.Use(middleware.CORS()) - `one to many` - `many to many` - - ## Logical Database Model ![DB.Model.Logical][04] - - ## Architecture ### List of files @@ -390,22 +448,22 @@ gorest ### Step 1 - `model`: This package contains all the necessary models. Each file is -responsible for one specific table in the database. To add new tables and to -create new relations between those tables, create new models, and place them in -this directory. All newly created files should have the same package name. + responsible for one specific table in the database. To add new tables and to + create new relations between those tables, create new models, and place them in + this directory. All newly created files should have the same package name. ### Step 2 - `controller`: This package contains all functions to process all related -incoming HTTP requests. + incoming HTTP requests. ### Step 3 - `autoMigrate.go`: Names of all newly added models should first be included -in this file to automatically create the complete database. It also contains -the function to delete the previous data and tables. When only newly created -tables or columns need to be migrated, first disable `db.DropTableIfExists()` -function before executing the file. + in this file to automatically create the complete database. It also contains + the function to delete the previous data and tables. When only newly created + tables or columns need to be migrated, first disable `db.DropTableIfExists()` + function before executing the file. ### Step 4 @@ -423,28 +481,20 @@ v1 := router.Group() } ``` - - ## Contributing Please see [CONTRIBUTING][61] to join this amazing project. - - ## Code of conduct Please see [this][62] document. - - ## License © Mahir Hasan 2019 - 2022 Released under the [MIT license][13] - - [01]: https://goreportcard.com/report/github.com/piLinux/GoREST [03]: https://codebeat.co/projects/github-com-pilinux-gorest-master [04]: https://cdn.pilinux.workers.dev/images/GoREST/models/dbModelv1.0.svg diff --git a/controller/auth.go b/controller/auth.go index eb3910dc..8ed73fea 100644 --- a/controller/auth.go +++ b/controller/auth.go @@ -15,30 +15,38 @@ import ( func CreateUserAuth(c *gin.Context) { db := database.GetDB() auth := model.Auth{} + authFinal := model.Auth{} + // bind JSON if err := c.ShouldBindJSON(&auth); err != nil { render(c, gin.H{"msg": "bad request"}, http.StatusBadRequest) return } + // email validation if !service.IsEmailValid(auth.Email) { render(c, gin.H{"msg": "wrong email address"}, http.StatusBadRequest) return } + // email must be unique if err := db.Where("email = ?", auth.Email).First(&auth).Error; err == nil { - render(c, gin.H{"msg": "email already registered"}, http.StatusBadRequest) + render(c, gin.H{"msg": "email already registered"}, http.StatusForbidden) return } + // user must not be able to manipulate all fields + authFinal.Email = auth.Email + authFinal.Password = auth.Password + // one unique email for each account tx := db.Begin() - if err := tx.Create(&auth).Error; err != nil { + if err := tx.Create(&authFinal).Error; err != nil { tx.Rollback() log.WithError(err).Error("error code: 1001") render(c, gin.H{"msg": "internal server error"}, http.StatusInternalServerError) } else { tx.Commit() - render(c, auth, http.StatusCreated) + render(c, authFinal, http.StatusCreated) } } diff --git a/controller/post.go b/controller/post.go index 5f1a2a05..d77da349 100644 --- a/controller/post.go +++ b/controller/post.go @@ -2,6 +2,7 @@ package controller import ( "net/http" + "time" "github.com/pilinux/gorest/database" "github.com/pilinux/gorest/database/model" @@ -41,29 +42,35 @@ func CreatePost(c *gin.Context) { db := database.GetDB() user := model.User{} post := model.Post{} + postFinal := model.Post{} - user.IDAuth = middleware.AuthID + userIDAuth := middleware.AuthID - if err := db.Where("id_auth = ?", user.IDAuth).First(&user).Error; err != nil { - render(c, gin.H{"msg": "bad request"}, http.StatusBadRequest) + // does the user have an existing profile + if err := db.Where("id_auth = ?", userIDAuth).First(&user).Error; err != nil { + render(c, gin.H{"msg": "no user profile found"}, http.StatusForbidden) return } + // bind JSON if err := c.ShouldBindJSON(&post); err != nil { render(c, gin.H{"msg": "bad request"}, http.StatusBadRequest) return } - post.IDUser = user.UserID + // user must not be able to manipulate all fields + postFinal.Title = post.Title + postFinal.Body = post.Body + postFinal.IDUser = user.UserID tx := db.Begin() - if err := tx.Create(&post).Error; err != nil { + if err := tx.Create(&postFinal).Error; err != nil { tx.Rollback() log.WithError(err).Error("error code: 1201") render(c, gin.H{"msg": "internal server error"}, http.StatusInternalServerError) } else { tx.Commit() - render(c, post, http.StatusCreated) + render(c, postFinal, http.StatusCreated) } } @@ -72,33 +79,42 @@ func UpdatePost(c *gin.Context) { db := database.GetDB() user := model.User{} post := model.Post{} + postFinal := model.Post{} id := c.Params.ByName("id") - user.IDAuth = middleware.AuthID + userIDAuth := middleware.AuthID - if err := db.Where("id_auth = ?", user.IDAuth).First(&user).Error; err != nil { - render(c, gin.H{"msg": "not found"}, http.StatusNotFound) + // does the user have an existing profile + if err := db.Where("id_auth = ?", userIDAuth).First(&user).Error; err != nil { + render(c, gin.H{"msg": "no user profile found"}, http.StatusForbidden) return } - if err := db.Where("post_id = ?", id).Where("id_user = ?", user.UserID).First(&post).Error; err != nil { - render(c, gin.H{"msg": "not found"}, http.StatusNotFound) + // does the post exist + does user have right to modify this post + if err := db.Where("post_id = ?", id).Where("id_user = ?", user.UserID).First(&postFinal).Error; err != nil { + render(c, gin.H{"msg": "operation not possible"}, http.StatusForbidden) return } + // bind JSON if err := c.ShouldBindJSON(&post); err != nil { render(c, gin.H{"msg": "bad request"}, http.StatusBadRequest) return } + // user must not be able to manipulate all fields + postFinal.UpdatedAt = time.Now() + postFinal.Title = post.Title + postFinal.Body = post.Body + tx := db.Begin() - if err := tx.Save(&post).Error; err != nil { + if err := tx.Save(&postFinal).Error; err != nil { tx.Rollback() log.WithError(err).Error("error code: 1211") render(c, gin.H{"msg": "internal server error"}, http.StatusInternalServerError) } else { tx.Commit() - render(c, post, http.StatusOK) + render(c, postFinal, http.StatusOK) } } @@ -109,20 +125,17 @@ func DeletePost(c *gin.Context) { post := model.Post{} id := c.Params.ByName("id") - user.IDAuth = middleware.AuthID + userIDAuth := middleware.AuthID - if err := db.Where("id_auth = ?", user.IDAuth).First(&user).Error; err != nil { - render(c, gin.H{"msg": "not found"}, http.StatusNotFound) + // does the user have an existing profile + if err := db.Where("id_auth = ?", userIDAuth).First(&user).Error; err != nil { + render(c, gin.H{"msg": "no user profile found"}, http.StatusForbidden) return } + // does the post exist + does user have right to delete this post if err := db.Where("post_id = ?", id).Where("id_user = ?", user.UserID).First(&post).Error; err != nil { - render(c, gin.H{"msg": "not found"}, http.StatusNotFound) - return - } - - if err := c.ShouldBindJSON(&post); err != nil { - render(c, gin.H{"msg": "bad request"}, http.StatusBadRequest) + render(c, gin.H{"msg": "operation not possible"}, http.StatusForbidden) return } diff --git a/controller/user.go b/controller/user.go index 6160cb4c..4b127172 100644 --- a/controller/user.go +++ b/controller/user.go @@ -2,6 +2,7 @@ package controller import ( "net/http" + "time" "github.com/pilinux/gorest/database" "github.com/pilinux/gorest/database/model" @@ -62,27 +63,35 @@ func GetUser(c *gin.Context) { func CreateUser(c *gin.Context) { db := database.GetDB() user := model.User{} + userFinal := model.User{} - user.IDAuth = middleware.AuthID + userIDAuth := middleware.AuthID - if err := db.Where("id_auth = ?", user.IDAuth).First(&user).Error; err == nil { - render(c, gin.H{"msg": "bad request"}, http.StatusBadRequest) + // does the user have an existing profile + if err := db.Where("id_auth = ?", userIDAuth).First(&userFinal).Error; err == nil { + render(c, gin.H{"msg": "user profile found, no need to create a new one"}, http.StatusForbidden) return } + // bind JSON if err := c.ShouldBindJSON(&user); err != nil { render(c, gin.H{"msg": "bad request"}, http.StatusBadRequest) return } + // user must not be able to manipulate all fields + userFinal.FirstName = user.FirstName + userFinal.LastName = user.LastName + userFinal.IDAuth = userIDAuth + tx := db.Begin() - if err := tx.Create(&user).Error; err != nil { + if err := tx.Create(&userFinal).Error; err != nil { tx.Rollback() log.WithError(err).Error("error code: 1101") render(c, gin.H{"msg": "internal server error"}, http.StatusInternalServerError) } else { tx.Commit() - render(c, user, http.StatusCreated) + render(c, userFinal, http.StatusCreated) } } @@ -90,27 +99,35 @@ func CreateUser(c *gin.Context) { func UpdateUser(c *gin.Context) { db := database.GetDB() user := model.User{} + userFinal := model.User{} - user.IDAuth = middleware.AuthID + userIDAuth := middleware.AuthID - if err := db.Where("id_auth = ?", user.IDAuth).First(&user).Error; err != nil { - render(c, gin.H{"msg": "not found"}, http.StatusNotFound) + // does the user have an existing profile + if err := db.Where("id_auth = ?", userIDAuth).First(&userFinal).Error; err != nil { + render(c, gin.H{"msg": "no user profile found"}, http.StatusNotFound) return } + // bind JSON if err := c.ShouldBindJSON(&user); err != nil { render(c, gin.H{"msg": "bad request"}, http.StatusBadRequest) return } + // user must not be able to manipulate all fields + userFinal.UpdatedAt = time.Now() + userFinal.FirstName = user.FirstName + userFinal.LastName = user.LastName + tx := db.Begin() - if err := tx.Save(&user).Error; err != nil { + if err := tx.Save(&userFinal).Error; err != nil { tx.Rollback() log.WithError(err).Error("error code: 1111") render(c, gin.H{"msg": "internal server error"}, http.StatusInternalServerError) } else { tx.Commit() - render(c, user, http.StatusOK) + render(c, userFinal, http.StatusOK) } } @@ -119,27 +136,31 @@ func AddHobby(c *gin.Context) { db := database.GetDB() user := model.User{} hobby := model.Hobby{} + hobbyNew := model.Hobby{} hobbyFound := 0 // default (do not create new hobby) = 0, create new hobby = 1 - user.IDAuth = middleware.AuthID + userIDAuth := middleware.AuthID - if err := db.Where("id_auth = ?", user.IDAuth).First(&user).Error; err != nil { - render(c, gin.H{"msg": "not found"}, http.StatusNotFound) + // does the user have an existing profile + if err := db.Where("id_auth = ?", userIDAuth).First(&user).Error; err != nil { + render(c, gin.H{"msg": "no user profile found"}, http.StatusForbidden) return } + // bind JSON if err := c.ShouldBindJSON(&hobby); err != nil { render(c, gin.H{"msg": "bad request"}, http.StatusBadRequest) return } - if err := db.First(&hobby, "hobby = ?", hobby.Hobby).Error; err != nil { + if err := db.Where("hobby = ?", hobby.Hobby).First(&hobbyNew).Error; err != nil { hobbyFound = 1 // create new hobby } if hobbyFound == 1 { + hobbyNew.Hobby = hobby.Hobby tx := db.Begin() - if err := tx.Create(&hobby).Error; err != nil { + if err := tx.Create(&hobbyNew).Error; err != nil { tx.Rollback() log.WithError(err).Error("error code: 1121") render(c, gin.H{"msg": "internal server error"}, http.StatusInternalServerError) @@ -150,7 +171,7 @@ func AddHobby(c *gin.Context) { } if hobbyFound == 0 { - user.Hobbies = append(user.Hobbies, hobby) + user.Hobbies = append(user.Hobbies, hobbyNew) tx := db.Begin() if err := tx.Save(&user).Error; err != nil { tx.Rollback() diff --git a/database/migrate/autoMigrate.go b/database/migrate/autoMigrate.go index 76ed8e51..efa0e284 100644 --- a/database/migrate/autoMigrate.go +++ b/database/migrate/autoMigrate.go @@ -95,7 +95,7 @@ func migrateTables() { func setPkFk() { // Manually set foreign key for MySQL and PostgreSQL - if err := db.Migrator().CreateConstraint(&auth{}, "Users"); err != nil { + if err := db.Migrator().CreateConstraint(&auth{}, "User"); err != nil { errorState = 1 fmt.Println(err) } diff --git a/database/model/auth.go b/database/model/auth.go index 6f0849c5..0c5c1a52 100644 --- a/database/model/auth.go +++ b/database/model/auth.go @@ -20,7 +20,7 @@ type Auth struct { DeletedAt gorm.DeletedAt `gorm:"index"` Email string `json:"Email"` Password string `json:"Password"` - Users User `gorm:"foreignkey:IDAuth;references:AuthID;constraint:OnUpdate:CASCADE,OnDelete:CASCADE"` + User User `gorm:"foreignkey:IDAuth;references:AuthID;constraint:OnUpdate:CASCADE,OnDelete:CASCADE"` } // UnmarshalJSON ... @@ -33,6 +33,14 @@ func (v *Auth) UnmarshalJSON(b []byte) error { if err := json.Unmarshal(b, &aux); err != nil { return err } + + // check password length + // if more checks are required i.e. password pattern, + // add all conditions here + if len(aux.Password) < 6 { + return errors.New("short password") + } + v.AuthID = aux.AuthID v.Email = aux.Email if v.Password = HashPass(aux.Password); v.Password == "error" { diff --git a/service/auth.go b/service/auth.go index cd3275d9..26d8853f 100644 --- a/service/auth.go +++ b/service/auth.go @@ -11,7 +11,7 @@ func GetUserByEmail(email string) (*model.Auth, error) { var auth model.Auth - if err := db.Where("email = ? ", email).Find(&auth).Error; err != nil { + if err := db.Where("email = ? ", email).First(&auth).Error; err != nil { return nil, err }