Changing the 1 seconds check to 1.5 seconds#199
Conversation
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: kapish <upadhyaykapish@gmail.com>
Signed-off-by: gouravkrosx <gouravgreatkr@gmail.com>
Signed-off-by: gouravkrosx <gouravgreatkr@gmail.com>
Signed-off-by: gouravkrosx <gouravgreatkr@gmail.com>
Signed-off-by: kapish <upadhyaykapish@gmail.com>
Signed-off-by: kapish <upadhyaykapish@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR makes substantial changes to the go-jwt sample application, including migrating the database from PostgreSQL to MySQL, adding a new time-sensitive endpoint, and updating the Docker configuration. The PR title "Changing the 1 seconds check to 1.5 seconds" only reflects a small part of the actual changes.
Changes:
- Added a new
/check-timeendpoint with a 1.5-second tolerance for client-server time synchronization validation - Migrated the database backend from PostgreSQL to MySQL with connection retry logic
- Changed the Docker architecture target from arm64 to amd64
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| go-jwt/main.go | Added CheckTimeHandler endpoint, migrated database connection to MySQL with retry logic, added debug print statements |
| go-jwt/test_time_endpoint.sh | New bash script to test the time-sensitive /check-time endpoint |
| go-jwt/docker-compose.yaml | Replaced PostgreSQL service with MySQL 8.0 configuration |
| go-jwt/go.mod | Replaced PostgreSQL driver dependency with MySQL driver |
| go-jwt/Dockerfile | Changed architecture from arm64 to amd64, added faketime build tag |
| go-jwt/README.md | Updated documentation to reflect MySQL migration, corrected spelling errors, added instructions for testing time endpoint |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if db.Where("username = ?", username).First(&user).RecordNotFound() { | ||
| user = User{Username: username, Password: password, Token: tokenString} | ||
| db.Create(&user) | ||
| fmt.Println("token getting saved :", user) |
There was a problem hiding this comment.
Debug print statements should be removed before merging to production. Use proper logging with log.Printf() if this information is needed for operational purposes.
| ) | ||
|
|
||
| // 6. Check if the difference is greater than 1.5 seconds | ||
| if diff > 1500*time.Millisecond { |
There was a problem hiding this comment.
The PR title states "Changing the 1 seconds check to 1.5 seconds" but this PR contains significantly more changes including: migration from PostgreSQL to MySQL, architecture change from arm64 to amd64, new /check-time endpoint, retry logic for database connections, and documentation updates. The PR title should accurately reflect the scope of changes, or these changes should be split into separate PRs for better reviewability.
|
|
||
| #### Generate testcases | ||
|
|
||
| To genereate testcases we just need to make some API calls. You can use [Postman](https://www.postman.com/), [Hoppscotch](https://hoppscotch.io/), or simply `curl` |
There was a problem hiding this comment.
There's a typo in "genereate" which should be "generate".
| To genereate testcases we just need to make some API calls. You can use [Postman](https://www.postman.com/), [Hoppscotch](https://hoppscotch.io/), or simply `curl` | |
| To generate testcases we just need to make some API calls. You can use [Postman](https://www.postman.com/), [Hoppscotch](https://hoppscotch.io/), or simply `curl` |
| ./test_time_endpoint.sh | ||
| ``` | ||
|
|
||
| This will send a request to the `/check-time` endpoint and you should see a successful response: |
There was a problem hiding this comment.
The documentation is incomplete - the sentence "This will send a request to the /check-time endpoint and you should see a successful response:" appears to be cut off. It should either end with a period or continue with more details about what the response looks like.
| This will send a request to the `/check-time` endpoint and you should see a successful response: | |
| This will send a request to the `/check-time` endpoint and you should see a successful response. |
| dsn := "myuser:mypassword@tcp(localhost:3306)/mydb?charset=utf8&parseTime=True&loc=Local&timeout=60s&readTimeout=60s" | ||
|
|
There was a problem hiding this comment.
The database credentials are hardcoded in the DSN string. While this may be acceptable for a sample/demo application, consider using environment variables for better security and flexibility across different environments. This would allow different configurations for development, testing, and production without code changes.
| dsn := "myuser:mypassword@tcp(localhost:3306)/mydb?charset=utf8&parseTime=True&loc=Local&timeout=60s&readTimeout=60s" | |
| dbUser := os.Getenv("DB_USER") | |
| if dbUser == "" { | |
| dbUser = "myuser" | |
| } | |
| dbPassword := os.Getenv("DB_PASSWORD") | |
| if dbPassword == "" { | |
| dbPassword = "mypassword" | |
| } | |
| dbHost := os.Getenv("DB_HOST") | |
| if dbHost == "" { | |
| dbHost = "localhost" | |
| } | |
| dbPort := os.Getenv("DB_PORT") | |
| if dbPort == "" { | |
| dbPort = "3306" | |
| } | |
| dbName := os.Getenv("DB_NAME") | |
| if dbName == "" { | |
| dbName = "mydb" | |
| } | |
| dsn := fmt.Sprintf("%s:%s@tcp(%s:%s)/%s?charset=utf8&parseTime=True&loc=Local&timeout=60s&readTimeout=60s", | |
| dbUser, dbPassword, dbHost, dbPort, dbName) |
| time.Sleep(1 * time.Second) | ||
|
|
There was a problem hiding this comment.
The time.Sleep call introduces an unconditional 1-second delay in the handler, which will significantly impact performance and user experience. This sleep appears to serve no functional purpose and should be removed. If this was added for testing purposes, it should not be in production code.
| time.Sleep(1 * time.Second) |
| if err := db.DB().Ping(); err == nil { | ||
| break | ||
| } |
There was a problem hiding this comment.
The retry loop doesn't update the error state when ping fails. If gorm.Open succeeds but db.DB().Ping() fails (lines 48-50), the loop continues but connectionErr remains nil. This means the error check at line 57 may pass even though the connection is not functional. The ping error should be captured and assigned to connectionErr.
| if err := db.DB().Ping(); err == nil { | |
| break | |
| } | |
| if pingErr := db.DB().Ping(); pingErr == nil { | |
| break | |
| } | |
| // Treat ping failure as a connection error for retry logic | |
| connectionErr = pingErr |
| c.JSON(http.StatusOK, gin.H{"username": claims.Username}) | ||
| } | ||
|
|
||
| // CheckTimeHandler checks if a client-provided timestamp is within 1 second of the server time. |
There was a problem hiding this comment.
The comment states "within 1 second" but the code now checks for 1.5 seconds (line 198). Update the comment to reflect the actual behavior: "within 1.5 seconds".
| // CheckTimeHandler checks if a client-provided timestamp is within 1 second of the server time. | |
| // CheckTimeHandler checks if a client-provided timestamp is within 1.5 seconds of the server time. |
| username := "example_user" | ||
| password := "example_password" | ||
|
|
||
| fmt.Println("here is the current time :", time.Now().Unix()) |
There was a problem hiding this comment.
Debug print statements should be removed before merging to production. Use proper logging with log.Printf() if this information is needed for operational purposes.
| ADD https://keploy-enterprise.s3.us-west-2.amazonaws.com/releases/latest/assets/go_freeze_time_amd64 /lib/keploy/go_freeze_time_amd64 | ||
|
|
||
| #set suitable permissions | ||
| RUN chmod +x /lib/keploy/go_freeze_time_arm64 | ||
| RUN chmod +x /lib/keploy/go_freeze_time_amd64 | ||
|
|
||
| # run the binary | ||
| RUN /lib/keploy/go_freeze_time_arm64 | ||
| RUN /lib/keploy/go_freeze_time_amd64 |
There was a problem hiding this comment.
This Dockerfile downloads and executes a binary (go_freeze_time_amd64) directly from a remote URL (https://keploy-enterprise.s3.us-west-2.amazonaws.com/releases/latest/assets/go_freeze_time_amd64) during the image build without any integrity verification or pinning to an immutable identifier. If that S3 asset (or the path behind latest) is compromised or replaced, an attacker can run arbitrary code in the build environment and tamper with the resulting image. To mitigate this supply-chain risk, fetch a versioned artifact pinned to an immutable identifier (e.g., content hash or exact version) and/or verify its integrity (checksum or signature) before execution, or vendor the tool into the repository instead of downloading it at build time.
No description provided.