Merged
Conversation
WalkthroughAdds a new custom TCP-based HTTP server, a response writer utility, header management methods, and a CLI entrypoint. The server parses requests, dispatches to a handler, and writes responses. The handler serves HTML, a video file, or proxies /httpbin/* with manual chunked streaming and trailers. Graceful shutdown is supported. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Server as Server.ListenAndServe
participant Listener
participant Conn as Server.HandleConn
participant Parser as Request Parser
participant Handler as Custom Handler (main.go)
participant Resp as response.Writer
Client->>Server: Start server on :4000
Server->>Listener: Bind & accept loop
loop Incoming connections
Listener-->>Client: Accept TCP
Listener->>Conn: New connection
Conn->>Parser: Parse HTTP request
alt Parse error
Conn->>Resp: Write 400 + default headers
Resp-->>Client: 400 response
else OK
Conn->>Handler: Handle(r, w)
Handler->>Resp: Write status/headers/body
Resp-->>Client: Response stream
end
end
sequenceDiagram
autonumber
participant Client
participant Handler as /httpbin/* Handler
participant Remote as https://httpbin.org
participant Resp as response.Writer
Client->>Handler: GET /httpbin/...
Handler->>Remote: Forward request
alt Remote error
Handler->>Resp: 500 + HTML
Resp-->>Client: 500 response
else Success
note over Handler,Resp: Stream body in 30-byte chunks<br/>Write chunk sizes (hex), then 0-length chunk
Handler->>Resp: Status + headers (Transfer-encoding: chunked)
loop Body chunks
Remote-->>Handler: Read bytes
Handler->>Resp: Write chunk-size + chunk
end
Handler->>Resp: Final 0-chunk + trailers<br/>(X-Content-SHA256, X-Content-length)
Resp-->>Client: Chunked response complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (12)
internal/headers/headers.go (1)
84-91: Optional: include a space after comma when coalescing duplicate headersImproves readability and aligns with common header formatting.
- h.headers[fieldName] = fmt.Sprintf("%s,%s", v, value) + h.headers[fieldName] = fmt.Sprintf("%s, %s", v, value)Note: update tests that assert exact string values.
cmd/httpServer/main.go (4)
3-18: Missing import for io after switching to streamingAdd io to imports.
import ( "crypto/sha256" + "io" "fmt"
103-106: Check and handle write errorsIgnoring write errors can silently corrupt responses under backpressure or disconnects.
- Capture and log/abort on errors from WriteStatusLine, WriteHeaders, and WriteBody.
Also applies to: 121-127, 138-141, 156-159
22-29: Optional: simplify toStrUse fmt or encoding/hex; less allocation and clearer intent.
-func toStr(data []byte) string { - builder := strings.Builder{} - for _, b := range data { - builder.WriteString(fmt.Sprintf("%02x", b)) - } - return builder.String() -} +func toStr(data []byte) string { + return fmt.Sprintf("%x", data) +}
31-44: User-facing copy is snarkyConsider neutral, professional error messages for production.
Also applies to: 46-59, 61-74
internal/server/server.go (2)
24-36: Optional: add panic recovery and (if net.Conn) deadlines in HandleConnPrevents a bad handler from killing the goroutine; timeouts avoid hanging reads/writes.
func (s *Server) HandleConn(conn io.ReadWriteCloser) { - defer conn.Close() + defer conn.Close() + defer func() { _ = recover() }()Additionally (not shown): type-assert
net.Connand set read/write deadlines per request.
19-22: HandlerErr is unusedRemove until needed to avoid dead code.
internal/response/response.go (5)
66-80: Remove duplication: delegate to the top-level status writer.Keeps behavior in one place and inherits the unsupported-code guard.
Apply:
func (w *Writer) WriteStatusLine(statusCode StatusCode) error { - statusLine := []byte{} - - switch statusCode { - case StatusOK: - statusLine = []byte("HTTP/1.1 200 OK\r\n") - case StatusBadRequest: - statusLine = []byte("HTTP/1.1 400 Bad Request\r\n") - case StatusInternalServerError: - statusLine = []byte("HTTP/1.1 500 Internal Server Error\r\n") - } - - _, err := w.Writer.Write(statusLine) - return err + return WriteStatusLine(w.Writer, statusCode) }
82-90: Remove duplication: delegate to the top-level headers writer.Single source of truth for validation and formatting.
Apply:
func (w *Writer) WriteHeaders(h *headers.Headers) error { - b := []byte{} - h.ForEach(func(n, v string) { - b = fmt.Appendf(b, "%s: %s\r\n", n, v) - }) - b = fmt.Append(b, "\r\n") - _, err := w.Writer.Write(b) - return err + return WriteHeaders(w.Writer, h) }
18-20: Remove or implement Response struct.It’s unused; drop it or make it carry status, headers, and body for higher-level composition.
56-63: Avoid exporting the inner writer field.Expose methods only or embed io.Writer anonymously. An exported field invites misuse.
Possible change (may require call-site updates):
-type Writer struct { - Writer io.Writer -} +type Writer struct{ io.Writer } func NewWriter(writer io.Writer) *Writer { - return &Writer{ - Writer: writer, - } + return &Writer{Writer: writer} }
37-45: Consider canonicalizing header names.Use textproto.CanonicalMIMEHeaderKey when rendering to normalize casing consistently.
If you prefer to keep your custom headers type, call:
name := textproto.CanonicalMIMEHeaderKey(n)before formatting.
Also applies to: 82-90
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
assets/vim.mp4is excluded by!**/*.mp4
📒 Files selected for processing (4)
cmd/httpServer/main.go(1 hunks)internal/headers/headers.go(2 hunks)internal/response/response.go(1 hunks)internal/server/server.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
internal/server/server.go (2)
internal/response/response.go (7)
Writer(56-58)StatusCode(10-10)NewWriter(60-64)WriteStatusLine(21-35)StatusBadRequest(14-14)WriteHeaders(37-45)GetDefaultHeaders(47-54)internal/request/request.go (2)
Request(31-38)RequestFromReader(162-184)
cmd/httpServer/main.go (4)
internal/server/server.go (1)
ListenAndServe(53-67)internal/response/response.go (7)
Writer(56-58)GetDefaultHeaders(47-54)StatusOK(13-13)StatusBadRequest(14-14)WriteStatusLine(21-35)WriteHeaders(37-45)StatusInternalServerError(15-15)internal/request/request.go (2)
Request(31-38)RequestLine(25-29)internal/headers/headers.go (1)
NewHeaders(43-47)
internal/headers/headers.go (1)
internal/headers/headers_test.go (1)
TestHeaderParse(10-52)
internal/response/response.go (1)
internal/headers/headers.go (2)
Headers(39-41)NewHeaders(43-47)
🔇 Additional comments (4)
internal/headers/headers.go (3)
75-82: Get: case-insensitive lookup looks correctNo functional issues spotted.
93-96: Replace: simple, correct overwriteGood addition; matches intended semantics.
98-101: Delete: correct case-insensitive removalLooks good.
internal/response/response.go (1)
47-54: Fix default headers formatting: update Content-Length header to canonical casing without trailing CRLF and include charset in Content-Type. Verified this helper isn’t used with Transfer-Encoding: chunked.
Comment on lines
+97
to
+113
| if target == "/video" { | ||
| slog.Info("jandler", "target", target) | ||
|
|
||
| f, _ := os.ReadFile("assets/vim.mp4") | ||
| h.Replace("Content-type", "video/mp4") | ||
| h.Replace("Content-length", fmt.Sprintf("%d", len(f))) | ||
| w.WriteStatusLine(status) | ||
| w.WriteHeaders(h) | ||
| w.WriteBody(f) | ||
|
|
||
| // file, _ := os.Open("./assets/vim.mp4") | ||
| // stat, _ := file.Stat() | ||
| // h.Replace("Content-type", "video/mp4") | ||
| // h.Replace("Content-length", fmt.Sprintf("%d", stat.Size())) | ||
| // w.WriteHeaders(h) | ||
| // io.Copy(w.Writer, file) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
/video path: double response + ignored file I/O errors; stream and return
- You write a full response, then fall through and write another one (protocol corruption).
os.ReadFileloads the entire video into memory and ignores errors.
Fix by streaming with os.Open, handle errors, and return after serving:
- if target == "/video" {
- slog.Info("jandler", "target", target)
-
- f, _ := os.ReadFile("assets/vim.mp4")
- h.Replace("Content-type", "video/mp4")
- h.Replace("Content-length", fmt.Sprintf("%d", len(f)))
- w.WriteStatusLine(status)
- w.WriteHeaders(h)
- w.WriteBody(f)
- // file, _ := os.Open("./assets/vim.mp4")
- // stat, _ := file.Stat()
- // h.Replace("Content-type", "video/mp4")
- // h.Replace("Content-length", fmt.Sprintf("%d", stat.Size()))
- // w.WriteHeaders(h)
- // io.Copy(w.Writer, file)
- }
+ if target == "/video" {
+ slog.Info("handler", "target", target)
+ file, err := os.Open("assets/vim.mp4")
+ if err != nil {
+ status = response.StatusInternalServerError
+ body = respond500()
+ } else {
+ defer file.Close()
+ stat, err := file.Stat()
+ if err != nil {
+ status = response.StatusInternalServerError
+ body = respond500()
+ } else {
+ h.Replace("Content-type", "video/mp4")
+ h.Replace("Content-length", fmt.Sprintf("%d", stat.Size()))
+ _ = w.WriteStatusLine(status)
+ _ = w.WriteHeaders(h)
+ _, _ = io.Copy(w.Writer, file)
+ return
+ }
+ }
+ }📝 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.
Suggested change
| if target == "/video" { | |
| slog.Info("jandler", "target", target) | |
| f, _ := os.ReadFile("assets/vim.mp4") | |
| h.Replace("Content-type", "video/mp4") | |
| h.Replace("Content-length", fmt.Sprintf("%d", len(f))) | |
| w.WriteStatusLine(status) | |
| w.WriteHeaders(h) | |
| w.WriteBody(f) | |
| // file, _ := os.Open("./assets/vim.mp4") | |
| // stat, _ := file.Stat() | |
| // h.Replace("Content-type", "video/mp4") | |
| // h.Replace("Content-length", fmt.Sprintf("%d", stat.Size())) | |
| // w.WriteHeaders(h) | |
| // io.Copy(w.Writer, file) | |
| } | |
| if target == "/video" { | |
| slog.Info("handler", "target", target) | |
| file, err := os.Open("assets/vim.mp4") | |
| if err != nil { | |
| status = response.StatusInternalServerError | |
| body = respond500() | |
| } else { | |
| defer file.Close() | |
| stat, err := file.Stat() | |
| if err != nil { | |
| status = response.StatusInternalServerError | |
| body = respond500() | |
| } else { | |
| h.Replace("Content-type", "video/mp4") | |
| h.Replace("Content-length", fmt.Sprintf("%d", stat.Size())) | |
| _ = w.WriteStatusLine(status) | |
| _ = w.WriteHeaders(h) | |
| _, _ = io.Copy(w.Writer, file) | |
| return | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In cmd/httpServer/main.go around lines 97 to 113, the /video handler currently
reads the entire file into memory with os.ReadFile, ignores errors, and then
falls through causing a second response; change it to open the file with
os.Open, check and handle errors (log and return an appropriate HTTP error
status), stat the file to set Content-length, set Content-type, write the status
and headers, stream the file to the response using io.Copy, close the file, and
return immediately after serving to avoid writing a second response.
Comment on lines
+115
to
+152
| if strings.HasPrefix(target, "/httpbin/") { | ||
| res, err := http.Get("https://httpbin.org/" + target[len("/httpbin/"):]) | ||
| if err != nil { | ||
| status = response.StatusInternalServerError | ||
| body = respond500() | ||
| } else { | ||
| w.WriteStatusLine(status) | ||
|
|
||
| h.Delete("Content-length") | ||
| h.Set("Transfer-encoding", "chuncked") | ||
| h.Replace("Content-type", "text/plain") | ||
| w.WriteHeaders(h) | ||
|
|
||
| fullBody := make([]byte, 0) | ||
|
|
||
| for { | ||
| buf := make([]byte, 30) | ||
| n, err := res.Body.Read(buf) | ||
| if err != nil { | ||
| break | ||
| } | ||
|
|
||
| fullBody = append(fullBody, buf[:n]...) | ||
| w.WriteBody(fmt.Appendf(nil, "%x\r\n", n)) | ||
| w.WriteBody(buf[:n]) | ||
| w.WriteBody([]byte("\r\n")) | ||
| } | ||
|
|
||
| w.WriteBody([]byte("0\r\n")) | ||
|
|
||
| out := sha256.Sum256(fullBody) | ||
| trailers := headers.NewHeaders() | ||
| trailers.Set("X-Content-SHA256", toStr(out[:])) | ||
| trailers.Set("X-Content-length", fmt.Sprintf("%d", len(fullBody))) | ||
| w.WriteHeaders(trailers) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
/httpbin path: wrong Transfer-Encoding, leaked body, chunking loop drops bytes, trailers not advertised
- Header typo: “chuncked” -> “chunked” (clients won’t parse your framing).
res.Bodynot closed (leak).- Loop breaks on any error before writing remaining bytes (drops data on EOF-with-bytes).
- Trailers should be announced via “Trailer” header; also fix header casing for
X-Content-Length. - Prefer upstream Content-Type when available.
if strings.HasPrefix(target, "/httpbin/") {
res, err := http.Get("https://httpbin.org/" + target[len("/httpbin/"):])
if err != nil {
status = response.StatusInternalServerError
body = respond500()
} else {
- w.WriteStatusLine(status)
-
- h.Delete("Content-length")
- h.Set("Transfer-encoding", "chuncked")
- h.Replace("Content-type", "text/plain")
- w.WriteHeaders(h)
-
- fullBody := make([]byte, 0)
-
- for {
- buf := make([]byte, 30)
- n, err := res.Body.Read(buf)
- if err != nil {
- break
- }
-
- fullBody = append(fullBody, buf[:n]...)
- w.WriteBody(fmt.Appendf(nil, "%x\r\n", n))
- w.WriteBody(buf[:n])
- w.WriteBody([]byte("\r\n"))
- }
-
- w.WriteBody([]byte("0\r\n"))
-
- out := sha256.Sum256(fullBody)
- trailers := headers.NewHeaders()
- trailers.Set("X-Content-SHA256", toStr(out[:]))
- trailers.Set("X-Content-length", fmt.Sprintf("%d", len(fullBody)))
- w.WriteHeaders(trailers)
- return
+ defer res.Body.Close()
+ _ = w.WriteStatusLine(status)
+
+ h.Delete("Content-length")
+ h.Replace("Transfer-Encoding", "chunked")
+ if ct := res.Header.Get("Content-Type"); ct != "" {
+ h.Replace("Content-type", ct)
+ } else {
+ h.Replace("Content-type", "text/plain")
+ }
+ h.Set("Trailer", "X-Content-SHA256, X-Content-Length")
+ _ = w.WriteHeaders(h)
+
+ fullBody := make([]byte, 0)
+ buf := make([]byte, 4096)
+ for {
+ n, readErr := res.Body.Read(buf)
+ if n > 0 {
+ fullBody = append(fullBody, buf[:n]...)
+ _, _ = w.WriteBody(fmt.Appendf(nil, "%x\r\n", n))
+ _, _ = w.WriteBody(buf[:n])
+ _, _ = w.WriteBody([]byte("\r\n"))
+ }
+ if readErr == io.EOF {
+ break
+ }
+ if readErr != nil {
+ // Stop streaming on read error; client will treat as truncated.
+ break
+ }
+ }
+
+ _, _ = w.WriteBody([]byte("0\r\n"))
+
+ out := sha256.Sum256(fullBody)
+ trailers := headers.NewHeaders()
+ trailers.Set("X-Content-SHA256", toStr(out[:]))
+ trailers.Set("X-Content-Length", fmt.Sprintf("%d", len(fullBody)))
+ _ = w.WriteHeaders(trailers)
+ return
}
}📝 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.
Suggested change
| if strings.HasPrefix(target, "/httpbin/") { | |
| res, err := http.Get("https://httpbin.org/" + target[len("/httpbin/"):]) | |
| if err != nil { | |
| status = response.StatusInternalServerError | |
| body = respond500() | |
| } else { | |
| w.WriteStatusLine(status) | |
| h.Delete("Content-length") | |
| h.Set("Transfer-encoding", "chuncked") | |
| h.Replace("Content-type", "text/plain") | |
| w.WriteHeaders(h) | |
| fullBody := make([]byte, 0) | |
| for { | |
| buf := make([]byte, 30) | |
| n, err := res.Body.Read(buf) | |
| if err != nil { | |
| break | |
| } | |
| fullBody = append(fullBody, buf[:n]...) | |
| w.WriteBody(fmt.Appendf(nil, "%x\r\n", n)) | |
| w.WriteBody(buf[:n]) | |
| w.WriteBody([]byte("\r\n")) | |
| } | |
| w.WriteBody([]byte("0\r\n")) | |
| out := sha256.Sum256(fullBody) | |
| trailers := headers.NewHeaders() | |
| trailers.Set("X-Content-SHA256", toStr(out[:])) | |
| trailers.Set("X-Content-length", fmt.Sprintf("%d", len(fullBody))) | |
| w.WriteHeaders(trailers) | |
| return | |
| } | |
| } | |
| if strings.HasPrefix(target, "/httpbin/") { | |
| res, err := http.Get("https://httpbin.org/" + target[len("/httpbin/"):]) | |
| if err != nil { | |
| status = response.StatusInternalServerError | |
| body = respond500() | |
| } else { | |
| // Ensure the response body is closed | |
| defer res.Body.Close() | |
| _ = w.WriteStatusLine(status) | |
| // Remove any existing Content-Length, use proper chunked framing | |
| h.Delete("Content-length") | |
| h.Replace("Transfer-Encoding", "chunked") | |
| // Preserve upstream Content-Type if present | |
| if ct := res.Header.Get("Content-Type"); ct != "" { | |
| h.Replace("Content-type", ct) | |
| } else { | |
| h.Replace("Content-type", "text/plain") | |
| } | |
| // Advertise trailers upfront | |
| h.Set("Trailer", "X-Content-SHA256, X-Content-Length") | |
| _ = w.WriteHeaders(h) | |
| // Read and stream the body in chunks, preserving all bytes | |
| fullBody := make([]byte, 0) | |
| buf := make([]byte, 4096) | |
| for { | |
| n, readErr := res.Body.Read(buf) | |
| if n > 0 { | |
| fullBody = append(fullBody, buf[:n]...) | |
| _, _ = w.WriteBody(fmt.Appendf(nil, "%x\r\n", n)) | |
| _, _ = w.WriteBody(buf[:n]) | |
| _, _ = w.WriteBody([]byte("\r\n")) | |
| } | |
| if readErr == io.EOF { | |
| break | |
| } | |
| if readErr != nil { | |
| // On I/O error, stop the stream (client sees a truncated body) | |
| break | |
| } | |
| } | |
| // Final zero-length chunk | |
| _, _ = w.WriteBody([]byte("0\r\n")) | |
| // Calculate and emit trailers | |
| out := sha256.Sum256(fullBody) | |
| trailers := headers.NewHeaders() | |
| trailers.Set("X-Content-SHA256", toStr(out[:])) | |
| trailers.Set("X-Content-Length", fmt.Sprintf("%d", len(fullBody))) | |
| _ = w.WriteHeaders(trailers) | |
| return | |
| } | |
| } |
🤖 Prompt for AI Agents
In cmd/httpServer/main.go around lines 115-152: fix multiple issues in the
/httpbin proxy path — set Transfer-Encoding to "chunked" (fix typo), close
res.Body (defer res.Body.Close()) to avoid leaks, and read the upstream body
loop correctly (handle io.EOF specially so any bytes read with EOF are written
before breaking, and treat other read errors as failures). Announce trailers
before sending headers by setting a "Trailer" header listing "X-Content-SHA256"
and "X-Content-Length", preserve upstream Content-Type if present (fallback to
text/plain), remove or delete any Content-Length header, write chunked encoding
correctly including the terminating "0\r\n\r\n" before sending trailers, and use
canonical header name "X-Content-Length" when setting the trailer value.
Comment on lines
+3
to
+8
| import ( | ||
| "fmt" | ||
| "io" | ||
|
|
||
| "github.com/merge/handly/internal/headers" | ||
| ) |
There was a problem hiding this comment.
Fix incorrect import path and add missing dependency.
The module path is wrong and will not compile. Also, upcoming header validation requires strings.
Apply:
import (
"fmt"
"io"
-
- "github.com/merge/handly/internal/headers"
+ "strings"
+
+ "github.com/1500-Bytes/handly/internal/headers"
)📝 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.
Suggested change
| import ( | |
| "fmt" | |
| "io" | |
| "github.com/merge/handly/internal/headers" | |
| ) | |
| import ( | |
| "fmt" | |
| "io" | |
| "strings" | |
| "github.com/1500-Bytes/handly/internal/headers" | |
| ) |
🤖 Prompt for AI Agents
In internal/response/response.go around lines 3 to 8, the import block uses an
incorrect module path and is missing the strings package needed for upcoming
header validation; update the headers import to the repository's correct module
path (replace the wrong path with the actual module path used by the project)
and add "strings" to the import list so the file compiles and header validation
can use strings functions.
Comment on lines
+21
to
+35
| func WriteStatusLine(w io.Writer, statusCode StatusCode) error { | ||
| statusLine := []byte{} | ||
|
|
||
| switch statusCode { | ||
| case StatusOK: | ||
| statusLine = []byte("HTTP/1.1 200 OK\r\n") | ||
| case StatusBadRequest: | ||
| statusLine = []byte("HTTP/1.1 400 Bad Request\r\n") | ||
| case StatusInternalServerError: | ||
| statusLine = []byte("HTTP/1.1 500 Internal Server Error\r\n") | ||
| } | ||
|
|
||
| _, err := w.Write(statusLine) | ||
| return err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against unsupported status codes.
Unknown codes currently write an empty status line, producing an invalid HTTP response.
Apply:
func WriteStatusLine(w io.Writer, statusCode StatusCode) error {
statusLine := []byte{}
switch statusCode {
case StatusOK:
statusLine = []byte("HTTP/1.1 200 OK\r\n")
case StatusBadRequest:
statusLine = []byte("HTTP/1.1 400 Bad Request\r\n")
case StatusInternalServerError:
statusLine = []byte("HTTP/1.1 500 Internal Server Error\r\n")
+ default:
+ return fmt.Errorf("unsupported status code: %d", statusCode)
}
_, err := w.Write(statusLine)
return err
}🤖 Prompt for AI Agents
In internal/response/response.go around lines 21 to 35, the switch over
StatusCode can fall through leaving statusLine empty which produces an invalid
HTTP response; add a guard for unsupported/unknown status codes by handling the
default case: either set a sensible default status line (e.g., "HTTP/1.1 500
Internal Server Error\r\n") or return a non-nil error (e.g.,
fmt.Errorf("unsupported status code: %d", statusCode)); ensure the function does
not write an empty status line and consistently returns an error when an
unsupported code is encountered.
Comment on lines
+37
to
+45
| func WriteHeaders(w io.Writer, h *headers.Headers) error { | ||
| b := []byte{} | ||
| h.ForEach(func(n, v string) { | ||
| b = fmt.Appendf(b, "%s: %s\r\n", n, v) | ||
| }) | ||
| b = fmt.Append(b, "\r\n") | ||
| _, err := w.Write(b) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Prevent CRLF header injection and fail fast on invalid header fields.
Header names/values aren’t sanitized; this allows response-splitting and request smuggling via embedded CR/LF.
Apply:
func WriteHeaders(w io.Writer, h *headers.Headers) error {
- b := []byte{}
- h.ForEach(func(n, v string) {
- b = fmt.Appendf(b, "%s: %s\r\n", n, v)
- })
- b = fmt.Append(b, "\r\n")
- _, err := w.Write(b)
- return err
+ b := []byte{}
+ var herr error
+ h.ForEach(func(n, v string) {
+ if herr != nil {
+ return
+ }
+ if strings.ContainsAny(n, "\r\n") || strings.ContainsAny(v, "\r\n") {
+ herr = fmt.Errorf("invalid header %q: CR/LF not allowed in name or value", n)
+ return
+ }
+ b = fmt.Appendf(b, "%s: %s\r\n", n, v)
+ })
+ if herr != nil {
+ return herr
+ }
+ b = fmt.Append(b, "\r\n")
+ _, err := w.Write(b)
+ return err
}📝 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.
Suggested change
| func WriteHeaders(w io.Writer, h *headers.Headers) error { | |
| b := []byte{} | |
| h.ForEach(func(n, v string) { | |
| b = fmt.Appendf(b, "%s: %s\r\n", n, v) | |
| }) | |
| b = fmt.Append(b, "\r\n") | |
| _, err := w.Write(b) | |
| return err | |
| } | |
| func WriteHeaders(w io.Writer, h *headers.Headers) error { | |
| b := []byte{} | |
| var herr error | |
| h.ForEach(func(n, v string) { | |
| if herr != nil { | |
| return | |
| } | |
| // Fail fast if name or value contains CR or LF | |
| if strings.ContainsAny(n, "\r\n") || strings.ContainsAny(v, "\r\n") { | |
| herr = fmt.Errorf("invalid header %q: CR/LF not allowed in name or value", n) | |
| return | |
| } | |
| b = fmt.Appendf(b, "%s: %s\r\n", n, v) | |
| }) | |
| if herr != nil { | |
| return herr | |
| } | |
| b = fmt.Append(b, "\r\n") | |
| _, err := w.Write(b) | |
| return err | |
| } |
🤖 Prompt for AI Agents
In internal/response/response.go around lines 37 to 45, header names and values
are written without validation, allowing CRLF injection; change the function to
validate each header name and value inside the h.ForEach loop and return an
error immediately if either contains prohibited characters (CR '\r', LF '\n') or
otherwise invalid header token characters per RFC (e.g. control chars or
separators), rejecting the write; only after all headers pass validation, build
the output and write to w (fail fast on first invalid header) so
response-splitting/request-smuggling cannot occur.
Comment on lines
+92
to
+99
| func (w *Writer) WriteBody(p []byte) (int, error) { | ||
| n, err := w.Writer.Write(p) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| return n, nil | ||
| } |
There was a problem hiding this comment.
Preserve partial writes; don’t zero out n on error.
Current code returns (0, err) even if some bytes were written, hiding progress and complicating retries.
Apply:
func (w *Writer) WriteBody(p []byte) (int, error) {
- n, err := w.Writer.Write(p)
- if err != nil {
- return 0, err
- }
-
- return n, nil
+ n, err := w.Writer.Write(p)
+ return n, err
}Optionally, guarantee full writes:
func (w *Writer) WriteBody(p []byte) (int, error) {
wrote := 0
for wrote < len(p) {
n, err := w.Writer.Write(p[wrote:])
wrote += n
if err != nil {
return wrote, err
}
}
return wrote, nil
}🤖 Prompt for AI Agents
internal/response/response.go lines 92-99: the WriteBody implementation zeroes
the byte count on error (returns 0, err) which hides partial progress; change it
to preserve and return the number of bytes actually written when an error occurs
(return n, err) or, better, implement a write loop that repeatedly writes the
remaining slice until either all bytes are written (return len(p), nil) or an
error occurs, in which case return the cumulative bytes written and the error.
Comment on lines
+14
to
+17
| type Server struct { | ||
| close bool | ||
| handler Handler | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Server cannot actually stop; store the listener and use an atomic closed flag
s.Close() sets a bool that the accept loop never observes until after Accept returns. Store the listener and make closure race-free.
type Server struct {
- close bool
- handler Handler
+ handler Handler
+ listener net.Listener
+ closed atomic.Bool
}Note: add sync/atomic to imports.
📝 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.
Suggested change
| type Server struct { | |
| close bool | |
| handler Handler | |
| } | |
| type Server struct { | |
| handler Handler | |
| listener net.Listener | |
| closed atomic.Bool | |
| } |
🤖 Prompt for AI Agents
In internal/server/server.go around lines 14-17, the Server currently stores a
plain bool "close" that the accept loop never sees and cannot race-free stop;
replace it by storing the net.Listener on the Server and an atomic closed flag
(e.g. uint32) instead of bool, add sync/atomic to imports, set the listener when
starting, have Close() call s.listener.Close() and atomic.StoreUint32(&s.closed,
1), and update the accept loop to check atomic.LoadUint32(&s.closed) (or rely on
Accept returning an error after Close) so shutdown is race-free and immediate.
Comment on lines
+38
to
+51
| func (s *Server) Listen(listener net.Listener) { | ||
| for { | ||
| conn, err := listener.Accept() | ||
| if s.close { | ||
| return | ||
| } | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| go s.HandleConn(conn) | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Accept loop should honor shutdown and handle transient errors
- Unblock via
listener.Close()and checks.closed. - Continue on temporary errors; close accepted conn if shutting down.
func (s *Server) Listen(listener net.Listener) {
for {
- conn, err := listener.Accept()
- if s.close {
- return
- }
- if err != nil {
- return
- }
-
- go s.HandleConn(conn)
+ conn, err := listener.Accept()
+ if s.closed.Load() {
+ if conn != nil {
+ conn.Close()
+ }
+ return
+ }
+ if err != nil {
+ if ne, ok := err.(net.Error); ok && ne.Temporary() {
+ continue
+ }
+ if s.closed.Load() {
+ return
+ }
+ return
+ }
+ go s.HandleConn(conn)
}
}📝 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.
Suggested change
| func (s *Server) Listen(listener net.Listener) { | |
| for { | |
| conn, err := listener.Accept() | |
| if s.close { | |
| return | |
| } | |
| if err != nil { | |
| return | |
| } | |
| go s.HandleConn(conn) | |
| } | |
| } | |
| func (s *Server) Listen(listener net.Listener) { | |
| for { | |
| conn, err := listener.Accept() | |
| if s.closed.Load() { | |
| if conn != nil { | |
| conn.Close() | |
| } | |
| return | |
| } | |
| if err != nil { | |
| if ne, ok := err.(net.Error); ok && ne.Temporary() { | |
| continue | |
| } | |
| if s.closed.Load() { | |
| return | |
| } | |
| return | |
| } | |
| go s.HandleConn(conn) | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/server/server.go around lines 38 to 51, the accept loop currently
returns on any error and doesn't correctly honor shutdown; modify it so shutdown
via listener.Close() and s.closed is checked and handled: after Accept() check
s.closed and return if set; on Accept errors, if the error is temporary
(net.Error and Temporary()) continue the loop, otherwise return; after a
successful Accept, if s.closed became true close the accepted conn and return;
otherwise launch go s.HandleConn(conn). Ensure s.closed reads are safe (use
existing mutex or atomic if required) so the loop reliably notices shutdown
unblocking via listener.Close().
Comment on lines
+53
to
+67
| func ListenAndServe(port uint16, handler Handler) (*Server, error) { | ||
| netListener, err := net.Listen("tcp", fmt.Sprintf(":%d", port)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| s := &Server{ | ||
| close: false, | ||
| handler: handler, | ||
| } | ||
|
|
||
| go s.Listen(netListener) | ||
|
|
||
| return s, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Persist listener in Server
Needed so Close() can unblock Accept().
func ListenAndServe(port uint16, handler Handler) (*Server, error) {
netListener, err := net.Listen("tcp", fmt.Sprintf(":%d", port))
if err != nil {
return nil, err
}
s := &Server{
- close: false,
- handler: handler,
+ handler: handler,
+ listener: netListener,
}
- go s.Listen(netListener)
+ go s.Listen(netListener)
return s, 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.
Suggested change
| func ListenAndServe(port uint16, handler Handler) (*Server, error) { | |
| netListener, err := net.Listen("tcp", fmt.Sprintf(":%d", port)) | |
| if err != nil { | |
| return nil, err | |
| } | |
| s := &Server{ | |
| close: false, | |
| handler: handler, | |
| } | |
| go s.Listen(netListener) | |
| return s, nil | |
| } | |
| func ListenAndServe(port uint16, handler Handler) (*Server, error) { | |
| netListener, err := net.Listen("tcp", fmt.Sprintf(":%d", port)) | |
| if err != nil { | |
| return nil, err | |
| } | |
| s := &Server{ | |
| handler: handler, | |
| listener: netListener, | |
| } | |
| go s.Listen(netListener) | |
| return s, nil | |
| } |
🤖 Prompt for AI Agents
In internal/server/server.go around lines 53 to 67, the net.Listener created by
net.Listen must be stored on the Server so Close() can call listener.Close() to
unblock Accept(); add a listener field of type net.Listener to the Server struct
if it doesn't exist, assign s.listener = netListener before spawning the
goroutine, and ensure Close() uses that stored listener to close the socket (and
handle nil checks).
Comment on lines
+69
to
+71
| func (s *Server) Close() { | ||
| s.close = true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Close should set flag and close the listener
Currently it only flips a bool; Accept() remains blocked.
func (s *Server) Close() {
- s.close = true
+ s.closed.Store(true)
+ if s.listener != nil {
+ _ = s.listener.Close()
+ }
}📝 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.
Suggested change
| func (s *Server) Close() { | |
| s.close = true | |
| } | |
| func (s *Server) Close() { | |
| s.closed.Store(true) | |
| if s.listener != nil { | |
| _ = s.listener.Close() | |
| } | |
| } |
🤖 Prompt for AI Agents
internal/server/server.go lines 69-71: the Close method only flips a bool so
Accept() remains blocked; modify Close to acquire the server mutex, set the
close flag, and close the listener to unblock Accept. Specifically: lock the
server's mutex, check/set s.close, if s.listener != nil call s.listener.Close()
(handle/ignore returned error), then release the mutex; ensure closing is done
only once to avoid races/panics.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit