feat: Improve CaptchaBone (#1243)

* feat: Validates score of [reCAPTCHA
v3](https://developers.google.com/recaptcha/docs/v3?hl=en) challenge
* feat: Add more verbose error messages
* feat: Add config option `captcha_enforce_always`
* change: Support bone name and "g-recaptcha-response" in `fromClient`
* refactor: Use `requests`
* docs: Update docstrings and describe behavior

---------

Co-authored-by: Jan Max Meyer <[email protected]>

src/viur/core/bones/captcha.py

@@ -1,82 +1,130 @@
-import json
-import urllib.parse
-import urllib.request
+import logging
 import typing as t
 
+import requests
+
 from viur.core import conf, current
 from viur.core.bones.base import BaseBone, ReadFromClientError, ReadFromClientErrorSeverity
 
+if t.TYPE_CHECKING:
+    from viur.core.skeleton import SkeletonInstance
+
 
 class CaptchaBone(BaseBone):
     r"""
     The CaptchaBone is used to ensure that a user is not a bot.
 
     The Captcha bone uses the Google reCAPTCHA API to perform the Captcha
-    validation and is derived from the BaseBone.
+    validation and supports v2 and v3.
+
+    .. seealso::
+
+        Option :attr:`core.config.Security.captcha_default_credentials`
+        for global security settings.
 
-    :param publicKey: The public key for the Captcha validation.
-    :param privateKey: The private key for the Captcha validation.
-    :param \**kwargs: Additional arguments to pass to the base class constructor.
+        Option :attr:`core.config.Security.captcha_enforce_always`
+        for developing.
     """
+
     type = "captcha"
 
-    def __init__(self, *, publicKey=None, privateKey=None, **kwargs):
+    def __init__(
+        self,
+        *,
+        publicKey: str = None,
+        privateKey: str = None,
+        score_threshold: float = 0.5,
+        **kwargs: t.Any
+    ):
+        """
+        Initializes a new CaptchaBone.
+
+        `publicKey` and `privateKey` can be omitted, if they are set globally
+        in :attr:`core.config.Security.captcha_default_credentials`.
+
+        :param publicKey: The public key for the Captcha validation.
+        :param privateKey: The private key for the Captcha validation.
+        :score_threshold: If reCAPTCHA v3 is used, the score must be at least this threshold.
+            For reCAPTCHA v2 this property will be ignored.
+        """
         super().__init__(**kwargs)
         self.defaultValue = self.publicKey = publicKey
         self.privateKey = privateKey
+        if not (0 < score_threshold <= 1):
+            raise ValueError("score_threshold must be between 0 and 1.")
+        self.score_threshold = score_threshold
         if not self.defaultValue and not self.privateKey:
             # Merge these values from the side-wide configuration if set
             if conf.security.captcha_default_credentials:
                 self.defaultValue = self.publicKey = conf.security.captcha_default_credentials["sitekey"]
                 self.privateKey = conf.security.captcha_default_credentials["secret"]
         self.required = True
+        if not self.privateKey:
+            raise ValueError("privateKey must be set.")
 
-    def serialize(self, skel: 'SkeletonInstance', name: str, parentIndexed: bool) -> bool:
+    def serialize(self, skel: "SkeletonInstance", name: str, parentIndexed: bool) -> bool:
         """
         Serializing the Captcha bone is not possible so it return False
         """
         return False
 
-    def unserialize(self, skel, name) -> bool:
+    def unserialize(self, skel: "SkeletonInstance", name) -> t.Literal[True]:
         """
-        Unserialize the Captcha bone.
+        Stores the publicKey in the SkeletonInstance
 
-        :param skel: The SkeletonInstance containing the Captcha bone.
-        :param name: The name of the Captcha bone.
+        :param skel: The target :class:`SkeletonInstance`.
+        :param name: The name of the CaptchaBone in the :class:`SkeletonInstance`.
 
         :returns: boolean, that is true, as the Captcha bone is always unserialized successfully.
         """
         skel.accessedValues[name] = self.publicKey
         return True
 
-    def fromClient(self, skel: 'SkeletonInstance', name: str, data: dict) -> None | list[ReadFromClientError]:
+    def fromClient(self, skel: "SkeletonInstance", name: str, data: dict) -> None | list[ReadFromClientError]:
         """
-            Reads a value from the client.
-            If this value is valid for this bone,
-            store this value and return None.
-            Otherwise our previous value is
-            left unchanged and an error-message
-            is returned.
-
-            :param name: Our name in the skeleton
-            :param data: *User-supplied* request-data
-            :returns: None or a list of errors
+        Load the reCAPTCHA token from the provided data and validate it with the help of the API.
+
+        reCAPTCHA provides the token via callback usually as "g-recaptcha-response",
+        but to fit into the skeleton logic, we support both names.
+        So the token can be provided as "g-recaptcha-response" or the name of the CaptchaBone in the Skeleton.
+        While the latter one is the preferred name.
         """
-        if conf.instance.is_dev_server:  # We dont enforce captchas on dev server
+        if not conf.security.captcha_enforce_always and conf.instance.is_dev_server:
+            logging.info("Skipping captcha validation on development server")
             return None
-        if (user := current.user.get()) and "root" in user["access"]:
-            return None  # Don't bother trusted users with this (not supported by admin/vi anyways)
-        if not "g-recaptcha-response" in data:
+        if not conf.security.captcha_enforce_always and (user := current.user.get()) and "root" in user["access"]:
+            logging.info("Skipping captcha validation for root user")
+            return None  # Don't bother trusted users with this (not supported by admin/vi anyway)
+        if name not in data and "g-recaptcha-response" not in data:
             return [ReadFromClientError(ReadFromClientErrorSeverity.NotSet, "No Captcha given!")]
-        data = {
-            "secret": self.privateKey,
-            "remoteip": current.request.get().request.remote_addr,
-            "response": data["g-recaptcha-response"]
-        }
-        req = urllib.request.Request(url="https://www.google.com/recaptcha/api/siteverify",
-                                     data=urllib.parse.urlencode(data).encode(),
-                                     method="POST")
-        response = urllib.request.urlopen(req)
-        if json.loads(response.read()).get("success"):
-            return None
-        return [ReadFromClientError(ReadFromClientErrorSeverity.Invalid, "Invalid Captcha")]
+
+        result = requests.post(
+            url="https://www.google.com/recaptcha/api/siteverify",
+            data={
+                "secret": self.privateKey,
+                "remoteip": current.request.get().request.remote_addr,
+                "response": data.get(name, data.get("g-recaptcha-response")),
+            },
+            timeout=10,
+        )
+        if not result.ok:
+            logging.error(f"{result.status_code} {result.reason}: {result.text}")
+            raise ValueError(f"Request to reCAPTCHA failed: {result.status_code} {result.reason}")
+        data = result.json()
+        logging.debug(f"Captcha verification {data=}")
+
+        if not data.get("success"):
+            logging.error(data.get("error-codes"))
+            return [ReadFromClientError(
+                ReadFromClientErrorSeverity.Invalid,
+                f'Invalid Captcha: {", ".join(data.get("error-codes", []))}'
+            )]
+
+        if "score" in data and data["score"] < self.score_threshold:
+            # it's reCAPTCHA v3; check the score
+            return [ReadFromClientError(
+                ReadFromClientErrorSeverity.Invalid,
+                f'Invalid Captcha: {data["score"]} is lower than threshold {self.score_threshold}'
+            )]
+
+        return None  # okay

src/viur/core/config.py

@@ -2,9 +2,9 @@
 import hashlib
 import logging
 import os
+import typing as t
 import warnings
 from pathlib import Path
-import typing as t
 
 import google.auth
 
@@ -23,6 +23,12 @@
 Multiple: t.TypeAlias = list[_T] | tuple[_T] | set[_T] | frozenset[_T]  # TODO: Refactor for Python 3.12
 
 
+class CaptchaDefaultCredentialsType(t.TypedDict):
+    """Expected type of global captcha credential, see :attr:`Security.captcha_default_credentials`"""
+    sitekey: str
+    secret: str
+
+
 class ConfigType:
     """An abstract class for configurations.
 
@@ -373,8 +379,8 @@ class Security(ConfigType):
     Use security.enableStrictTransportSecurity to set this property"""
 
     x_frame_options: t.Optional[
-        tuple[t.Literal["deny", "sameorigin", "allow-from"],
-              t.Optional[str]]] = ("sameorigin", None)
+        tuple[t.Literal["deny", "sameorigin", "allow-from"], t.Optional[str]]
+    ] = ("sameorigin", None)
     """If set, ViUR will emit an X-Frame-Options header
 
     In case of allow-from, the second parameters must be the host-url.
@@ -390,11 +396,18 @@ class Security(ConfigType):
     x_permitted_cross_domain_policies: t.Optional[t.Literal["none", "master-only", "by-content-type", "all"]] = "none"
     """Unless set to logical none; ViUR will emit a X-Permitted-Cross-Domain-Policies with each request"""
 
-    captcha_default_credentials: t.Optional[dict[t.Literal["sitekey", "secret"], str]] = None
-    """The default sitekey and secret to use for the captcha-bone.
+    captcha_default_credentials: t.Optional[CaptchaDefaultCredentialsType] = None
+    """The default sitekey and secret to use for the :class:`CaptchaBone`.
     If set, must be a dictionary of "sitekey" and "secret".
     """
 
+    captcha_enforce_always: bool = False
+    """By default a captcha of the :class:`CaptchaBone` must not be solved on a local development server
+    or by a root user. But for development it can be helpful to test the implementation
+    on a local development server. Setting this flag to True, disables this behavior and
+    enforces always a valid captcha.
+    """
+
     password_recovery_key_length: int = 42
     """Length of the Password recovery key"""