Vulnerable sample for Action test#2
Conversation
WalkthroughIntroduces an HTTP server in Go exposing GET /read. The handler reads the query parameter file, loads ./data/, computes its MD5 checksum, and writes the 32-hex digest to the response. The server listens on :8080. File I/O errors are not handled in the handler. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Server as HTTP Server (:8080)
participant FS as Filesystem (./data)
participant MD5 as MD5
Client->>Server: GET /read?file=<name>
Server->>Server: Parse query param "file"
Server->>FS: Read ./data/<file>
FS-->>Server: []byte contents
Server->>MD5: md5.Sum(contents)
MD5-->>Server: 16-byte digest
Server->>Server: Format 32-hex string
Server-->>Client: 200 OK with hex digest
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Comment |
🔐 Secure Code Review (AI)Risk Summary: High (3), Medium (1)
Safeguards Checklist:
This diff is not truncated, but it introduces several critical security issues that need to be addressed to ensure the application is secure. Models can make mistakes. Verify before merging. |
Micro-Learning Topic: Weak cryptographic algorithm (Detected by phrase)Matched on "Weak Cryptographic Hash"As computing power and availability increases, cryptographic algorithms are periodically updated to ensure that these increases do not allow brute force attacks to succeed. Furthermore, ongoing cryptography research will often identify flaws in existing algorithms that weaken their security. Use of weak or outdated algorithms to protect sensitive data places it at risk because encryption weaknesses, hashing or signature algorithms can result in the protection being undone. Try a challenge in Secure Code WarriorHelpful references
Micro-Learning Topic: Denial of service (Detected by phrase)Matched on "denial of service"The Denial of Service (DoS) attack is focused on making a resource (site, application, server) unavailable for the purpose it was designed. There are many ways to make a service unavailable for legitimate users by manipulating network packets, programming, logical, or resources handling vulnerabilities, among others. Source: https://www.owasp.org/index.php/Denial_of_Service Try a challenge in Secure Code WarriorMicro-Learning Topic: Path traversal (Detected by phrase)Matched on "Path Traversal"Path traversal vulnerabilities occur when inputs that have not been sufficiently validated or sanitised are used to build directory or file paths. If an attacker can influence the path being accessed by the server, they may be able to gain unauthorised access to files or even execute arbitrary code on the server (when coupled with file upload functionality). Try a challenge in Secure Code WarriorHelpful references
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
main.go (1)
14-15: Handle server errors and add timeouts to mitigate slowloris.Don’t ignore
ListenAndServeerrors; configure reasonable timeouts.- http.ListenAndServe(":8080", nil) + srv := &http.Server{ + Addr: ":8080", + ReadTimeout: 5 * time.Second, + WriteTimeout: 10 * time.Second, + IdleTimeout: 60 * time.Second, + } + log.Fatal(srv.ListenAndServe())
| import ( | ||
| "crypto/md5" | ||
| "fmt" | ||
| "io/ioutil" | ||
| "net/http" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use non-deprecated APIs and a modern hash.
ioutil is deprecated; MD5 is cryptographically broken. Prefer os + streaming and sha256.
-import (
- "crypto/md5"
- "fmt"
- "io/ioutil"
- "net/http"
-)
+import (
+ "crypto/sha256"
+ "fmt"
+ "io"
+ "log"
+ "net/http"
+ "os"
+ "path/filepath"
+ "strings"
+ "time"
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import ( | |
| "crypto/md5" | |
| "fmt" | |
| "io/ioutil" | |
| "net/http" | |
| ) | |
| import ( | |
| "crypto/sha256" | |
| "fmt" | |
| "io" | |
| "log" | |
| "net/http" | |
| "os" | |
| "path/filepath" | |
| "strings" | |
| "time" | |
| ) |
🤖 Prompt for AI Agents
In main.go around lines 2 to 7, the code imports deprecated ioutil and insecure
crypto/md5; replace them with non-deprecated streaming APIs and a modern hash:
swap "io/ioutil" for "io" (and "os" if reading files from disk), replace
"crypto/md5" with "crypto/sha256", and update the code to stream data into the
hasher (e.g., create sha256.New() and use io.Copy(hasher, source) where source
is an http response body or os.File) instead of reading everything with ReadAll;
ensure you close response bodies and handle errors appropriately.
| http.HandleFunc("/read", func(w http.ResponseWriter, r *http.Request) { | ||
| file := r.URL.Query().Get("file") | ||
| b, _ := ioutil.ReadFile("./data/" + file) | ||
| fmt.Fprintf(w, "%x", md5.Sum(b)) | ||
| }) |
There was a problem hiding this comment.
Block path traversal, handle I/O errors, avoid full-file reads, and set Content-Type.
Untrusted file is concatenated into a path (path traversal). Read errors are ignored, and ReadFile loads entire files (DoS risk). Also returns a digest without declaring a content type.
Apply this safer handler:
- http.HandleFunc("/read", func(w http.ResponseWriter, r *http.Request) {
- file := r.URL.Query().Get("file")
- b, _ := ioutil.ReadFile("./data/" + file)
- fmt.Fprintf(w, "%x", md5.Sum(b))
- })
+ http.HandleFunc("/read", func(w http.ResponseWriter, r *http.Request) {
+ w.Header().Set("Content-Type", "text/plain; charset=utf-8")
+ file := r.URL.Query().Get("file")
+ if file == "" {
+ http.Error(w, "missing 'file' parameter", http.StatusBadRequest)
+ return
+ }
+ base := "./data"
+ baseAbs, err := filepath.Abs(base)
+ if err != nil {
+ http.Error(w, "server error", http.StatusInternalServerError)
+ return
+ }
+ targetAbs, err := filepath.Abs(filepath.Join(base, file))
+ if err != nil || !(targetAbs == baseAbs || strings.HasPrefix(targetAbs, baseAbs+string(os.PathSeparator))) {
+ http.Error(w, "invalid path", http.StatusBadRequest)
+ return
+ }
+ f, err := os.Open(targetAbs)
+ if err != nil {
+ if os.IsNotExist(err) {
+ http.Error(w, "not found", http.StatusNotFound)
+ } else {
+ http.Error(w, "unable to open file", http.StatusInternalServerError)
+ }
+ return
+ }
+ defer f.Close()
+ info, err := f.Stat()
+ if err != nil || info.IsDir() {
+ http.Error(w, "invalid file", http.StatusBadRequest)
+ return
+ }
+ const max = 10 << 20 // 10 MiB
+ if info.Size() > max {
+ http.Error(w, "file too large", http.StatusRequestEntityTooLarge)
+ return
+ }
+ h := sha256.New()
+ if _, err := io.Copy(h, f); err != nil {
+ http.Error(w, "read error", http.StatusInternalServerError)
+ return
+ }
+ fmt.Fprintf(w, "%x", h.Sum(nil))
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| http.HandleFunc("/read", func(w http.ResponseWriter, r *http.Request) { | |
| file := r.URL.Query().Get("file") | |
| b, _ := ioutil.ReadFile("./data/" + file) | |
| fmt.Fprintf(w, "%x", md5.Sum(b)) | |
| }) | |
| http.HandleFunc("/read", func(w http.ResponseWriter, r *http.Request) { | |
| w.Header().Set("Content-Type", "text/plain; charset=utf-8") | |
| file := r.URL.Query().Get("file") | |
| if file == "" { | |
| http.Error(w, "missing 'file' parameter", http.StatusBadRequest) | |
| return | |
| } | |
| base := "./data" | |
| baseAbs, err := filepath.Abs(base) | |
| if err != nil { | |
| http.Error(w, "server error", http.StatusInternalServerError) | |
| return | |
| } | |
| targetAbs, err := filepath.Abs(filepath.Join(base, file)) | |
| if err != nil || !(targetAbs == baseAbs || strings.HasPrefix(targetAbs, baseAbs+string(os.PathSeparator))) { | |
| http.Error(w, "invalid path", http.StatusBadRequest) | |
| return | |
| } | |
| f, err := os.Open(targetAbs) | |
| if err != nil { | |
| if os.IsNotExist(err) { | |
| http.Error(w, "not found", http.StatusNotFound) | |
| } else { | |
| http.Error(w, "unable to open file", http.StatusInternalServerError) | |
| } | |
| return | |
| } | |
| defer f.Close() | |
| info, err := f.Stat() | |
| if err != nil || info.IsDir() { | |
| http.Error(w, "invalid file", http.StatusBadRequest) | |
| return | |
| } | |
| const max = 10 << 20 // 10 MiB | |
| if info.Size() > max { | |
| http.Error(w, "file too large", http.StatusRequestEntityTooLarge) | |
| return | |
| } | |
| h := sha256.New() | |
| if _, err := io.Copy(h, f); err != nil { | |
| http.Error(w, "read error", http.StatusInternalServerError) | |
| return | |
| } | |
| fmt.Fprintf(w, "%x", h.Sum(nil)) | |
| }) |
1 participant
This PR intentionally adds risky patterns for testing ai-secure-code-review-action.
Summary by CodeRabbit