diff --git a/.env.example b/.env.example index af8a75d..1b294d1 100644 --- a/.env.example +++ b/.env.example @@ -9,6 +9,11 @@ APP_NAME=OpenMind # Port publie sur l'hote. L'application ecoute toujours sur 8080 dans le conteneur. APP_PORT=8080 +# Interface sur laquelle le port est publie. En production derriere un reverse +# proxy installe sur l'hote, mettez 127.0.0.1 : OpenMind n'est alors joignable +# que par le proxy, jamais directement depuis Internet. +APP_BIND=0.0.0.0 + # Base SQLite des cartes PARTAGEES uniquement. # /app/data est monte depuis ./data sur l'hote (bind mount). DATABASE_URL=sqlite:////app/data/openmind.db @@ -26,8 +31,10 @@ MAX_NODES=5000 SHARING_ENABLED=true # Limites de debit, au format / (second, minute, hour, day). -# Vide ou 0 desactive la limite. Les compteurs sont en memoire : ils repartent -# de zero au redemarrage du conteneur. +# Pour desactiver une limite : 0, off, none, disabled, ou une valeur vide. +# Toute autre valeur illisible (10/hours, 10/banana...) ARRETE le demarrage : +# une faute de frappe ne doit pas desactiver silencieusement une protection. +# Les compteurs sont en memoire : ils repartent de zero au redemarrage. SHARE_CREATE_RATE_LIMIT=10/hour SHARE_READ_RATE_LIMIT=120/minute SHARE_WRITE_RATE_LIMIT=60/hour @@ -46,8 +53,18 @@ ALLOW_PERMANENT_SHARES=true # A activer UNIQUEMENT derriere un reverse proxy de confiance : sinon un client # direct pourrait falsifier X-Forwarded-For et contourner les limites de debit. +# Activer TRUST_PROXY_HEADERS impose de renseigner TRUSTED_PROXY_CIDRS : sans +# reseau declare, aucun en-tete ne serait accepte, et le demarrage echoue plutot +# que de laisser croire a une configuration active. TRUST_PROXY_HEADERS=false +# Reseaux depuis lesquels X-Forwarded-For est accepte (IPv4, IPv6, CIDR), +# separes par des virgules. Exemple pour un proxy dans Docker : +# TRUSTED_PROXY_CIDRS=172.16.0.0/12,10.0.0.0/8 +# Pour un Nginx sur l'hote, avec un port publie sur 127.0.0.1 : +# TRUSTED_PROXY_CIDRS=127.0.0.1/32,::1/128 +TRUSTED_PROXY_CIDRS= + # Origines autorisees en CORS, separees par des virgules. # Vide = aucune origine externe (l'interface est servie par ce meme serveur). CORS_ORIGINS= diff --git a/README.md b/README.md index c12e899..6c6f07a 100644 --- a/README.md +++ b/README.md @@ -121,6 +121,7 @@ Toutes definies dans `.env` (modele : `.env.example`). Aucun secret n'est requis |---|---|---| | `APP_NAME` | `OpenMind` | Nom du produit, affiche dans l'interface et l'onglet | | `APP_PORT` | `8080` | Port publie sur l'hote (le conteneur ecoute toujours 8080) | +| `APP_BIND` | `0.0.0.0` | Interface de publication. `127.0.0.1` en production derriere un proxy local | | `DATABASE_URL` | `sqlite:////app/data/openmind.db` | Base des cartes partagees | | `MAX_MAP_SIZE_MB` | `10` | Taille maximale d'une carte envoyee au serveur | | `MAX_NODES` | `5000` | Nombre maximal de noeuds par carte | @@ -140,10 +141,26 @@ Exposition publique : | `DEFAULT_SHARE_EXPIRY_DAYS` | `30` | Duree de vie par defaut d'un nouveau partage. `0` = permanent | | `ALLOW_PERMANENT_SHARES` | `true` | Autorise l'option "Jamais" dans le choix d'expiration | | `TRUST_PROXY_HEADERS` | `false` | Croire `X-Forwarded-For`. **A n'activer que derriere un reverse proxy de confiance** | +| `TRUSTED_PROXY_CIDRS` | vide | Reseaux depuis lesquels `X-Forwarded-For` est accepte. Obligatoire si `TRUST_PROXY_HEADERS=true` | Format des limites de debit : `/`, ou la fenetre vaut `second`, -`minute`, `hour` ou `day`. Une valeur vide, `0` ou `off` desactive la limite -concernee. +`minute`, `hour` ou `day`. + +**La configuration est validee au demarrage.** Desactiver une limite se fait +avec `0`, `off`, `none`, `disabled` ou une valeur vide. Toute autre valeur +illisible arrete l'application avec un message nommant la variable : + +``` +Configuration invalide : + SHARE_CREATE_RATE_LIMIT : '10/hours' n'est pas une limite de debit valide : + fenetre 'hours' inconnue. Fenetres acceptees : day, hour, minute, second. +``` + +Une faute de frappe n'est pas une desactivation volontaire : sans ce controle, +une instance publique tournerait sans protection en croyant en avoir une. Sont +validees de la meme facon : `MAX_SHARED_MAPS >= 0`, `MAX_SHARE_SIZE_MB >= 0` +(`0` = reprend `MAX_MAP_SIZE_MB`), `MAX_MAP_SIZE_MB > 0`, +`DEFAULT_SHARE_EXPIRY_DAYS >= 0`, et chaque entree de `TRUSTED_PROXY_CIDRS`. Le nom du produit est centralise : cote backend via `APP_NAME`, cote frontend dans `frontend/src/config/app.ts`. Le frontend recupere le nom a l'execution via @@ -346,6 +363,61 @@ docker compose exec openmind python -m app.cli restore /app/data/backups/openmin | `backup ` | Sauvegarde a chaud via l'API SQLite. `--yes` ecrase un fichier existant | | `restore ` | Restauration prudente : verification, copie de securite, puis remplacement | +`backup` accepte `--keep N` : apres ecriture, seules les `N` sauvegardes les +plus recentes du meme dossier sont conservees, la nouvelle comprise. Sans +`--keep`, rien n'est jamais supprime. + +### Automatiser la purge et la sauvegarde + +OpenMind n'embarque volontairement **aucun planificateur** : ces commandes +s'appellent depuis l'outillage de la machine. + +Avec cron, une entree quotidienne : + +```cron +# Sauvegarde a 3h15, sept jours de retention +15 3 * * * cd /srv/openmind && docker compose exec -T openmind python -m app.cli backup /app/data/backups/openmind-$(date +\%F).db --yes --keep 7 + +# Purge des partages expires a 3h30 +30 3 * * * cd /srv/openmind && docker compose exec -T openmind python -m app.cli shares purge-expired --yes +``` + +Dans un fichier cron, `%` doit etre echappe : d'ou `\%F`. + +Avec systemd, un service et son minuteur : + +```ini +# /etc/systemd/system/openmind-backup.service +[Unit] +Description=Sauvegarde OpenMind + +[Service] +Type=oneshot +WorkingDirectory=/srv/openmind +ExecStart=/usr/bin/docker compose exec -T openmind python -m app.cli backup /app/data/backups/openmind-daily.db --yes --keep 7 +ExecStart=/usr/bin/docker compose exec -T openmind python -m app.cli shares purge-expired --yes +``` + +```ini +# /etc/systemd/system/openmind-backup.timer +[Unit] +Description=Sauvegarde quotidienne d'OpenMind + +[Timer] +OnCalendar=daily +Persistent=true + +[Install] +WantedBy=timers.target +``` + +```bash +systemctl enable --now openmind-backup.timer +``` + +Les fichiers produits arrivent dans `./data/backups/` sur l'hote : copiez-les +hors de la machine si vous voulez une vraie sauvegarde. + Aucune commande n'affiche le jeton de gestion, son hash, ni le contenu des cartes. Les jetons de partage y sont masques (`sbcY...BCyw`) : suffisant pour identifier une ligne, insuffisant pour ouvrir le lien. @@ -361,11 +433,38 @@ n'est imposee. Points a respecter : 1. Transmettre l'hote et le protocole d'origine. -2. Activer `TRUST_PROXY_HEADERS=true` **uniquement** dans ce cas : la limitation - de debit lit alors la derniere valeur de `X-Forwarded-For`, celle ajoutee par - votre proxy. Laisse a `false`, un client direct pourrait forger cet en-tete et - changer d'identite a chaque requete. -3. Ne pas mettre en cache `/api/`. +2. **Ne pas publier 8080 sur Internet.** Posez `APP_BIND=127.0.0.1` dans `.env` + si le proxy tourne sur l'hote : OpenMind n'est alors joignable que par lui. + Si le proxy tourne dans Docker, retirez entierement la section `ports` et + placez les deux conteneurs sur un reseau Docker commun. +3. Activer `TRUST_PROXY_HEADERS=true` **et** renseigner `TRUSTED_PROXY_CIDRS` + avec les reseaux d'ou viennent les connexions du proxy. Les deux vont + ensemble : activer la confiance sans declarer de reseau arrete le demarrage. +4. Ne pas mettre en cache `/api/`. + +### Comment l'adresse du client est determinee + +`X-Forwarded-For` est un en-tete que n'importe qui peut ecrire. OpenMind ne le +croit qu'a deux conditions cumulees : + +| `TRUST_PROXY_HEADERS` | Connexion venant d'un reseau de `TRUSTED_PROXY_CIDRS` | Adresse retenue | +|---|---|---| +| `false` | peu importe | adresse reelle de la socket | +| `true` | non | adresse reelle de la socket | +| `true` | oui | derniere valeur de `X-Forwarded-For`, sinon `X-Real-IP`, sinon la socket | + +OpenMind suppose **un reverse proxy frontal de confiance**. Un proxy +correctement configure (`$proxy_add_x_forwarded_for` chez Nginx) ajoute a la fin +de la chaine l'adresse qu'il a reellement vue : c'est donc la derniere valeur +qui fait foi, les precedentes pouvant avoir ete forgees par le client. Aucun +mecanisme multi-sauts n'est prevu ; avec plusieurs proxies en cascade, declarez +uniquement celui qui est directement devant OpenMind. + +L'adresse retenue est normalisee avec le module standard `ipaddress` : les +differentes ecritures d'une meme IPv6 (`2001:db8::1`, +`2001:0db8:0000:...:0001`, `2001:DB8::1`) donnent une seule cle de limitation, +et il ne suffit donc pas de reecrire son adresse autrement pour repartir avec un +compteur neuf. Aucune variable `PUBLIC_BASE_URL` n'est necessaire : le serveur ne fabrique jamais d'URL absolue (il renvoie un chemin relatif `/s/`), et le lien @@ -393,17 +492,43 @@ server { } ``` +Cote OpenMind, dans `.env` : + +```ini +APP_BIND=127.0.0.1 +TRUST_PROXY_HEADERS=true +TRUSTED_PROXY_CIDRS=127.0.0.1/32,::1/128 +``` + Exemple Traefik (labels sur le service `openmind` du `docker-compose.yml`) : ```yaml -labels: - - traefik.enable=true - - traefik.http.routers.openmind.rule=Host(`mind.example.com`) - - traefik.http.routers.openmind.entrypoints=websecure - - traefik.http.routers.openmind.tls.certresolver=letsencrypt - - traefik.http.services.openmind.loadbalancer.server.port=8080 +# docker-compose.override.yml — Traefik reste hors du projet. +services: + openmind: + # Plus aucun port publie : seul Traefik joint le conteneur. + ports: !reset [] + networks: [proxy] + environment: + TRUST_PROXY_HEADERS: "true" + TRUSTED_PROXY_CIDRS: "172.16.0.0/12" # reseau du conteneur Traefik + labels: + - traefik.enable=true + - traefik.docker.network=proxy + - traefik.http.routers.openmind.rule=Host(`mind.example.com`) + - traefik.http.routers.openmind.entrypoints=websecure + - traefik.http.routers.openmind.tls.certresolver=letsencrypt + - traefik.http.services.openmind.loadbalancer.server.port=8080 + +networks: + proxy: + external: true ``` +Traefik transmet `X-Forwarded-For`, `X-Forwarded-Proto` et `X-Real-IP` par +defaut. Adaptez `TRUSTED_PROXY_CIDRS` au sous-reseau reel du reseau `proxy` +(`docker network inspect proxy`). + Les journaux du reverse proxy contiennent les URL demandees, jetons de partage compris. Si cela vous derange, configurez leur format ou leur retention : c'est independant d'OpenMind. @@ -913,6 +1038,16 @@ modeles, assainissement des URL. auraient chacun les leurs. C'est un garde-fou contre la saturation triviale d'une instance publique, pas un quota strict. Un reverse proxy peut ajouter sa propre limitation si le besoin est plus exigeant. +- **Capacite maximale** : le comptage et l'insertion sont serialises par un + verrou de processus, ce qui empeche deux creations simultanees de franchir + ensemble la derniere place. Cette garantie vaut pour l'architecture livree : + un seul service applicatif devant un SQLite local. Avec plusieurs workers ou + plusieurs conteneurs, `MAX_SHARED_MAPS` redeviendrait un garde-fou approximatif + et non un quota transactionnel. +- **Un seul niveau de proxy** : OpenMind suppose un reverse proxy frontal de + confiance et lit la derniere valeur de `X-Forwarded-For`. Avec plusieurs + proxies en cascade, declarez uniquement celui qui est directement devant + l'application ; aucun mecanisme multi-sauts n'est prevu. - **Partage expire et information** : un lien expire repond `410` et non `404`, ce qui revele qu'un partage a existe a cette adresse. C'est assume : sans cela, la page publique ne pourrait pas expliquer la difference entre un lien expire diff --git a/backend/app/api/deps.py b/backend/app/api/deps.py index 235f2d8..bbacdda 100644 --- a/backend/app/api/deps.py +++ b/backend/app/api/deps.py @@ -56,7 +56,11 @@ def _rate_limit(bucket: str, spec_name: str): # type: ignore[no-untyped-def] rule = parse_rate(getattr(settings, spec_name)) if rule is None: return - client = resolve_client_ip(request, trust_proxy_headers=settings.trust_proxy_headers) + client = resolve_client_ip( + request, + trust_proxy_headers=settings.trust_proxy_headers, + trusted_networks=settings.trusted_proxy_networks, + ) retry_after = limiter.check(f"{bucket}:{client}", rule) if retry_after is not None: raise RateLimitedError(retry_after) diff --git a/backend/app/cli.py b/backend/app/cli.py index a9014e6..7a975f6 100644 --- a/backend/app/cli.py +++ b/backend/app/cli.py @@ -224,6 +224,36 @@ def _sqlite_backup(source: Path, destination: Path) -> None: origin.backup(copy) +def _apply_retention(destination: Path, keep: int) -> list[Path]: + """Ne garde que les `keep` sauvegardes les plus recentes du meme dossier. + + Le filtre est volontairement etroit : meme dossier, meme extension, et le + fichier qui vient d'etre ecrit est toujours conserve. Une commande de + sauvegarde ne doit pas devenir un effaceur de fichiers surprise. + """ + if keep <= 0: + return [] + + others = sorted( + ( + path + for path in destination.parent.glob(f"*{destination.suffix}") + if path.is_file() and path != destination + ), + key=lambda path: path.stat().st_mtime, + reverse=True, + ) + # La sauvegarde qui vient d'etre ecrite compte comme la plus recente, et + # comme l'une des `keep` conservees : on ne peut donc pas finir avec keep+1. + ordered = [destination, *others] + + removed = [] + for path in ordered[keep:]: + path.unlink() + removed.append(path) + return removed + + def cmd_backup(args: argparse.Namespace) -> int: source = database_path() if source is None or not source.exists(): @@ -237,6 +267,9 @@ def cmd_backup(args: argparse.Namespace) -> int: _sqlite_backup(source, destination) print(f"Sauvegarde ecrite : {destination} ({_format_size(destination.stat().st_size)})") + + for removed in _apply_retention(destination, args.keep): + print(f"Ancienne sauvegarde supprimee : {removed.name}") return 0 @@ -315,6 +348,12 @@ def build_parser() -> argparse.ArgumentParser: backup = sub.add_parser("backup", help="Sauvegarder la base pendant que l'app tourne") backup.add_argument("destination", help="Chemin du fichier de sauvegarde") backup.add_argument("--yes", action="store_true", help="Ecraser un fichier existant") + backup.add_argument( + "--keep", + type=int, + default=0, + help="Ne garder que les N sauvegardes les plus recentes du dossier (0 = tout garder)", + ) backup.set_defaults(func=cmd_backup) restore = sub.add_parser("restore", help="Restaurer une sauvegarde (prudent)") diff --git a/backend/app/config.py b/backend/app/config.py index 9e12f0a..dbfba68 100644 --- a/backend/app/config.py +++ b/backend/app/config.py @@ -1,12 +1,51 @@ -"""Configuration applicative, entierement pilotee par variables d'environnement.""" +"""Configuration applicative, entierement pilotee par variables d'environnement. + +Toute valeur illisible arrete le demarrage avec un message nommant la variable +fautive. Une instance publique ne doit jamais tourner en croyant appliquer une +protection qu'une faute de frappe aurait desactivee. +""" from __future__ import annotations +import ipaddress from functools import lru_cache -from pydantic import Field +from pydantic import Field, ValidationError, field_validator, model_validator from pydantic_settings import BaseSettings, SettingsConfigDict +from app.services.rate_limit import RateLimitSyntaxError, parse_rate + + +class ConfigurationError(RuntimeError): + """Configuration inexploitable : le demarrage doit s'arreter ici.""" + + +def _validate_rate(value: str) -> str: + """Verifie une regle de debit sans la conserver : elle est relue a l'usage.""" + parse_rate(value) # leve RateLimitSyntaxError si la syntaxe est fautive + return value + + +def parse_cidrs(raw: str) -> list[ipaddress.IPv4Network | ipaddress.IPv6Network]: + """Lit une liste de reseaux CIDR separes par des virgules. + + `strict=False` accepte aussi bien `10.0.0.0/8` qu'une adresse seule comme + `192.0.2.7`, qui devient alors un reseau /32. + """ + networks: list[ipaddress.IPv4Network | ipaddress.IPv6Network] = [] + for entry in raw.split(","): + text = entry.strip() + if not text: + continue + try: + networks.append(ipaddress.ip_network(text, strict=False)) + except ValueError: + raise ValueError( + f"'{text}' n'est pas un reseau valide. Attendu : une adresse ou un CIDR, " + "par exemple 172.16.0.0/12, 10.0.0.0/8 ou 2001:db8::/32." + ) from None + return networks + class Settings(BaseSettings): """Parametres du backend. @@ -34,7 +73,8 @@ class Settings(BaseSettings): sharing_enabled: bool = True # Limites de debit, au format "/" : 10/hour, 120/minute, - # 5/second. Une valeur vide ou "0" desactive la limite concernee. + # 5/second. Seules les valeurs 0, off, none, disabled ou vide desactivent + # une limite ; toute autre valeur illisible arrete le demarrage. share_create_rate_limit: str = "10/hour" share_read_rate_limit: str = "120/minute" share_write_rate_limit: str = "60/hour" @@ -50,9 +90,13 @@ class Settings(BaseSettings): # Autorise l'option "Jamais" dans l'interface et via l'API. allow_permanent_shares: bool = True - # A n'activer que derriere un reverse proxy de confiance : sinon un client - # direct pourrait falsifier son adresse et contourner les limites de debit. + # A n'activer que derriere un reverse proxy de confiance, et uniquement avec + # trusted_proxy_cidrs renseigne : sinon un client direct pourrait falsifier + # son adresse et contourner les limites de debit. trust_proxy_headers: bool = False + # Reseaux depuis lesquels X-Forwarded-For est accepte, separes par des + # virgules. IPv4, IPv6 et CIDR acceptes. + trusted_proxy_cidrs: str = "" # Liste d'origines autorisees, separees par des virgules. # Vide = aucune origine externe (l'app est servie par ce meme serveur). @@ -60,6 +104,32 @@ class Settings(BaseSettings): log_level: str = "INFO" + @field_validator("share_create_rate_limit", "share_read_rate_limit", "share_write_rate_limit") + @classmethod + def _check_rate(cls, value: str) -> str: + return _validate_rate(value) + + @field_validator("trusted_proxy_cidrs") + @classmethod + def _check_cidrs(cls, value: str) -> str: + parse_cidrs(value) + return value + + @model_validator(mode="after") + def _check_proxy_trust(self) -> Settings: + """Faire confiance a un proxy sans dire lequel ne protege de rien. + + Autant le dire au demarrage : sans reseau declare, aucun en-tete ne + serait accepte et l'exploitant croirait sa configuration active. + """ + if self.trust_proxy_headers and not parse_cidrs(self.trusted_proxy_cidrs): + raise ValueError( + "TRUST_PROXY_HEADERS=true exige TRUSTED_PROXY_CIDRS : indiquez les reseaux " + "depuis lesquels X-Forwarded-For peut etre accepte, par exemple " + "172.16.0.0/12,10.0.0.0/8." + ) + return self + @property def max_map_size_bytes(self) -> int: return int(self.max_map_size_mb * 1024 * 1024) @@ -75,6 +145,10 @@ class Settings(BaseSettings): def max_share_size_mb_effective(self) -> float: return self.max_share_size_bytes / (1024 * 1024) + @property + def trusted_proxy_networks(self) -> list[ipaddress.IPv4Network | ipaddress.IPv6Network]: + return parse_cidrs(self.trusted_proxy_cidrs) + @property def cors_origin_list(self) -> list[str]: return [origin.strip() for origin in self.cors_origins.split(",") if origin.strip()] @@ -84,6 +158,21 @@ class Settings(BaseSettings): return self.app_env.lower() in {"dev", "development", "debug"} +def _readable_error(error: ValidationError) -> str: + """Traduit une erreur Pydantic en message nommant la variable fautive.""" + lines = [] + for item in error.errors(): + variable = str(item["loc"][0]).upper() if item.get("loc") else "CONFIGURATION" + reason = str(item.get("msg", "")).replace("Value error, ", "") + lines.append(f" {variable} : {reason}") + return "\n".join(lines) + + @lru_cache def get_settings() -> Settings: - return Settings() + try: + return Settings() + except RateLimitSyntaxError as error: # pragma: no cover - filet de securite + raise ConfigurationError(f"Configuration invalide :\n {error}") from None + except ValidationError as error: + raise ConfigurationError(f"Configuration invalide :\n{_readable_error(error)}") from None diff --git a/backend/app/db/migrations.py b/backend/app/db/migrations.py index ba43982..794f441 100644 --- a/backend/app/db/migrations.py +++ b/backend/app/db/migrations.py @@ -22,7 +22,6 @@ from alembic.config import Config from sqlalchemy import inspect from app.config import get_settings -from app.db.session import engine as default_engine logger = logging.getLogger(__name__) @@ -43,6 +42,11 @@ def build_config(database_url: str | None = None, *, configure_logger: bool = Fa def run_migrations(engine=None, database_url: str | None = None) -> None: # type: ignore[no-untyped-def] + # Import tardif : creer le moteur lit la configuration. En le faisant ici + # plutot qu'a l'import du module, une configuration invalide remonte comme + # une erreur attrapable, avec un message d'exploitation et sans traceback. + from app.db.session import engine as default_engine + target = engine or default_engine config = build_config(database_url) @@ -63,5 +67,13 @@ def run_migrations(engine=None, database_url: str | None = None) -> None: # typ if __name__ == "__main__": # pragma: no cover - appele par l'entrypoint Docker import sys + from app.config import ConfigurationError + logging.basicConfig(level="INFO", format="%(levelname)-8s | %(message)s", stream=sys.stdout) - run_migrations() + try: + run_migrations() + except ConfigurationError as error: + # Message d'exploitation, pas de traceback : c'est le fichier .env qu'il + # faut corriger, pas le code. + print(str(error), file=sys.stderr) + raise SystemExit(2) from None diff --git a/backend/app/services/client_ip.py b/backend/app/services/client_ip.py index 1a5a94b..efce4a6 100644 --- a/backend/app/services/client_ip.py +++ b/backend/app/services/client_ip.py @@ -4,16 +4,22 @@ Le point sensible : `X-Forwarded-For` est un en-tete que n'importe quel client peut ecrire. Le croire sans condition permettrait a un client direct de changer d'identite a chaque requete et de contourner toute limite. -La confiance est donc explicite et desactivee par defaut -(`TRUST_PROXY_HEADERS`). Quand elle est activee, on lit la DERNIERE valeur de -`X-Forwarded-For` : un reverse proxy correctement configure ajoute l'adresse -qu'il a reellement vue a la fin de la liste, tandis que les valeurs precedentes -peuvent avoir ete forgees par le client. Cette lecture suppose un seul proxy -directement devant OpenMind, ce qui correspond au deploiement documente. +La confiance est donc doublement conditionnee : + +1. `TRUST_PROXY_HEADERS` doit etre actif ; +2. la connexion doit provenir d'un reseau declare dans `TRUSTED_PROXY_CIDRS`. + +Sans ces deux conditions, l'en-tete est ignore et seule l'adresse reelle de la +socket compte. OpenMind suppose **un reverse proxy frontal de confiance** : on +lit la derniere valeur de la chaine, celle que ce proxy a ajoutee, les +precedentes pouvant avoir ete forgees par le client. """ from __future__ import annotations +import ipaddress +from collections.abc import Sequence + from starlette.requests import Request FORWARDED_FOR = "x-forwarded-for" @@ -21,19 +27,60 @@ REAL_IP = "x-real-ip" UNKNOWN_CLIENT = "unknown" +IpNetwork = ipaddress.IPv4Network | ipaddress.IPv6Network -def resolve_client_ip(request: Request, *, trust_proxy_headers: bool) -> str: - if trust_proxy_headers: + +def normalize_ip(value: str) -> str: + """Forme canonique d'une adresse, pour que la cle de limitation soit unique. + + Sans cela, `2001:db8::1` et `2001:0db8:0000:0000:0000:0000:0000:0001` + designeraient deux clients differents, et il suffirait de reecrire son + adresse autrement pour repartir avec un compteur neuf. + """ + text = value.strip() + if not text: + return UNKNOWN_CLIENT + + # Un port peut trainer dans un en-tete mal configure : [::1]:443, 1.2.3.4:80. + if text.startswith("[") and "]" in text: + text = text[1 : text.index("]")] + elif text.count(":") == 1 and "." in text: + text = text.split(":", 1)[0] + + try: + return str(ipaddress.ip_address(text)) + except ValueError: + # Adresse illisible : on la garde telle quelle, bornee, plutot que de + # regrouper tous les clients douteux sous une meme cle. + return text[:64] + + +def is_trusted_peer(host: str, networks: Sequence[IpNetwork]) -> bool: + if not networks: + return False + try: + address = ipaddress.ip_address(host) + except ValueError: + return False + return any(address in network for network in networks) + + +def resolve_client_ip( + request: Request, + *, + trust_proxy_headers: bool, + trusted_networks: Sequence[IpNetwork] = (), +) -> str: + peer = request.client.host if request.client and request.client.host else "" + + if trust_proxy_headers and is_trusted_peer(peer, trusted_networks): forwarded = request.headers.get(FORWARDED_FOR) if forwarded: parts = [part.strip() for part in forwarded.split(",") if part.strip()] if parts: - return parts[-1] + return normalize_ip(parts[-1]) real_ip = request.headers.get(REAL_IP) if real_ip and real_ip.strip(): - return real_ip.strip() + return normalize_ip(real_ip) - client = request.client - if client and client.host: - return client.host - return UNKNOWN_CLIENT + return normalize_ip(peer) if peer else UNKNOWN_CLIENT diff --git a/backend/app/services/rate_limit.py b/backend/app/services/rate_limit.py index c8fd1c9..e0914af 100644 --- a/backend/app/services/rate_limit.py +++ b/backend/app/services/rate_limit.py @@ -33,6 +33,14 @@ _WINDOW_SECONDS = { # Garde-fou memoire : au-dela, les entrees les plus anciennes sont oubliees. MAX_TRACKED_CLIENTS = 20000 +# Seules ces valeurs desactivent volontairement une limite. Toute autre valeur +# non analysable est une faute de frappe, pas une intention. +DISABLED_VALUES = frozenset({"", "0", "off", "none", "disabled"}) + + +class RateLimitSyntaxError(ValueError): + """Regle de debit illisible. Remonte comme erreur de configuration.""" + @dataclass(frozen=True) class RateRule: @@ -48,24 +56,39 @@ class RateRule: def parse_rate(spec: str) -> RateRule | None: """Lit une regle "10/hour". Retourne None si la limite est desactivee. - Une valeur vide, "0", ou une syntaxe non reconnue desactive la limite : - une configuration erronee ne doit jamais rendre l'instance inutilisable. + Desactiver une limite se fait explicitement : valeur vide, `0`, `off`, + `none` ou `disabled`. Toute autre valeur illisible leve une erreur plutot + que d'etre silencieusement traitee comme une desactivation : une faute de + frappe comme `10/hours` laisserait sinon l'instance sans protection, sans + que personne ne s'en apercoive. """ text = (spec or "").strip().lower() - if not text or text in {"0", "none", "off", "disabled"}: + if text in DISABLED_VALUES: return None - number, _, unit = text.partition("/") + number, separator, unit = text.partition("/") try: limit = int(number.strip()) except ValueError: - return None + raise RateLimitSyntaxError( + f"'{spec}' n'est pas une limite de debit valide : le nombre est illisible. " + "Attendu : /, par exemple 10/hour." + ) from None if limit <= 0: - return None + raise RateLimitSyntaxError( + f"'{spec}' n'est pas une limite de debit valide : le nombre doit etre positif. " + "Pour desactiver la limite, utilisez 0, off, ou une valeur vide." + ) - window = _WINDOW_SECONDS.get(unit.strip() or "minute") + # "10" vaut 10/minute ; "10/" est une fenetre oubliee, pas un raccourci. + unit_name = unit.strip() if separator else "minute" + window = _WINDOW_SECONDS.get(unit_name) if window is None: - return None + allowed = ", ".join(sorted({"second", "minute", "hour", "day"})) + raise RateLimitSyntaxError( + f"'{spec}' n'est pas une limite de debit valide : fenetre '{unit_name}' inconnue. " + f"Fenetres acceptees : {allowed}." + ) return RateRule(limit=limit, window_seconds=window) diff --git a/backend/app/services/shares.py b/backend/app/services/shares.py index bf762ca..d26bb5e 100644 --- a/backend/app/services/shares.py +++ b/backend/app/services/shares.py @@ -5,6 +5,7 @@ from __future__ import annotations import hashlib import json import logging +import threading from datetime import timedelta from sqlalchemy import func, select @@ -33,6 +34,13 @@ logger = logging.getLogger(__name__) # Duree maximale acceptee pour une expiration explicite (10 ans). MAX_EXPIRY_DAYS = 3650 +# Serialise "compter puis inserer" pour que deux creations simultanees ne +# franchissent pas ensemble la derniere place. Suffisant pour l'architecture +# d'OpenMind : un seul processus applicatif devant un SQLite local. Avec +# plusieurs workers, la limite redeviendrait un garde-fou approximatif (voir +# README, limitations connues). +_capacity_lock = threading.Lock() + def share_ref(share_token: str) -> str: """Reference courte et non reversible d'un partage, pour les journaux. @@ -131,7 +139,6 @@ def create_share( """ payload = serialize_map(document) _guard_payload_size(payload) - _guard_capacity(session) management_token = generate_management_token() share = SharedMap( @@ -142,8 +149,11 @@ def create_share( map_data=payload, expires_at=resolve_expiry(expires_in_days, explicit=expiry_explicit), ) - session.add(share) - session.commit() + + with _capacity_lock: + _guard_capacity(session) + session.add(share) + session.commit() session.refresh(share) logger.info( diff --git a/backend/tests/test_cli.py b/backend/tests/test_cli.py index f96af9a..5cf0b5c 100644 --- a/backend/tests/test_cli.py +++ b/backend/tests/test_cli.py @@ -192,3 +192,39 @@ def test_restore_puts_back_a_backup(client: TestClient, tmp_path: Path, capsys) with SessionLocal() as session: titles = session.execute(select(SharedMap.title)).scalars().all() assert titles == ["Avant sauvegarde"] + + +def test_backup_retention_keeps_the_most_recent(client: TestClient, tmp_path: Path, capsys) -> None: + """--keep N ne conserve que les N sauvegardes les plus recentes du dossier.""" + import os + import time + + _create(client, "Infrastructure") + folder = tmp_path / "backups" + + for index in range(5): + destination = folder / f"openmind-{index}.db" + assert cli.main(["backup", str(destination), "--yes"]) == 0 + # Horodatage distinct : le tri de retention doit etre deterministe. + os.utime(destination, (time.time() + index, time.time() + index)) + capsys.readouterr() + + latest = folder / "openmind-latest.db" + assert cli.main(["backup", str(latest), "--keep", "3"]) == 0 + out = capsys.readouterr().out + assert "Ancienne sauvegarde supprimee" in out + + remaining = sorted(path.name for path in folder.glob("*.db")) + assert len(remaining) == 3 + # La sauvegarde qui vient d'etre ecrite est toujours conservee, et compte + # dans les trois : on ne finit jamais avec une de plus que demande. + assert "openmind-latest.db" in remaining + + +def test_backup_without_keep_deletes_nothing(client: TestClient, tmp_path: Path) -> None: + _create(client) + folder = tmp_path / "backups" + for index in range(3): + assert cli.main(["backup", str(folder / f"openmind-{index}.db")]) == 0 + assert cli.main(["backup", str(folder / "openmind-3.db")]) == 0 + assert len(list(folder.glob("*.db"))) == 4 diff --git a/backend/tests/test_client_ip.py b/backend/tests/test_client_ip.py index d4613ca..2abac2b 100644 --- a/backend/tests/test_client_ip.py +++ b/backend/tests/test_client_ip.py @@ -1,11 +1,20 @@ -"""Identification du client : X-Forwarded-For n'est cru que sur demande.""" +"""Identification du client : X-Forwarded-For n'est cru qu'a deux conditions. + +`TRUST_PROXY_HEADERS` doit etre actif, ET la connexion doit venir d'un reseau +declare dans `TRUSTED_PROXY_CIDRS`. Ces tests reproduisent les deux chemins : +derriere un proxy approuve, et en connexion directe forgeant le meme en-tete. +""" from __future__ import annotations +import pytest from starlette.datastructures import Headers from starlette.requests import Request -from app.services.client_ip import resolve_client_ip +from app.config import parse_cidrs +from app.services.client_ip import is_trusted_peer, normalize_ip, resolve_client_ip + +PROXY_NETWORKS = parse_cidrs("172.16.0.0/12,10.0.0.0/8,2001:db8::/32") def _request(headers: dict[str, str], peer: str | None = "203.0.113.9") -> Request: @@ -19,34 +28,108 @@ def _request(headers: dict[str, str], peer: str | None = "203.0.113.9") -> Reque return Request(scope) -def test_proxy_headers_are_ignored_by_default() -> None: - request = _request({"x-forwarded-for": "1.2.3.4"}) - assert resolve_client_ip(request, trust_proxy_headers=False) == "203.0.113.9" +class TestWithoutTrust: + def test_proxy_headers_are_ignored_by_default(self) -> None: + request = _request({"x-forwarded-for": "1.2.3.4"}) + assert resolve_client_ip(request, trust_proxy_headers=False) == "203.0.113.9" + + def test_forged_header_cannot_change_identity(self) -> None: + """Un client direct ne peut pas se demultiplier pour diluer sa limite.""" + seen = { + resolve_client_ip( + _request({"x-forwarded-for": f"10.0.0.{i}"}), trust_proxy_headers=False + ) + for i in range(20) + } + assert seen == {"203.0.113.9"} + + def test_falls_back_to_peer_without_headers(self) -> None: + assert resolve_client_ip(_request({}), trust_proxy_headers=True) == "203.0.113.9" + + def test_unknown_client_is_handled(self) -> None: + assert resolve_client_ip(_request({}, peer=None), trust_proxy_headers=False) == "unknown" -def test_forged_header_cannot_change_identity() -> None: - """Sans confiance explicite, un client direct ne peut pas se demultiplier.""" - seen = { - resolve_client_ip(_request({"x-forwarded-for": f"10.0.0.{i}"}), trust_proxy_headers=False) - for i in range(20) - } - assert seen == {"203.0.113.9"} +class TestTrustedProxy: + def test_approved_proxy_is_believed(self) -> None: + # Le proxy (172.18.0.1) a ajoute l'adresse reellement vue. + request = _request({"x-forwarded-for": "9.9.9.9, 198.51.100.7"}, peer="172.18.0.1") + resolved = resolve_client_ip( + request, trust_proxy_headers=True, trusted_networks=PROXY_NETWORKS + ) + assert resolved == "198.51.100.7" + + def test_direct_client_forging_the_same_header_is_not_believed(self) -> None: + """Meme en-tete, connexion directe : c'est l'adresse socket qui compte.""" + request = _request({"x-forwarded-for": "9.9.9.9, 198.51.100.7"}, peer="203.0.113.9") + resolved = resolve_client_ip( + request, trust_proxy_headers=True, trusted_networks=PROXY_NETWORKS + ) + assert resolved == "203.0.113.9" + + def test_approved_proxy_falls_back_to_real_ip(self) -> None: + request = _request({"x-real-ip": "198.51.100.8"}, peer="10.1.2.3") + resolved = resolve_client_ip( + request, trust_proxy_headers=True, trusted_networks=PROXY_NETWORKS + ) + assert resolved == "198.51.100.8" + + def test_no_declared_network_means_no_trust(self) -> None: + request = _request({"x-forwarded-for": "198.51.100.7"}, peer="172.18.0.1") + resolved = resolve_client_ip(request, trust_proxy_headers=True, trusted_networks=[]) + assert resolved == "172.18.0.1" + + def test_ipv6_proxy_is_recognised(self) -> None: + request = _request({"x-forwarded-for": "198.51.100.7"}, peer="2001:db8::5") + resolved = resolve_client_ip( + request, trust_proxy_headers=True, trusted_networks=PROXY_NETWORKS + ) + assert resolved == "198.51.100.7" + + def test_peer_membership(self) -> None: + assert is_trusted_peer("172.18.0.1", PROXY_NETWORKS) is True + assert is_trusted_peer("10.255.255.254", PROXY_NETWORKS) is True + assert is_trusted_peer("203.0.113.9", PROXY_NETWORKS) is False + assert is_trusted_peer("2001:db8::1", PROXY_NETWORKS) is True + assert is_trusted_peer("2001:dead::1", PROXY_NETWORKS) is False + assert is_trusted_peer("pas-une-ip", PROXY_NETWORKS) is False + assert is_trusted_peer("10.0.0.1", []) is False -def test_trusted_proxy_uses_the_last_forwarded_value() -> None: - # Le client a forge "9.9.9.9" ; le proxy a ajoute l'adresse reellement vue. - request = _request({"x-forwarded-for": "9.9.9.9, 198.51.100.7"}) - assert resolve_client_ip(request, trust_proxy_headers=True) == "198.51.100.7" +class TestNormalisation: + @pytest.mark.parametrize( + ("raw", "expected"), + [ + ("2001:db8::1", "2001:db8::1"), + ("2001:0db8:0000:0000:0000:0000:0000:0001", "2001:db8::1"), + ("2001:DB8::1", "2001:db8::1"), + ("2001:0db8::0:1", "2001:db8::1"), + ("[2001:db8::1]:443", "2001:db8::1"), + (" 192.0.2.7 ", "192.0.2.7"), + ("192.0.2.7:8080", "192.0.2.7"), + ], + ) + def test_same_address_gives_the_same_key(self, raw: str, expected: str) -> None: + assert normalize_ip(raw) == expected + def test_ipv6_rewriting_cannot_reset_the_counter(self) -> None: + """Plusieurs ecritures de la meme IPv6 doivent produire une seule cle.""" + forms = [ + "2001:db8::1", + "2001:0DB8:0000:0000:0000:0000:0000:0001", + "2001:db8:0:0:0:0:0:1", + "[2001:db8::1]:51000", + ] + keys = { + resolve_client_ip( + _request({"x-forwarded-for": form}, peer="172.18.0.1"), + trust_proxy_headers=True, + trusted_networks=PROXY_NETWORKS, + ) + for form in forms + } + assert keys == {"2001:db8::1"} -def test_trusted_proxy_falls_back_to_real_ip() -> None: - request = _request({"x-real-ip": "198.51.100.8"}) - assert resolve_client_ip(request, trust_proxy_headers=True) == "198.51.100.8" - - -def test_falls_back_to_peer_without_headers() -> None: - assert resolve_client_ip(_request({}), trust_proxy_headers=True) == "203.0.113.9" - - -def test_unknown_client_is_handled() -> None: - assert resolve_client_ip(_request({}, peer=None), trust_proxy_headers=False) == "unknown" + def test_unreadable_value_is_kept_bounded(self) -> None: + assert normalize_ip("x" * 500) == "x" * 64 + assert normalize_ip(" ") == "unknown" diff --git a/backend/tests/test_configuration.py b/backend/tests/test_configuration.py new file mode 100644 index 0000000..138e47b --- /dev/null +++ b/backend/tests/test_configuration.py @@ -0,0 +1,153 @@ +"""Validation de la configuration au demarrage. + +Une valeur illisible doit arreter l'application avec un message nommant la +variable fautive, jamais desactiver silencieusement une protection. +""" + +from __future__ import annotations + +import pytest +from pydantic import ValidationError + +from app.config import ConfigurationError, Settings, get_settings, parse_cidrs +from app.services.rate_limit import RateLimitSyntaxError, parse_rate + + +@pytest.mark.parametrize( + ("spec", "limit", "window"), + [ + ("10/hour", 10, 3600), + ("120/minute", 120, 60), + ("5/second", 5, 1), + ("2/day", 2, 86400), + ("10/h", 10, 3600), + (" 30/MINUTE ", 30, 60), + ("3", 3, 60), + ], +) +def test_valid_rate_specs(spec: str, limit: int, window: int) -> None: + rule = parse_rate(spec) + assert rule is not None + assert rule.limit == limit + assert rule.window_seconds == window + + +@pytest.mark.parametrize("spec", ["", "0", "off", "none", "disabled", " OFF "]) +def test_explicit_deactivation(spec: str) -> None: + """Desactiver une limite reste possible, mais doit etre explicite.""" + assert parse_rate(spec) is None + + +@pytest.mark.parametrize( + "spec", + ["10/hours", "abc", "-1/hour", "10/banana", "10/", "/hour", "dix/hour", "10 par heure"], +) +def test_invalid_rate_specs_are_refused(spec: str) -> None: + with pytest.raises(RateLimitSyntaxError): + parse_rate(spec) + + +def test_invalid_rate_message_is_actionable() -> None: + with pytest.raises(RateLimitSyntaxError) as error: + parse_rate("10/hours") + message = str(error.value) + assert "10/hours" in message + assert "hours" in message + assert "second" in message and "hour" in message + + +class TestSettingsValidation: + def test_valid_configuration_loads(self) -> None: + settings = Settings( + share_create_rate_limit="10/hour", + share_read_rate_limit="120/minute", + share_write_rate_limit="off", + ) + assert settings.share_write_rate_limit == "off" + + @pytest.mark.parametrize( + "field", + ["share_create_rate_limit", "share_read_rate_limit", "share_write_rate_limit"], + ) + def test_each_rate_variable_is_validated(self, field: str) -> None: + with pytest.raises(Exception) as error: + Settings(**{field: "10/banana"}) + assert "banana" in str(error.value) + + def test_negative_shared_map_limit_is_refused(self) -> None: + with pytest.raises(ValidationError): + Settings(max_shared_maps=-1) + + def test_negative_share_size_is_refused(self) -> None: + with pytest.raises(ValidationError): + Settings(max_share_size_mb=-2) + + def test_zero_share_size_means_inherit(self) -> None: + settings = Settings(max_share_size_mb=0, max_map_size_mb=10) + assert settings.max_share_size_bytes == settings.max_map_size_bytes + + def test_negative_expiry_is_refused(self) -> None: + with pytest.raises(ValidationError): + Settings(default_share_expiry_days=-1) + + def test_zero_expiry_is_allowed(self) -> None: + assert Settings(default_share_expiry_days=0).default_share_expiry_days == 0 + + def test_zero_map_size_is_refused(self) -> None: + with pytest.raises(ValidationError): + Settings(max_map_size_mb=0) + + +class TestTrustedProxyConfiguration: + def test_valid_cidrs(self) -> None: + networks = parse_cidrs("172.16.0.0/12, 10.0.0.0/8 ,2001:db8::/32") + assert len(networks) == 3 + assert str(networks[0]) == "172.16.0.0/12" + + def test_single_address_becomes_a_host_network(self) -> None: + assert str(parse_cidrs("192.0.2.7")[0]) == "192.0.2.7/32" + + def test_empty_value_is_allowed(self) -> None: + assert parse_cidrs("") == [] + assert parse_cidrs(" , ") == [] + + @pytest.mark.parametrize("value", ["999.1.1.1/8", "10.0.0.0/64", "pas-un-reseau", "10.0.0.0/"]) + def test_invalid_cidr_is_refused(self, value: str) -> None: + with pytest.raises(ValueError, match="reseau valide"): + parse_cidrs(value) + + def test_invalid_cidr_stops_settings(self) -> None: + with pytest.raises(Exception) as error: + Settings(trusted_proxy_cidrs="10.0.0.0/8,pas-un-reseau") + assert "pas-un-reseau" in str(error.value) + + def test_trusting_proxies_without_declaring_them_is_refused(self) -> None: + """Faire confiance sans dire a qui n'appliquerait aucune confiance.""" + with pytest.raises(Exception) as error: + Settings(trust_proxy_headers=True, trusted_proxy_cidrs="") + assert "TRUSTED_PROXY_CIDRS" in str(error.value) + + def test_trusting_proxies_with_networks_is_accepted(self) -> None: + settings = Settings(trust_proxy_headers=True, trusted_proxy_cidrs="172.16.0.0/12") + assert len(settings.trusted_proxy_networks) == 1 + + +class TestStartupFailure: + def test_bad_rate_stops_startup_with_a_readable_message(self, monkeypatch) -> None: + monkeypatch.setenv("SHARE_CREATE_RATE_LIMIT", "10/banana") + get_settings.cache_clear() + with pytest.raises(ConfigurationError) as error: + get_settings() + message = str(error.value) + assert "Configuration invalide" in message + assert "SHARE_CREATE_RATE_LIMIT" in message + assert "banana" in message + get_settings.cache_clear() + + def test_bad_cidr_stops_startup(self, monkeypatch) -> None: + monkeypatch.setenv("TRUSTED_PROXY_CIDRS", "10.0.0.0/8,nawak") + get_settings.cache_clear() + with pytest.raises(ConfigurationError) as error: + get_settings() + assert "TRUSTED_PROXY_CIDRS" in str(error.value) + get_settings.cache_clear() diff --git a/backend/tests/test_link_privacy.py b/backend/tests/test_link_privacy.py index ecedcee..548d552 100644 --- a/backend/tests/test_link_privacy.py +++ b/backend/tests/test_link_privacy.py @@ -113,6 +113,48 @@ def test_management_token_never_appears_in_logs(client: TestClient, caplog) -> N assert "share=" in logged +def test_no_token_leaks_on_any_management_route(client: TestClient, caplog) -> None: + """PATCH, DELETE et verify manipulent aussi les deux jetons.""" + with caplog.at_level(logging.DEBUG): + created = client.post("/api/shares", json={"title": "x", "map": build_map()}).json() + token = created["share_token"] + management = created["management_token"] + headers = {"X-Management-Token": management} + + client.patch(f"/api/shares/{token}", json={"expires_in_days": 7}, headers=headers) + client.post(f"/api/shares/{token}/verify", headers=headers) + client.post(f"/api/shares/{token}/verify", headers={"X-Management-Token": "faux"}) + client.patch( + f"/api/shares/{token}", json={"permanent": True}, headers={"X-Management-Token": "faux"} + ) + client.delete(f"/api/shares/{token}", headers=headers) + + logged = "\n".join( + record.getMessage() + for record in caplog.records + if not record.name.startswith(("httpx", "httpcore")) + ) + assert management not in logged + assert token not in logged + assert "share=" in logged + + +def test_access_log_redaction_covers_every_share_route() -> None: + """Les chemins journalises par uvicorn ne doivent porter aucun jeton.""" + token = "sbcY5FwVInYtTSGLNjBCyw" + for method, path in [ + ("GET", f"/api/shares/{token}"), + ("PUT", f"/api/shares/{token}"), + ("PATCH", f"/api/shares/{token}"), + ("DELETE", f"/api/shares/{token}"), + ("POST", f"/api/shares/{token}/verify"), + ("GET", f"/s/{token}"), + ]: + line = redact_tokens(f"{method} {path} HTTP/1.1") + assert token not in line, line + assert "[REDACTED]" in line + + def test_application_logging_survives_migrations(client: TestClient, caplog) -> None: """Les migrations ne doivent pas desactiver les loggers de l'application. diff --git a/backend/tests/test_public_exposure.py b/backend/tests/test_public_exposure.py index 30d4bee..a7ae418 100644 --- a/backend/tests/test_public_exposure.py +++ b/backend/tests/test_public_exposure.py @@ -27,7 +27,8 @@ class TestRateRuleParsing: assert parse_rate("3").window_seconds == 60 def test_disabled_values(self) -> None: - for spec in ("", "0", "off", "none", "-4/hour", "n'importe quoi"): + """Seules ces valeurs desactivent : voir test_configuration.py pour le reste.""" + for spec in ("", "0", "off", "none", "disabled"): assert parse_rate(spec) is None @@ -155,3 +156,37 @@ def test_share_specific_size_limit(client: TestClient, configure) -> None: response = client.post("/api/shares", json={"title": "x", "map": build_map(node_count=50)}) assert response.status_code == 413 assert "limite de partage" in response.json()["detail"] + + +def test_capacity_is_not_crossed_by_simultaneous_creations(client: TestClient, configure) -> None: + """La derniere place ne doit pas etre prise deux fois. + + Le comptage et l'insertion sont serialises par un verrou de processus : + avec un seul service applicatif devant SQLite, cela suffit a ce que deux + requetes concurrentes ne franchissent pas ensemble la limite. + """ + import threading + + configure(max_shared_maps=5) + for _ in range(4): + assert _post(client) == 201 + + results: list[int] = [] + lock = threading.Lock() + + def attempt() -> None: + status = client.post("/api/shares", json={"title": "x", "map": build_map()}).status_code + with lock: + results.append(status) + + threads = [threading.Thread(target=attempt) for _ in range(8)] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + assert results.count(201) == 1 + assert results.count(503) == 7 + + listing = client.get("/api/config") + assert listing.status_code == 200 diff --git a/backend/tests/test_reverse_proxy.py b/backend/tests/test_reverse_proxy.py new file mode 100644 index 0000000..49d86a4 --- /dev/null +++ b/backend/tests/test_reverse_proxy.py @@ -0,0 +1,103 @@ +"""Limitation de debit derriere un reverse proxy, de bout en bout. + +Deux chemins sont reproduits sur l'application reelle : + +* une connexion venant d'un proxy approuve, qui transmet l'adresse du client ; +* une connexion directe qui forge le meme en-tete. + +Dans le premier cas la limite doit suivre l'adresse transmise, dans le second +l'adresse reelle de la socket. +""" + +from __future__ import annotations + +from collections.abc import Iterator + +import pytest +from fastapi.testclient import TestClient + +from app.main import app +from tests.conftest import build_map + +PROXY = "172.18.0.1" +DIRECT = "203.0.113.9" + + +def _client(peer: str) -> Iterator[TestClient]: + with TestClient(app, client=(peer, 40000)) as test_client: + yield test_client + + +@pytest.fixture() +def behind_proxy(configure) -> Iterator[TestClient]: + configure( + trust_proxy_headers=True, + trusted_proxy_cidrs="172.16.0.0/12,10.0.0.0/8", + share_create_rate_limit="2/hour", + ) + yield from _client(PROXY) + + +@pytest.fixture() +def direct_client(configure) -> Iterator[TestClient]: + configure( + trust_proxy_headers=True, + trusted_proxy_cidrs="172.16.0.0/12,10.0.0.0/8", + share_create_rate_limit="2/hour", + ) + yield from _client(DIRECT) + + +def _create(client: TestClient, forwarded_for: str | None) -> int: + headers = {"X-Forwarded-For": forwarded_for} if forwarded_for else {} + return client.post( + "/api/shares", json={"title": "x", "map": build_map()}, headers=headers + ).status_code + + +def test_limit_follows_the_address_transmitted_by_an_approved_proxy( + behind_proxy: TestClient, +) -> None: + assert _create(behind_proxy, "198.51.100.10") == 201 + assert _create(behind_proxy, "198.51.100.10") == 201 + assert _create(behind_proxy, "198.51.100.10") == 429 + + # Un autre client, derriere le meme proxy, garde son propre compteur. + assert _create(behind_proxy, "198.51.100.11") == 201 + + +def test_approved_proxy_ignores_the_part_forged_by_the_client( + behind_proxy: TestClient, +) -> None: + """Le client ecrit "9.9.9.9" ; le proxy ajoute l'adresse qu'il a vue.""" + assert _create(behind_proxy, "9.9.9.9, 198.51.100.20") == 201 + assert _create(behind_proxy, "9.9.9.9, 198.51.100.20") == 201 + assert _create(behind_proxy, "9.9.9.9, 198.51.100.20") == 429 + + # Changer la partie forgee ne remet pas le compteur a zero. + assert _create(behind_proxy, "1.1.1.1, 198.51.100.20") == 429 + + +def test_direct_client_cannot_escape_the_limit_by_forging_the_header( + direct_client: TestClient, +) -> None: + assert _create(direct_client, "10.0.0.1") == 201 + assert _create(direct_client, "10.0.0.2") == 201 + # Adresse forgee differente a chaque fois : la socket, elle, ne change pas. + assert _create(direct_client, "10.0.0.3") == 429 + assert _create(direct_client, "10.0.0.4") == 429 + assert _create(direct_client, None) == 429 + + +def test_ipv6_rewriting_does_not_reset_the_counter(behind_proxy: TestClient) -> None: + assert _create(behind_proxy, "2001:db8::1") == 201 + assert _create(behind_proxy, "2001:0db8:0000:0000:0000:0000:0000:0001") == 201 + assert _create(behind_proxy, "2001:DB8:0:0:0:0:0:1") == 429 + + +def test_headers_are_ignored_when_trust_is_disabled(configure) -> None: + configure(trust_proxy_headers=False, share_create_rate_limit="2/hour") + with TestClient(app, client=(PROXY, 40000)) as client: + assert _create(client, "198.51.100.30") == 201 + assert _create(client, "198.51.100.31") == 201 + assert _create(client, "198.51.100.32") == 429 diff --git a/docker-compose.yml b/docker-compose.yml index 14759ee..674e076 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -23,11 +23,16 @@ services: DEFAULT_SHARE_EXPIRY_DAYS: ${DEFAULT_SHARE_EXPIRY_DAYS:-30} ALLOW_PERMANENT_SHARES: ${ALLOW_PERMANENT_SHARES:-true} TRUST_PROXY_HEADERS: ${TRUST_PROXY_HEADERS:-false} + TRUSTED_PROXY_CIDRS: ${TRUSTED_PROXY_CIDRS:-} CORS_ORIGINS: ${CORS_ORIGINS:-} LOG_LEVEL: ${LOG_LEVEL:-INFO} STATIC_DIR: /app/static + # En developpement, le port est publie sur toutes les interfaces. + # En production derriere un reverse proxy, restreignez-le a la boucle locale + # en posant APP_BIND=127.0.0.1 dans .env : le service n'est alors joignable + # que par le proxy, jamais directement depuis l'exterieur. ports: - - "${APP_PORT:-8080}:8080" + - "${APP_BIND:-0.0.0.0}:${APP_PORT:-8080}:8080" volumes: # Bind mount explicite : les partages SQLite restent accessibles sur l'hote. - ./data:/app/data diff --git a/docker-entrypoint.sh b/docker-entrypoint.sh index 77b86e6..cfd1eb5 100644 --- a/docker-entrypoint.sh +++ b/docker-entrypoint.sh @@ -7,6 +7,9 @@ set -e echo "Migrations de la base..." -python -m app.db.migrations +if ! python -m app.db.migrations; then + echo "Demarrage interrompu : corrigez la configuration puis relancez." >&2 + exit 1 +fi exec "$@"