Implement JWT verification, rate limiting, and handlers with Docker support #15
Conversation
Wiz Scan Summary
|
There was a problem hiding this comment.
Hello!!
We really appreciated the effort you put into this assignment. 😃
We loved the idea that you added detailed tests and a rate limiter.
We made some comments to spark some discussions around your implementation. Feel free to respond to some or all of them.
|
|
||
| COPY . . | ||
|
|
||
| ENV JWT_SECRET=your_jwt_secret |
There was a problem hiding this comment.
You are going to pass a secret as an environment variable. How would you approach it in a production system?
There was a problem hiding this comment.
Good catch, on way its to use a secret management service such as AWS Secrets Manager or to inject environment variables at runtime using orchestration tools like Kubernetes
|
|
||
| ENV JWT_SECRET=your_jwt_secret | ||
|
|
||
| RUN go test ./... |
There was a problem hiding this comment.
How would you incorporate your tests or your testing suite in general into your daily routine?
There was a problem hiding this comment.
Using CI tools like Github actions
|
|
||
| ## Endpoints | ||
|
|
||
| - `POST /login`: Authenticates a user and returns a JWT token. |
There was a problem hiding this comment.
How would you communicate a change in one of these endpoints to the team that consumes your API?
There was a problem hiding this comment.
I would update the API documentation, notify the external team, and possibly ask if they would like to setup a meeting to ensure everyone understands the update
| go 1.22.3 | ||
|
|
||
| require ( | ||
| github.com/dgrijalva/jwt-go v3.2.0+incompatible |
There was a problem hiding this comment.
Is this library considered safe?
There was a problem hiding this comment.
No, I prioritised minimising development time for this assignment over implementing robust security measures, knowing that the code would not be deployed in a production environment.
| return | ||
| } | ||
|
|
||
| UsersFavorites[userID] = append(UsersFavorites[userID], asset) |
There was a problem hiding this comment.
No, this is not safe as it lacks concurrency control, which can lead to race conditions in a multi-threaded environment.
| - `GET /favorites/{userID}`: Retrieves a list of favorite assets for a user. | ||
| - `POST /favorites/{userID}`: Adds a new favorite asset for a user. | ||
| - `DELETE /favorites/{userID}/{assetID}`: Removes a favorite asset for a user. | ||
| - `PUT /favorites/{userID}/{assetID}`: Edits a favorite asset for a user. |
There was a problem hiding this comment.
Is userId safe to be url param ?
There was a problem hiding this comment.
No, passing userID as a URL parameter can expose it to unauthorised access and potential security risks, its safer to extract it from a secure token
| func EditFavorite(w http.ResponseWriter, r *http.Request) { | ||
| vars := mux.Vars(r) | ||
| userID := vars["userID"] | ||
| assetID := vars["assetID"] |
There was a problem hiding this comment.
How could a validator be useful here? What types of validations could we do?
There was a problem hiding this comment.
We could validate that userID and assetID are correctly formatted, exist in the database, and the user has permission to modify the specified asset
| return | ||
| } | ||
|
|
||
| http.SetCookie(w, &http.Cookie{ |
There was a problem hiding this comment.
Why did you choose http.SetCookie instead of a typical HTTP response?
There was a problem hiding this comment.
I chose http.SetCookie to securely store the JWT token in the client's browser, so it is automatically included in subsequent requests, making local testing easier
| "github.com/meletios/gwi-engineering-challenge/models" | ||
| ) | ||
|
|
||
| var UsersFavorites = make(map[string][]models.Asset) |
There was a problem hiding this comment.
I believe this database is coupled with this layer. Do you agree with that statement?
There was a problem hiding this comment.
Yes, I agree that the UsersFavorites map is coupled with this layer, making it difficult to decouple and test independently.
No description provided.