Skip to content
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.

Sign up for GitHub

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

hCaptcha: add custom error handling and fix documentation #1191

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

hCaptcha: add custom error handling and fix documentation #1191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions hcaptcha/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,18 +39,20 @@ hcaptcha.New(config hcaptcha.Config) fiber.Handler

## Config

| Property | Type | Description | Default |
|:----------------|:----------------------------------|:-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:--------------------------------------|
| SecretKey | `string` | The secret key you obtained from the HCaptcha admin panel. This field must not be empty. | `""` |
| ResponseKeyFunc | `func(fiber.Ctx) (string, error)` | ResponseKeyFunc should return the token that captcha provides upon successful solving. By default, it gets the token from the body by parsing a JSON request and returns the `hcaptcha_token` field. | `hcaptcha.DefaultResponseKeyFunc` |
| SiteVerifyURL | `string` | This property specifies the API resource used for token authentication. | `https://api.hcaptcha.com/siteverify` |
| Property | Type | Description | Default |
|:-----------------|:----------------------------------|:-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|:--------------------------------------|
| SecretKey | `string` | The secret key you obtained from the HCaptcha admin panel. This field must not be empty. | `""` |
| ResponseKeyFunc | `func(fiber.Ctx) (string, error)`| ResponseKeyFunc should return the token that captcha provides upon successful solving. By default, it gets the token from the body by parsing a JSON request and returns the `hcaptcha_token` field. | `hcaptcha.DefaultResponseKeyFunc` |
| SiteVerifyURL | `string` | This property specifies the API resource used for token authentication. | `https://api.hcaptcha.com/siteverify` |
| 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` |

## Example

```go
package main

import (
"errors"
"github.com/gofiber/contrib/hcaptcha"
"github.com/gofiber/fiber/v3"
"log"
Expand All @@ -63,9 +65,22 @@ const (

func main() {
app := fiber.New()

// Create HCaptcha middleware
captcha := hcaptcha.New(hcaptcha.Config{
// Must set the secret key
SecretKey: TestSecretKey,
// Custom validation function (optional)
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
},
Comment on lines +74 to +83
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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
},

})

app.Get("/api/", func(c fiber.Ctx) error {
Expand All @@ -74,9 +89,9 @@ func main() {
})
})

app.Post("/api/robots-excluded", func(c fiber.Ctx) error {
app.Post("/api/submit", captcha, func(c fiber.Ctx) error {
return c.SendString("You are not a robot")
}, captcha)
})
Comment on lines +92 to +94
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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
})
})


log.Fatal(app.Listen(":3000"))
}
Expand Down
2 changes: 2 additions & 0 deletions hcaptcha/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ type Config struct {
// SiteVerifyURL is the endpoint URL where the program should verify the given token
// default value is: "https://api.hcaptcha.com/siteverify"
SiteVerifyURL string
// ValidateFunc allows custom validation logic based on the HCaptcha validation result and the context
ValidateFunc func(success bool, c fiber.Ctx) error
}

// DefaultResponseKeyFunc is the default function to get the HCaptcha token from the request body
Expand Down
15 changes: 12 additions & 3 deletions hcaptcha/hcaptcha.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,18 @@ func (h *HCaptcha) Validate(c fiber.Ctx) error {
return fmt.Errorf("error decoding HCaptcha API response: %w", err)
}

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 {
// If the custom ValidateFunc returns an error, set the response status code accordingly
c.Status(fiber.StatusForbidden)
return err
}
Comment on lines +74 to +78
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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)
}

} else {
if !o.Success {
c.Status(fiber.StatusForbidden)
return errors.New("unable to check that you are not a robot")
}
Comment on lines +72 to +83
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
// 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)
}

Comment on lines +72 to +83
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
// 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")
}

}

return c.Next()
Expand Down
Loading