-
- Notifications
You must be signed in to change notification settings - Fork 703
Add solution for Challenge 5 by Johrespi #732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughA new Go HTTP server scaffold is introduced in the challenge-5 submission directory featuring authentication middleware that validates X-Auth-Token headers, two HTTP handlers (public and protected), and server configuration for routes Changes
Sequence DiagramsequenceDiagram participant Client participant AuthMiddleware participant Handler Client->>AuthMiddleware: GET /secure + X-Auth-Token: secret alt Token Valid AuthMiddleware->>Handler: Call next handler Handler->>Client: 200 "You are authorized!" else Token Missing/Invalid AuthMiddleware->>Client: 401 Unauthorized end Client->>AuthMiddleware: GET /hello (no auth required) AuthMiddleware->>Handler: helloHandler Handler->>Client: 200 "Hello!" Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes
Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
challenge-5/submissions/Johrespi/solution-template.go(1 hunks)
🔇 Additional comments (3)
challenge-5/submissions/Johrespi/solution-template.go (3)
39-42: LGTM!The secure handler is straightforward and correctly returns the authorization confirmation message.
44-57: LGTM!The server setup correctly configures the routes with proper middleware application. The public route remains unprotected while the secure route is wrapped with authentication middleware.
59-62: LGTM!The empty main with commented server code is appropriate for a challenge submission where tests likely invoke
SetupServer()directly.
| "net/http" | ||
| ) | ||
| | ||
| const validToken = "secret" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical security issue: Hardcoded authentication token.
The authentication token is hardcoded in the source code, which is a security vulnerability. Secrets should never be committed to version control.
Load the token from an environment variable instead:
-const validToken = "secret" +var validToken = os.Getenv("AUTH_TOKEN")And update the import section:
import ( "fmt" "net/http" +"os" )Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In challenge-5/submissions/Johrespi/solution-template.go around line 8, the token is hardcoded as const validToken = "secret"; replace this by reading the token from an environment variable (e.g., os.Getenv("VALID_TOKEN")) instead of a hardcoded constant, add the necessary import for "os" in the import block, and add a runtime check that the environment variable is present (log/fail or return an error if empty) so no secret is committed in code. | func AuthMiddleware(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| // TODO: Implement the logic: | ||
| // 1) Grab the "X-Auth-Token" header | ||
| // 2) Compare against validToken | ||
| // 3) If mismatch or missing, respond with 401 | ||
| // 4) Otherwise pass to next handler | ||
| | ||
| token := r.Header.Get("X-Auth-Token") | ||
| if token != validToken { | ||
| http.Error(w, "", 401) | ||
| return | ||
| } | ||
| next.ServeHTTP(w, r) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Improve error handling and remove TODO comments.
The implementation is functionally correct but has a few issues:
- Empty error message makes debugging difficult
- Hardcoded status code instead of using constants
- TODO comments should be removed in final submission
Apply this diff:
func AuthMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { -// TODO: Implement the logic: -// 1) Grab the "X-Auth-Token" header -// 2) Compare against validToken -// 3) If mismatch or missing, respond with 401 -// 4) Otherwise pass to next handler - token := r.Header.Get("X-Auth-Token") if token != validToken { -http.Error(w, "", 401) +http.Error(w, "Unauthorized", http.StatusUnauthorized) return } next.ServeHTTP(w, r) }) }📝 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.
| func AuthMiddleware(next http.Handler) http.Handler { | |
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| // TODO: Implement the logic: | |
| // 1) Grab the "X-Auth-Token" header | |
| // 2) Compare against validToken | |
| // 3) If mismatch or missing, respond with 401 | |
| // 4) Otherwise pass to next handler | |
| token := r.Header.Get("X-Auth-Token") | |
| if token != validToken { | |
| http.Error(w, "", 401) | |
| return | |
| } | |
| next.ServeHTTP(w, r) | |
| }) | |
| } | |
| func AuthMiddleware(next http.Handler) http.Handler { | |
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| token := r.Header.Get("X-Auth-Token") | |
| if token != validToken { | |
| http.Error(w, "Unauthorized", http.StatusUnauthorized) | |
| return | |
| } | |
| next.ServeHTTP(w, r) | |
| }) | |
| } |
🤖 Prompt for AI Agents
In challenge-5/submissions/Johrespi/solution-template.go around lines 13 to 28, remove the remaining TODO comment and improve the 401 response by returning a descriptive error message and using the standard http.StatusUnauthorized constant instead of the hardcoded 401; specifically, replace the empty http.Error call with one that supplies a meaningful message (e.g., "unauthorized: invalid or missing X-Auth-Token") and uses http.StatusUnauthorized, then ensure no TODOs remain in the function. | // helloHandler returns "Hello!" on GET /hello | ||
| func helloHandler(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method != http.MethodGet { | ||
| http.Error(w, "xd", 401) | ||
| return | ||
| } | ||
| fmt.Fprint(w, "Hello!") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect HTTP status code and unprofessional error message.
Line 33 has two critical issues:
- Returns 401 (Unauthorized) for incorrect HTTP methods, but should return 405 (Method Not Allowed)
- Error message "xd" is unprofessional and not descriptive
HTTP 401 is specifically for authentication failures, not method mismatches.
Apply this diff:
func helloHandler(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodGet { -http.Error(w, "xd", 401) +http.Error(w, "Method Not Allowed", http.StatusMethodNotAllowed) return } fmt.Fprint(w, "Hello!") }📝 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.
| // helloHandler returns "Hello!" on GET /hello | |
| func helloHandler(w http.ResponseWriter, r *http.Request) { | |
| if r.Method != http.MethodGet { | |
| http.Error(w, "xd", 401) | |
| return | |
| } | |
| fmt.Fprint(w, "Hello!") | |
| } | |
| // helloHandler returns "Hello!" on GET /hello | |
| func helloHandler(w http.ResponseWriter, r *http.Request) { | |
| if r.Method != http.MethodGet { | |
| http.Error(w, "Method Not Allowed", http.StatusMethodNotAllowed) | |
| return | |
| } | |
| fmt.Fprint(w, "Hello!") | |
| } |
🤖 Prompt for AI Agents
In challenge-5/submissions/Johrespi/solution-template.go around lines 30 to 37, the handler returns a 401 and an unprofessional message for wrong HTTP methods; change the response to use HTTP 405 Method Not Allowed and a clear message. Replace the http.Error(w, "xd", 401) with setting the Allow header to "GET" (w.Header().Set("Allow", "GET")) and call http.Error(w, "method not allowed", http.StatusMethodNotAllowed) so the status code and message are correct and informative.
Challenge 5 Solution
Submitted by: @Johrespi
Challenge: Challenge 5
Description
This PR contains my solution for Challenge 5.
Changes
challenge-5/submissions/Johrespi/solution-template.goTesting
Thank you for reviewing my submission! 🚀