hCaptcha: add custom error handling and fix documentation #1191
Conversation
- Extend the Config structure to include a ValidateFunc field for custom error handling. - Update the Validate method to call the custom function if provided. - Fix documentation to clarify the correct order of middleware and handler in usage examples. This change allows users to define custom validation responses for HCaptcha, providing more flexibility in error handling.
levatax
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe changes introduce a new property, Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
☢️ Bug
![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
🧹 Outside diff range and nitpick comments (4)
hcaptcha/config.go (2)
22-23: Add documentation for the ValidateFunc field.The new ValidateFunc field would benefit from documentation comments explaining:
- Its purpose and when it's called
- The meaning of the
successparameter- Expected behavior when returning nil vs error
- Default behavior when ValidateFunc is not set
Add documentation like this:
+ // ValidateFunc allows custom validation handling based on the HCaptcha validation result. + // If set, it will be called with the validation success status and the current context. + // Return nil to continue processing, or return an error to stop the middleware chain. + // If ValidateFunc is nil, the middleware will use default error handling. ValidateFunc func(success bool, c fiber.Ctx) error
Line range hint
13-23: Consider standardizing error handling across all configuration functions.The Config struct has a good design, but consider standardizing error handling across all function fields. Since we're adding custom validation with ValidateFunc, we could extend this pattern to other error scenarios:
Consider adding:
- A custom error type for different validation scenarios
- Error handling for invalid configuration (e.g., empty SecretKey)
- A method to validate the Config struct itself
Example error type:
type HCaptchaError struct { Type string Message string Err error } func (e *HCaptchaError) Error() string { return fmt.Sprintf("%s: %s", e.Type, e.Message) }hcaptcha/hcaptcha.go (1)
72-83: Document the ValidateFunc contract in code comments.The custom validation function's expected behavior, parameters, and return value contract should be documented to guide implementers.
Add documentation above the ValidateFunc field in the Config struct:
// ValidateFunc allows custom validation logic for hCaptcha responses. // It receives the validation success status and fiber.Ctx. // Return an error to indicate validation failure. // If the function returns an error, the response status will be set to 403 Forbidden.hcaptcha/README.md (1)
45-45: Minor grammar improvement neededAdd "the" before "captcha" for better readability.
-ResponseKeyFunc should return the token that captcha provides upon successful solving. +ResponseKeyFunc should return the token that the captcha provides upon successful solving.🧰 Tools
🪛 LanguageTool
[uncategorized] ~45-~45: You might be missing the article “the” here.
Context: ...nseKeyFunc should return the token that captcha provides upon successful solving. By de...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- hcaptcha/README.md (3 hunks)
- hcaptcha/config.go (1 hunks)
- hcaptcha/hcaptcha.go (1 hunks)
🧰 Additional context used
🪛 LanguageTool
hcaptcha/README.md
[uncategorized] ~45-~45: You might be missing the article “the” here.
Context: ...nseKeyFunc should return the token that captcha provides upon successful solving. By de...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (5)
hcaptcha/config.go (1)
Line range hint
26-40: Enhance request validation in DefaultResponseKeyFunc.While the implementation handles basic JSON decoding, consider adding more robust validation:
Consider these improvements:
- Add token validation:
func DefaultResponseKeyFunc(c fiber.Ctx) (string, error) { data := struct { HCaptchaToken string `json:"hcaptcha_token"` }{} err := json.NewDecoder(bytes.NewReader(c.Body())).Decode(&data) if err != nil { return "", fmt.Errorf("failed to decode HCaptcha token: %w", err) } + // Validate token is not empty or just whitespace + if len(strings.TrimSpace(data.HCaptchaToken)) == 0 { + return "", fmt.Errorf("hcaptcha token is empty") + } + return data.HCaptchaToken, nil }hcaptcha/hcaptcha.go (1)
72-83: Verify custom validation security implications.The custom validation function has the power to bypass security checks. Ensure that implementers understand the security implications.
Consider adding the following safeguards:
- Document security best practices in README.md
- Add validation that prevents bypassing the success check entirely
- Consider providing a validation builder function that enforces security constraints
Let's verify if there are any existing security-related comments or documentation:
hcaptcha/README.md (3)
47-47: LGTM! Clear documentation for ValidateFuncThe new ValidateFunc property is well documented with clear explanation of its purpose and behavior.
74-83: LGTM! Well-implemented custom validation functionThe ValidateFunc implementation demonstrates good practices:
- Proper error handling with custom messages
- Appropriate HTTP status code (403 Forbidden)
- Detailed JSON response structure
92-94: LGTM! Improved endpoint namingThe endpoint rename from
/api/robots-excludedto/api/submitis more descriptive and better reflects the action being performed.
| if err := h.ValidateFunc(o.Success, c); err != nil { | ||
| // If the custom ValidateFunc returns an error, set the response status code accordingly | ||
| c.Status(fiber.StatusForbidden) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Consider wrapping custom validation errors for security.
Custom validation functions might return errors containing sensitive information. Consider wrapping these errors with a generic message before returning them to clients.
Apply this change:
if err := h.ValidateFunc(o.Success, c); err != nil {
c.Status(fiber.StatusForbidden)
- return err
+ return fmt.Errorf("hCaptcha validation failed: %w", 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.
| if err := h.ValidateFunc(o.Success, c); err != nil { | |
| // If the custom ValidateFunc returns an error, set the response status code accordingly | |
| c.Status(fiber.StatusForbidden) | |
| return err | |
| } | |
| if err := h.ValidateFunc(o.Success, c); err != nil { | |
| // If the custom ValidateFunc returns an error, set the response status code accordingly | |
| c.Status(fiber.StatusForbidden) | |
| return fmt.Errorf("hCaptcha validation failed: %w", err) | |
| } |
| // Use custom ValidateFunc if defined, otherwise default behavior | ||
| if h.ValidateFunc != nil { | ||
| if err := h.ValidateFunc(o.Success, c); err != nil { | ||
| // If the custom ValidateFunc returns an error, set the response status code accordingly | ||
| c.Status(fiber.StatusForbidden) | ||
| return err | ||
| } | ||
| } else { | ||
| if !o.Success { | ||
| c.Status(fiber.StatusForbidden) | ||
| return errors.New("unable to check that you are not a robot") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate status code setting.
The status code setting is duplicated in both branches. Consider setting it once after the validation check.
Refactor the code to reduce duplication:
// Use custom ValidateFunc if defined, otherwise default behavior
+var validationErr error
if h.ValidateFunc != nil {
- if err := h.ValidateFunc(o.Success, c); err != nil {
- // If the custom ValidateFunc returns an error, set the response status code accordingly
- c.Status(fiber.StatusForbidden)
- return err
- }
+ validationErr = h.ValidateFunc(o.Success, c)
} else {
if !o.Success {
- c.Status(fiber.StatusForbidden)
- return errors.New("unable to check that you are not a robot")
+ validationErr = errors.New("unable to check that you are not a robot")
}
}
+
+if validationErr != nil {
+ c.Status(fiber.StatusForbidden)
+ return fmt.Errorf("hCaptcha validation failed: %w", validationErr)
+}📝 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.
| // Use custom ValidateFunc if defined, otherwise default behavior | |
| if h.ValidateFunc != nil { | |
| if err := h.ValidateFunc(o.Success, c); err != nil { | |
| // If the custom ValidateFunc returns an error, set the response status code accordingly | |
| c.Status(fiber.StatusForbidden) | |
| return err | |
| } | |
| } else { | |
| if !o.Success { | |
| c.Status(fiber.StatusForbidden) | |
| return errors.New("unable to check that you are not a robot") | |
| } | |
| // Use custom ValidateFunc if defined, otherwise default behavior | |
| var validationErr error | |
| if h.ValidateFunc != nil { | |
| validationErr = h.ValidateFunc(o.Success, c) | |
| } else { | |
| if !o.Success { | |
| validationErr = errors.New("unable to check that you are not a robot") | |
| } | |
| } | |
| if validationErr != nil { | |
| c.Status(fiber.StatusForbidden) | |
| return fmt.Errorf("hCaptcha validation failed: %w", validationErr) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
hcaptcha/config.go (1)
22-23: Consider enhancing ValidateFunc documentation and defaults.The addition of ValidateFunc is a good improvement for custom error handling. However, consider the following enhancements:
Add more detailed documentation about:
- When this function is called in the validation flow
- Whether it runs after or replaces the default validation
- Example usage showing common patterns
Consider providing a default implementation to maintain consistent behavior when ValidateFunc is nil.
Apply this diff to enhance the documentation:
// ValidateFunc allows custom validation logic based on the HCaptcha validation result and the context +// If not set, the middleware will use default validation behavior. +// The function is called after the HCaptcha API validation, allowing custom handling of the result. +// Example usage: +// config.ValidateFunc = func(success bool, c fiber.Ctx) error { +// if !success { +// return errors.New("custom validation failed") +// } +// return nil +// } ValidateFunc func(success bool, c fiber.Ctx) errorhcaptcha/hcaptcha.go (1)
72-83: Improve documentation of ValidateFunc behaviorThe inline comments should clarify:
- The expected behavior of ValidateFunc
- Whether ValidateFunc should respect o.Success
- The relationship between o.Success and the returned error
-// Use custom ValidateFunc if defined, otherwise default behavior +// Execute custom validation if ValidateFunc is defined. ValidateFunc receives the HCaptcha +// verification result (o.Success) and should return an error if validation fails. +// If ValidateFunc is nil, falls back to default behavior of checking o.Success.hcaptcha/README.md (2)
46-47: Enhance ValidateFunc documentation with parameter descriptions.The ValidateFunc documentation could be more helpful by explicitly describing the parameters:
-| ValidateFunc | `func(bool, fiber.Ctx) error` | A custom validation function that allows you to define the behavior upon validation success or failure. If set, it will be called with the validation result and the context. | `nil` | +| ValidateFunc | `func(bool, fiber.Ctx) error` | A custom validation function that allows you to define the behavior upon validation success or failure. Parameters: `success` (boolean indicating if hCaptcha validation passed), `c` (Fiber context). Return an error to indicate validation failure. | `nil` |
92-94: Add comments to clarify middleware order.The example would benefit from a comment explaining the importance of middleware order.
+ // Note: hcaptcha middleware must be placed before the handler + // to ensure validation occurs before processing the request app.Post("/api/submit", captcha, func(c fiber.Ctx) error { return c.SendString("You are not a robot") })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- hcaptcha/README.md (3 hunks)
- hcaptcha/config.go (1 hunks)
- hcaptcha/hcaptcha.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
hcaptcha/config.go (1)
22-23: Verify middleware implementation of ValidateFunc.Let's verify that the middleware properly implements the ValidateFunc and maintains secure validation flow.
✅ Verification successful
Based on the results, I can now generate the final response as I have sufficient information about the ValidateFunc implementation.
ValidateFunc is properly implemented with secure validation flow
The middleware correctly implements ValidateFunc with proper validation flow:
- Base validation (hCaptcha API verification) occurs first, providing the success status
- ValidateFunc is called with the validation result and context if defined
- Appropriate error handling and status codes are set for both custom and default validation paths
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the middleware implementation to ensure: # 1. Base validation occurs before custom validation # 2. ValidateFunc is properly handled # Search for ValidateFunc usage in the middleware ast-grep --pattern 'if c.ValidateFunc != nil { $$$ }' # Check the validation flow rg -A 10 'ValidateFunc' hcaptcha/Length of output: 2804
hcaptcha/hcaptcha.go (2)
72-83: Verify that validation cannot be bypassedThe custom ValidateFunc could potentially ignore the o.Success parameter and always return nil, effectively bypassing HCaptcha validation. Consider enforcing validation when o.Success is false.
Consider this safer implementation:
if h.ValidateFunc != nil { + if !o.Success { + c.Status(fiber.StatusForbidden) + if err := h.ValidateFunc(false, c); err != nil { + return err + } + return errors.New("hCaptcha validation failed") + } - if err := h.ValidateFunc(o.Success, c); err != nil { + if err := h.ValidateFunc(true, c); err != nil { c.Status(fiber.StatusForbidden) return err } }
82-82: LGTM: Error handling implementationThe use of the
errorspackage for error creation follows Go best practices and provides clear error messages.
| // Use custom ValidateFunc if defined, otherwise default behavior | ||
| if h.ValidateFunc != nil { | ||
| if err := h.ValidateFunc(o.Success, c); err != nil { | ||
| // If the custom ValidateFunc returns an error, set the response status code accordingly | ||
| c.Status(fiber.StatusForbidden) | ||
| return err | ||
| } | ||
| } else { | ||
| if !o.Success { | ||
| c.Status(fiber.StatusForbidden) | ||
| return errors.New("unable to check that you are not a robot") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider allowing custom status codes for validation failures
The current implementation forces HTTP 403 Forbidden status for all validation failures. Consider allowing the custom ValidateFunc to set its own status code for more flexibility.
if h.ValidateFunc != nil {
if err := h.ValidateFunc(o.Success, c); err != nil {
- // If the custom ValidateFunc returns an error, set the response status code accordingly
- c.Status(fiber.StatusForbidden)
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.
| // Use custom ValidateFunc if defined, otherwise default behavior | |
| if h.ValidateFunc != nil { | |
| if err := h.ValidateFunc(o.Success, c); err != nil { | |
| // If the custom ValidateFunc returns an error, set the response status code accordingly | |
| c.Status(fiber.StatusForbidden) | |
| return err | |
| } | |
| } else { | |
| if !o.Success { | |
| c.Status(fiber.StatusForbidden) | |
| return errors.New("unable to check that you are not a robot") | |
| } | |
| // Use custom ValidateFunc if defined, otherwise default behavior | |
| if h.ValidateFunc != nil { | |
| if err := h.ValidateFunc(o.Success, c); err != nil { | |
| return err | |
| } | |
| } else { | |
| if !o.Success { | |
| c.Status(fiber.StatusForbidden) | |
| return errors.New("unable to check that you are not a robot") | |
| } |
| app.Post("/api/submit", captcha, func(c fiber.Ctx) error { | ||
| return c.SendString("You are not a robot") | ||
| }, captcha) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add example of accessing validation results in the handler.
Consider showing how to access validation results in the handler for more complex use cases.
app.Post("/api/submit", captcha, func(c fiber.Ctx) error {
- return c.SendString("You are not a robot")
+ return c.JSON(fiber.Map{
+ "success": true,
+ "message": "HCaptcha validation successful",
+ // Add any additional processing here
+ })
})📝 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.
| app.Post("/api/submit", captcha, func(c fiber.Ctx) error { | |
| return c.SendString("You are not a robot") | |
| }, captcha) | |
| }) | |
| app.Post("/api/submit", captcha, func(c fiber.Ctx) error { | |
| return c.JSON(fiber.Map{ | |
| "success": true, | |
| "message": "HCaptcha validation successful", | |
| // Add any additional processing here | |
| }) | |
| }) |
| ValidateFunc: func(success bool, c fiber.Ctx) error { | ||
| if !success { | ||
| c.Status(fiber.StatusForbidden).JSON(fiber.Map{ | ||
| "error": "Custom error: validation failed, please try again", | ||
| "details": "The HCaptcha validation was unsuccessful.", | ||
| }) | ||
| return errors.New("custom error: validation failed") | ||
| } | ||
| return nil | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance ValidateFunc example with more specific error handling.
The current error handling could be more informative by including the specific validation failure reason.
ValidateFunc: func(success bool, c fiber.Ctx) error {
if !success {
c.Status(fiber.StatusForbidden).JSON(fiber.Map{
- "error": "Custom error: validation failed, please try again",
- "details": "The HCaptcha validation was unsuccessful.",
+ "error": "HCaptcha validation failed",
+ "details": "Please complete the HCaptcha challenge correctly and try again.",
+ "status": "error",
+ "code": "HCAPTCHA_VALIDATION_FAILED"
})
return errors.New("custom error: validation failed")
}
return 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.
| ValidateFunc: func(success bool, c fiber.Ctx) error { | |
| if !success { | |
| c.Status(fiber.StatusForbidden).JSON(fiber.Map{ | |
| "error": "Custom error: validation failed, please try again", | |
| "details": "The HCaptcha validation was unsuccessful.", | |
| }) | |
| return errors.New("custom error: validation failed") | |
| } | |
| return nil | |
| }, | |
| ValidateFunc: func(success bool, c fiber.Ctx) error { | |
| if !success { | |
| c.Status(fiber.StatusForbidden).JSON(fiber.Map{ | |
| "error": "HCaptcha validation failed", | |
| "details": "Please complete the HCaptcha challenge correctly and try again.", | |
| "status": "error", | |
| "code": "HCAPTCHA_VALIDATION_FAILED" | |
| }) | |
| return errors.New("custom error: validation failed") | |
| } | |
| return nil | |
| }, |
|
@levatax thanks for the work do the other fiber middlewares or other hcaptcha golang functions already have such functions ? just want to make sure that the existing fiber/golang users are satisfied with the function names and functionality and do not expect other schemes |
ReneWerner87
changed the title
This pull request introduces custom error handling functionality to the hCaptcha middleware. It allows users to define a custom validation response function, providing greater flexibility for handling hCaptcha validation errors. If the custom function is set, it will be invoked with the validation result, allowing for tailored error responses.
Additionally, this PR corrects the documentation, specifying the correct order of middleware usage. The hCaptcha middleware should be placed before the handler in the route definition, ensuring proper validation of the hCaptcha token before processing requests.
Changes Made:
Summary by CodeRabbit
New Features
ValidateFunc, allowing users to define their own validation logic for HCaptcha results./api/submit.Bug Fixes