security: gh security recs (#3368)

* change ALLOW_SIGNUP to default to false

* add 1.4.0 tag for OIDC docs

* new notes on security inline with security/policy review

* safer transport for external requests

* fix linter errors

* docs: Tidy up wording/formatting

* fix request errors

* whoops

* fix implementation with std lib

* format

* Remove check on netloc_parts. It only includes URL after any @

---------

Co-authored-by: boc-the-git <3479092+boc-the-git@users.noreply.github.com>
Co-authored-by: Brendan <b.oconnell14@gmail.com>

docs/docs/documentation/getting-started/authentication/oidc.md

@@ -1,5 +1,7 @@
 # OpenID Connect (OIDC) Authentication
 
+:octicons-tag-24: v1.4.0
+
 Mealie supports 3rd party authentication via [OpenID Connect (OIDC)](https://openid.net/connect/), an identity layer built on top of OAuth2. OIDC is supported by many Identity Providers (IdP), including:
 
 - [Authentik](https://goauthentik.io/integrations/sources/oauth/#openid-connect)

docs/docs/documentation/getting-started/installation/backend-config.md

@@ -4,17 +4,19 @@
 
 ### General
 
-| Variables     |        Default        | Description                                                                         |
-| ------------- | :-------------------: | ----------------------------------------------------------------------------------- |
-| PUID          |          911          | UserID permissions between host OS and container                                    |
-| PGID          |          911          | GroupID permissions between host OS and container                                   |
-| DEFAULT_GROUP |         Home          | The default group for users                                                         |
-| BASE_URL      | http://localhost:8080 | Used for Notifications                                                              |
-| TOKEN_TIME    |          48           | The time in hours that a login/auth token is valid                                  |
-| API_PORT      |         9000          | The port exposed by backend API. **Do not change this if you're running in Docker** |
-| API_DOCS      |         True          | Turns on/off access to the API documentation locally.                               |
-| TZ            |          UTC          | Must be set to get correct date/time on the server                                  |
-| ALLOW_SIGNUP  |         true          | Allow user sign-up without token                                                    |
+| Variables                     |        Default        | Description                                                                         |
+| ----------------------------- | :-------------------: | ----------------------------------------------------------------------------------- |
+| PUID                          |          911          | UserID permissions between host OS and container                                    |
+| PGID                          |          911          | GroupID permissions between host OS and container                                   |
+| DEFAULT_GROUP                 |         Home          | The default group for users                                                         |
+| BASE_URL                      | http://localhost:8080 | Used for Notifications                                                              |
+| TOKEN_TIME                    |          48           | The time in hours that a login/auth token is valid                                  |
+| API_PORT                      |         9000          | The port exposed by backend API. **Do not change this if you're running in Docker** |
+| API_DOCS                      |         True          | Turns on/off access to the API documentation locally.                               |
+| TZ                            |          UTC          | Must be set to get correct date/time on the server                                  |
+| ALLOW_SIGNUP<super>\*</super> |         false         | Allow user sign-up without token                                                    |
+
+<super>\*</super> Starting in v1.4.0 this was changed to default to `false` as apart of a security review of the application.
 
 ### Security
 
@@ -77,20 +79,22 @@ Changing the webworker settings may cause unforeseen memory leak issues with Mea
 
 ### OpenID Connect (OIDC)
 
+:octicons-tag-24: v1.4.0
+
 For usage, see [Usage - OpenID Connect](../authentication/oidc.md)
 
-| Variables | Default | Description |
-| --- | :--: | --- |
-| OIDC_AUTH_ENABLED | False | Enables authentication via OpenID Connect |
-| OIDC_SIGNUP_ENABLED | True | Enables new users to be created when signing in for the first time with OIDC |
-| OIDC_CONFIGURATION_URL | None | The URL to the OIDC configuration of your provider. This is usually something like https://auth.example.com/.well-known/openid-configuration |
-| OIDC_CLIENT_ID | None | The client id of your configured client in your provider |
-| OIDC_USER_GROUP| None | If specified, only users belonging to this group will be able to successfully authenticate, regardless of the `OIDC_ADMIN_GROUP`. For more information see [this page](../authentication/oidc.md#groups) |
-| OIDC_ADMIN_GROUP | None | If specified, users belonging to this group will be made an admin. For more information see [this page](../authentication/oidc.md#groups) |
-| OIDC_AUTO_REDIRECT | False | If `True`, then the login page will be bypassed an you will be sent directly to your Identity Provider. You can still get to the login page by adding `?direct=1` to the login URL |
-| OIDC_PROVIDER_NAME | OAuth | The provider name is shown in SSO login button. "Login with <OIDC_PROVIDER_NAME\>" |
-| OIDC_REMEMBER_ME | False | Because redirects bypass the login screen, you cant extend your session by clicking the "Remember Me" checkbox. By setting this value to true, a session will be extended as if "Remember Me" was checked |
-| OIDC_SIGNING_ALGORITHM | RS256 | The algorithm used to sign the id token (examples: RS256, HS256) |
+| Variables              | Default | Description                                                                                                                                                                                               |
+| ---------------------- | :-----: | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| OIDC_AUTH_ENABLED      |  False  | Enables authentication via OpenID Connect                                                                                                                                                                 |
+| OIDC_SIGNUP_ENABLED    |  True   | Enables new users to be created when signing in for the first time with OIDC                                                                                                                              |
+| OIDC_CONFIGURATION_URL |  None   | The URL to the OIDC configuration of your provider. This is usually something like https://auth.example.com/.well-known/openid-configuration                                                              |
+| OIDC_CLIENT_ID         |  None   | The client id of your configured client in your provider                                                                                                                                                  |
+| OIDC_USER_GROUP        |  None   | If specified, only users belonging to this group will be able to successfully authenticate, regardless of the `OIDC_ADMIN_GROUP`. For more information see [this page](../authentication/oidc.md#groups)  |
+| OIDC_ADMIN_GROUP       |  None   | If specified, users belonging to this group will be made an admin. For more information see [this page](../authentication/oidc.md#groups)                                                                 |
+| OIDC_AUTO_REDIRECT     |  False  | If `True`, then the login page will be bypassed an you will be sent directly to your Identity Provider. You can still get to the login page by adding `?direct=1` to the login URL                        |
+| OIDC_PROVIDER_NAME     |  OAuth  | The provider name is shown in SSO login button. "Login with <OIDC_PROVIDER_NAME\>"                                                                                                                        |
+| OIDC_REMEMBER_ME       |  False  | Because redirects bypass the login screen, you cant extend your session by clicking the "Remember Me" checkbox. By setting this value to true, a session will be extended as if "Remember Me" was checked |
+| OIDC_SIGNING_ALGORITHM |  RS256  | The algorithm used to sign the id token (examples: RS256, HS256)                                                                                                                                          |
 
 ### Themeing
 
@@ -113,7 +117,6 @@ Setting the following environmental variables will change the theme of the front
 | THEME_DARK_WARNING    | #FF6D00 | Dark Theme Config Variable  |
 | THEME_DARK_ERROR      | #EF5350 | Dark Theme Config Variable  |
 
-
 [workers_per_core]: https://github.com/tiangolo/uvicorn-gunicorn-docker/blob/2daa3e3873c837d5781feb4ff6a40a89f791f81b/README.md#workers_per_core
 [max_workers]: https://github.com/tiangolo/uvicorn-gunicorn-docker/blob/2daa3e3873c837d5781feb4ff6a40a89f791f81b/README.md#max_workers
 [web_concurrency]: https://github.com/tiangolo/uvicorn-gunicorn-docker/blob/2daa3e3873c837d5781feb4ff6a40a89f791f81b/README.md#web_concurrency

docs/docs/documentation/getting-started/installation/security.md

@@ -0,0 +1,43 @@
+---
+tags:
+  - Security
+---
+
+# Security Considerations
+
+This page is a collection of security considerations for Mealie. It mostly deals with reported issues and how it's possible to mitigate them. Note that this page is for you to use as a guide for how secure you want to make your deployment. It's important to note that most of these will not apply to you, if you:
+
+1. Run behind a VPN
+2. Use a strong password
+3. Disable Sign-Ups
+4. Don't host for malicious users
+
+Use your best judgement when deciding what to do.
+
+## Denial of Service
+
+By default, the API is **not** rate limited. This leaves Mealie open to a potential **Denial of Service Attack**. While it's possible to perform a **Denial of Service Attack** on any endpoint, there are a few key endpoints that are more vulnerable than others.
+
+- `/api/recipes/create-url`
+- `/api/recipes/{id}/image`
+
+These endpoints are used to scrape data based off a user provided URL. It is possible for a malicious user to issue multiple requests to download an arbitrarily large external file (e.g a Debian ISO) and sufficiently saturate a CPU assigned to the container. While we do implement some protections against this by chunking the response, and using a timeout strategy, it's still possible to overload the CPU if an attacker issues multiple requests concurrently.
+
+### Mitigation
+
+If you'd like to mitigate this risk, we suggest that you rate limit the API in general, and apply strict rate limits to these endpoints. You can do this by utilizing a reverse proxy. See the following links to get started:
+
+- [Traefik](https://doc.traefik.io/traefik/middlewares/http/ratelimit/)
+- [Nginx](https://nginx.org/en/docs/http/ngx_http_limit_req_module.html)
+- [Caddy](https://caddyserver.com/docs/modules/http.handlers.rate_limit)
+
+## Server Side Request Forgery
+
+- `/api/recipes/create-url`
+- `/api/recipes/{id}/image`
+
+Given the nature of these APIs it's possible to perform a **Server Side Request Forgery** attack. This is where a malicious user can issue a request to an internal network resource, and potentially exfiltrate data. We _do_ perform some checks to mitigate access to resources within your network but at the end of the day, users of Mealie are allowed to trigger HTTP requests on **your server**.
+
+### Mitigation
+
+If you'd like to mitigate this risk, we suggest that you isolate the container that Mealie is running in to ensure that it's access to internal resources is limited only to what is required. _Note that Mealie does require access to the internet for recipe imports._ You might consider isolating Mealie from your home network entirely and only allowing access to the external internet.

docs/mkdocs.yml

@@ -72,6 +72,7 @@ nav:
           - SQLite (Recommended): "documentation/getting-started/installation/sqlite.md"
           - PostgreSQL: "documentation/getting-started/installation/postgres.md"
           - Backend Configuration: "documentation/getting-started/installation/backend-config.md"
+          - Security: "documentation/getting-started/installation/security.md"
       - Usage:
           - Backup and Restoring: "documentation/getting-started/usage/backups-and-restoring.md"
           - Permissions and Public Access: "documentation/getting-started/usage/permissions-and-public-access.md"

mealie/core/settings/settings.py

@@ -47,7 +47,7 @@ class AppSettings(BaseSettings):
 
     GIT_COMMIT_HASH: str = "unknown"
 
-    ALLOW_SIGNUP: bool = True
+    ALLOW_SIGNUP: bool = False
 
     # ===============================================
     # Security Configuration

mealie/pkgs/safehttp/__init__.py

@@ -0,0 +1,7 @@
+from .transport import AsyncSafeTransport, ForcedTimeoutException, InvalidDomainError
+
+__all__ = [
+    "AsyncSafeTransport",
+    "ForcedTimeoutException",
+    "InvalidDomainError",
+]

mealie/pkgs/safehttp/transport.py

@@ -0,0 +1,78 @@
+import ipaddress
+import logging
+import socket
+
+import httpx
+
+
+class ForcedTimeoutException(Exception):
+    """
+    Raised when a request takes longer than the timeout value.
+    """
+
+    ...
+
+
+class InvalidDomainError(Exception):
+    """
+    Raised when a request is made to a local IP address.
+    """
+
+    ...
+
+
+class AsyncSafeTransport(httpx.AsyncBaseTransport):
+    """
+    A wrapper around the httpx transport class that enforces a timeout value
+    and that the request is not made to a local IP address.
+    """
+
+    timeout: int = 15
+
+    def __init__(self, log: logging.Logger | None = None, **kwargs):
+        self.timeout = kwargs.pop("timeout", self.timeout)
+        self._wrapper = httpx.AsyncHTTPTransport(**kwargs)
+        self._log = log
+
+    async def handle_async_request(self, request):
+        # override timeout value for _all_ requests
+        request.extensions["timeout"] = httpx.Timeout(self.timeout, pool=self.timeout).as_dict()
+
+        # validate the request is not attempting to connect to a local IP
+        # This is a security measure to prevent SSRF attacks
+
+        ip: ipaddress.IPv4Address | ipaddress.IPv6Address | None = None
+
+        netloc = request.url.netloc.decode()
+        if ":" in netloc:  # Either an IP, or a hostname:port combo
+            netloc_parts = netloc.split(":")
+
+            netloc = netloc_parts[0]
+
+            try:
+                ip = ipaddress.ip_address(netloc)
+            except ValueError:
+                if self._log:
+                    self._log.debug(f"failed to parse ip for {netloc=} falling back to domain resolution")
+                pass
+
+        # Request is a domain or a hostname.
+        if not ip:
+            if self._log:
+                self._log.debug(f"resolving IP for domain: {netloc}")
+
+            ip_str = socket.gethostbyname(netloc)
+            ip = ipaddress.ip_address(ip_str)
+
+            if self._log:
+                self._log.debug(f"resolved IP for domain: {netloc} -> {ip}")
+
+        if ip.is_private:
+            if self._log:
+                self._log.warning(f"invalid request on local resource: {request.url} -> {ip}")
+            raise InvalidDomainError(f"invalid request on local resource: {request.url} -> {ip}")
+
+        return await self._wrapper.handle_async_request(request)
+
+    async def aclose(self):
+        await self._wrapper.aclose()

mealie/services/recipe/recipe_data_service.py

@@ -5,7 +5,8 @@
 from httpx import AsyncClient, Response
 from pydantic import UUID4
 
-from mealie.pkgs import img
+from mealie.pkgs import img, safehttp
+from mealie.pkgs.safehttp.transport import AsyncSafeTransport
 from mealie.schema.recipe.recipe import Recipe
 from mealie.services._base_service import BaseService
 
@@ -29,12 +30,14 @@ async def largest_content_len(urls: list[str]) -> tuple[str, int]:
     largest_url = ""
     largest_len = 0
 
+    max_concurrency = 10
+
     async def do(client: AsyncClient, url: str) -> Response:
         return await client.head(url, headers={"User-Agent": _FIREFOX_UA})
 
-    async with AsyncClient() as client:
+    async with AsyncClient(transport=safehttp.AsyncSafeTransport()) as client:
         tasks = [do(client, url) for url in urls]
-        responses: list[Response] = await gather_with_concurrency(10, *tasks, ignore_exceptions=True)
+        responses: list[Response] = await gather_with_concurrency(max_concurrency, *tasks, ignore_exceptions=True)
         for response in responses:
             len_int = int(response.headers.get("Content-Length", 0))
             if len_int > largest_len:
@@ -101,52 +104,39 @@ def write_image(self, file_data: bytes | Path, extension: str, image_dir: Path |
 
         return image_path
 
-    @staticmethod
-    def _validate_image_url(url: str) -> bool:
-        # sourcery skip: invert-any-all, use-any
-        """
-        Validates that the URL is of an allowed source and restricts certain sources to prevent
-        malicious images from being downloaded.
-        """
-        invalid_domains = {"127.0.0.1", "localhost"}
-        for domain in invalid_domains:
-            if domain in url:
-                return False
-
-        return True
-
-    async def scrape_image(self, image_url) -> None:
+    async def scrape_image(self, image_url: str | dict[str, str] | list[str]) -> None:
         self.logger.info(f"Image URL: {image_url}")
 
-        if not self._validate_image_url(image_url):
-            self.logger.error(f"Invalid image URL: {image_url}")
-            raise InvalidDomainError(f"Invalid domain: {image_url}")
+        image_url_str = ""
 
         if isinstance(image_url, str):  # Handles String Types
-            pass
+            image_url_str = image_url
 
         elif isinstance(image_url, list):  # Handles List Types
             # Multiple images have been defined in the schema - usually different resolutions
             # Typically would be in smallest->biggest order, but can't be certain so test each.
             # 'Google will pick the best image to display in Search results based on the aspect ratio and resolution.'
-            image_url, _ = await largest_content_len(image_url)
+            image_url_str, _ = await largest_content_len(image_url)
 
         elif isinstance(image_url, dict):  # Handles Dictionary Types
             for key in image_url:
                 if key == "url":
-                    image_url = image_url.get("url")
+                    image_url_str = image_url.get("url", "")
+
+        if not image_url_str:
+            raise ValueError(f"image url could not be parsed from input: {image_url}")
 
-        ext = image_url.split(".")[-1]
+        ext = image_url_str.split(".")[-1]
 
         if ext not in img.IMAGE_EXTENSIONS:
             ext = "jpg"  # Guess the extension
 
         file_name = f"{str(self.recipe_id)}.{ext}"
         file_path = Recipe.directory_from_id(self.recipe_id).joinpath("images", file_name)
 
-        async with AsyncClient() as client:
+        async with AsyncClient(transport=AsyncSafeTransport()) as client:
             try:
-                r = await client.get(image_url, headers={"User-Agent": _FIREFOX_UA})
+                r = await client.get(image_url_str, headers={"User-Agent": _FIREFOX_UA})
             except Exception:
                 self.logger.exception("Fatal Image Request Exception")
                 return None

mealie/services/scraper/scraper.py

@@ -43,7 +43,7 @@ async def create_from_url(url: str, translator: Translator) -> tuple[Recipe, Scr
     recipe_data_service = RecipeDataService(new_recipe.id)
 
     try:
-        await recipe_data_service.scrape_image(new_recipe.image)
+        await recipe_data_service.scrape_image(new_recipe.image)  # type: ignore
 
         if new_recipe.name is None:
             new_recipe.name = "Untitled"

mealie/services/scraper/scraper_strategies.py

@@ -12,6 +12,7 @@
 
 from mealie.core.root_logger import get_logger
 from mealie.lang.providers import Translator
+from mealie.pkgs import safehttp
 from mealie.schema.recipe.recipe import Recipe, RecipeStep
 from mealie.services.scraper.scraped_extras import ScrapedExtras
 
@@ -31,7 +32,7 @@ async def safe_scrape_html(url: str) -> str:
     if the request takes longer than 15 seconds. This is used to mitigate
     DDOS attacks from users providing a url with arbitrary large content.
     """
-    async with AsyncClient() as client:
+    async with AsyncClient(transport=safehttp.AsyncSafeTransport()) as client:
         html_bytes = b""
         async with client.stream("GET", url, timeout=SCRAPER_TIMEOUT, headers={"User-Agent": _FIREFOX_UA}) as resp:
             start_time = time.time()