From 1174257adc8e5bb1526fe17a0d5e842967f20674 Mon Sep 17 00:00:00 2001 From: Arthur Lorin Date: Fri, 21 Aug 2026 17:56:52 +0200 Subject: [PATCH 01/28] feat(config): make the displayed product name configurable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add `config.site_name` (`PYPLET_SITE_NAME`, default `"Pyplet"`) and read it at call time for every user-facing product-name literal: - `templates.py`: ``, navbar wordmark, `About …` nav link, the login `Sign in to …` heading, and both `Welcome to …!` index branches; - `magiclink.py` (`_send_email_sync`): subject, plain body, HTML body and button label. This lets a deployment run under its own name by environment alone, with no patch to the core. Out of scope on purpose: the `/about` product copy, the copyright line and the links. Verified: with the default, the rendered output is byte-for-byte identical to the parent commit on the 9 affected surfaces (both `base_template` title branches, both navbar branches, both index branches, login, about, and the magic-link e-mail) — hence no existing test changes. With `PYPLET_SITE_NAME=Z2Z`, the name propagates to all of them (25 occurrences) and the only remaining `Pyplet` string is the out-of-scope `/about` copy. --- pyplet/server/config.py | 10 ++++++++++ pyplet/server/magiclink.py | 9 +++++---- pyplet/server/templates.py | 18 ++++++++++++------ 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/pyplet/server/config.py b/pyplet/server/config.py index bb8b7da..92159dd 100644 --- a/pyplet/server/config.py +++ b/pyplet/server/config.py @@ -105,6 +105,16 @@ class PypletConfig: env_var="PYPLET_FAVICON", ) + site_name = Param( + default="Pyplet", + description=( + "Product name shown to users: page titles, navbar wordmark, " + "sign-in page and magic-link e-mails. Set this to deploy " + "under your own name without patching the code." + ), + env_var="PYPLET_SITE_NAME", + ) + # ── Authentication ─────────────────────────────────────────────────────── oauth_cookie_secret = Param( default=None, diff --git a/pyplet/server/magiclink.py b/pyplet/server/magiclink.py index 78569ec..4fb96c0 100644 --- a/pyplet/server/magiclink.py +++ b/pyplet/server/magiclink.py @@ -129,14 +129,15 @@ def _consume_token(token: str) -> dict | None: def _send_email_sync(to_addr: str, base: str, magic_url: str) -> None: """Send the magic-link e-mail synchronously (called from a thread).""" ttl_min = config.magiclink_token_ttl // 60 + site_name = config.site_name msg = email.mime.multipart.MIMEMultipart("alternative") - msg["Subject"] = "Your Pyplet sign-in link" + msg["Subject"] = f"Your {site_name} sign-in link" msg["From"] = base msg["To"] = to_addr plain = ( - f"Click the link below to sign in to Pyplet.\n\n" + f"Click the link below to sign in to {site_name}.\n\n" f"{magic_url}\n\n" f"This link expires in {ttl_min} minute(s) and " "can only be used once.\nIf you did not request " @@ -144,12 +145,12 @@ def _send_email_sync(to_addr: str, base: str, magic_url: str) -> None: ) html = f"""\ <html><body> -<p>Click the button below to sign in to Pyplet.</p> +<p>Click the button below to sign in to {site_name}.</p> <p style="margin:24px 0"> <a href="{magic_url}" style="background:#0d6efd;color:#fff;padding:12px 24px; border-radius:6px;text-decoration:none;font-size:16px"> - Sign in to Pyplet + Sign in to {site_name} </a> </p> <p style="color:#6c757d;font-size:13px"> diff --git a/pyplet/server/templates.py b/pyplet/server/templates.py index d205b3d..41f1418 100644 --- a/pyplet/server/templates.py +++ b/pyplet/server/templates.py @@ -72,7 +72,11 @@ def base_template( meta( name="viewport", content="width=device-width, initial-scale=1" ), - title[f"{page_title} - Pyplet" if page_title else "Pyplet"], + title[ + f"{page_title} - {config.site_name}" + if page_title + else config.site_name + ], link( href="https://cdn.jsdelivr.net/npm/bootstrap@5.3.3/dist/css/bootstrap.min.css", # noqa: E501 rel="stylesheet", @@ -144,12 +148,14 @@ def default_navbar( else: # Not authenticated or auth disabled: show "About" link right_items_content = [ - li(".nav-item")[a(".nav-link", href="/about")["About Pyplet"]] + li(".nav-item")[ + a(".nav-link", href="/about")[f"About {config.site_name}"] + ] ] return nav(".navbar.navbar-expand-sm.bg-body-tertiary")[ div(".container")[ - a(".navbar-brand", href="/")["Pyplet"], + a(".navbar-brand", href="/")[config.site_name], ul(".navbar-nav.ms-auto")[*right_items_content], ] ] @@ -211,7 +217,7 @@ def login_template(handler: RequestHandler) -> Node: providers = oauth.enabled_providers() show_magiclink = magiclink.enabled() - card_children = [h4(".mb-4.text-center")["Sign in to Pyplet"]] + card_children = [h4(".mb-4.text-center")[f"Sign in to {config.site_name}"]] # ── OAuth provider buttons ────────────────────────────────────────── for provider in providers: @@ -338,13 +344,13 @@ def index_template( *[li[project, ul[*apps]] for project, apps in projects.items()] ] body_content = [ - p["Welcome to Pyplet!"], + p[f"Welcome to {config.site_name}!"], p["The following applications are available:"], application_list, ] else: body_content = [ - p["Welcome to Pyplet!"], + p[f"Welcome to {config.site_name}!"], p[ ( em["No applications are available for your account."] From 878ee61120f24170f5dd11105b250387eb14316d Mon Sep 17 00:00:00 2001 From: Arthur Lorin <arthur.lorin@cetic.be> Date: Fri, 21 Aug 2026 17:58:48 +0200 Subject: [PATCH 02/28] refactor(server): load app modules before building the Tornado app `astart` built `tornado.web.Application(**_app_spec)` and started listening before importing the `*_server.py` modules. Since importing a module is what fires `ServerApplication.__init_subclass__` and registers the instance in `server_applications`, the registry was still empty when the app was built, so nothing derived from it could reach `_app_spec`. Pure reordering: the loader (`_load_server_module`) is unchanged, and the favicon block still runs immediately before `Application(**_app_spec)`. No behaviour change for the current handler set. This is the base the startup policies and the app-declared route merge of the following changes are wired into. --- pyplet/server/_server.py | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/pyplet/server/_server.py b/pyplet/server/_server.py index e2e7aa1..b373b28 100644 --- a/pyplet/server/_server.py +++ b/pyplet/server/_server.py @@ -482,6 +482,19 @@ def _load_server_module(path: str) -> str: async def astart(): + # Load all server applications FIRST: importing each *_server.py + # fires ServerApplication.__init_subclass__, which registers the + # instance in server_applications. Anything derived from that + # registry has to run once it is populated, so the modules are + # loaded before the Tornado Application is built from _app_spec. + server_modules = glob.glob(f"{config.apps}/*/*_server.py") + for path in server_modules: + try: + module_name = _load_server_module(path) + logger.debug(f"Loaded module: {module_name}") + except Exception as e: + logger.error(f"Failed to load module {path}: {e}", exc_info=True) + favicon_uri = None if config.favicon: # Relative paths (e.g. the default "../images/...") are resolved @@ -506,15 +519,6 @@ async def astart(): app = tornado.web.Application(**_app_spec) app.listen(config.port, config.address) - # Load all server applications - server_modules = glob.glob(f"{config.apps}/*/*_server.py") - for path in server_modules: - try: - module_name = _load_server_module(path) - logger.debug(f"Loaded module: {module_name}") - except Exception as e: - logger.error(f"Failed to load module {path}: {e}", exc_info=True) - url = config.url or f"http://{config.address}:{config.port}" logger.info(f"Pyplet server started on {url}") logger.info(f"Loaded {len(server_applications)} application(s)") From 6f9855b3f13b32ac294977a08c6e37a390d76415 Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Sun, 31 May 2026 05:21:53 +0200 Subject: [PATCH 03/28] =?UTF-8?q?feat(auth):=20PYPLET=5FSESSION=5FTTL=5FDA?= =?UTF-8?q?YS=20=E2=80=94=20promote=20session=20TTL=20to=20config=20Param?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The session cookie lifetime was the module-level constant `_SESSION_MAX_AGE_DAYS = 1` in `oauth.py`, used for both `expires_days` (set) and `max_age_days` (read). It becomes `config.session_max_age_days` (`PYPLET_SESSION_TTL_DAYS`, `type_cast=int`), so a deployment can extend the session without patching the code. Default unchanged: 1 day. Originally landed on the app branch under the name `CHANI_SESSION_TTL_DAYS`; renamed here — the core carries no deployment-specific name. (Story 14.6) --- pyplet/server/config.py | 9 +++++++++ pyplet/server/oauth.py | 5 ++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/pyplet/server/config.py b/pyplet/server/config.py index 92159dd..26f0b9c 100644 --- a/pyplet/server/config.py +++ b/pyplet/server/config.py @@ -124,6 +124,15 @@ class PypletConfig: ), env_var="PYPLET_COOKIE_SECRET", ) + session_max_age_days = Param( + default=1, + description=( + "Session cookie lifetime in days (default: 1 = 24 h). Set " + "PYPLET_SESSION_TTL_DAYS to extend." + ), + type_cast=int, + env_var="PYPLET_SESSION_TTL_DAYS", + ) oauth_google_client_id = Param( default=None, description="Google OAuth2 client ID.", diff --git a/pyplet/server/oauth.py b/pyplet/server/oauth.py index 57e78c3..b8ec986 100644 --- a/pyplet/server/oauth.py +++ b/pyplet/server/oauth.py @@ -174,7 +174,6 @@ def _decode_id_token_claims(id_token: str) -> dict: # --------------------------------------------------------------------------- SESSION_COOKIE = "pyplet_user" -_SESSION_MAX_AGE_DAYS = 1 # 24 hours def set_session(handler, user_info: dict) -> None: @@ -183,7 +182,7 @@ def set_session(handler, user_info: dict) -> None: handler.set_signed_cookie( SESSION_COOKIE, payload, - expires_days=_SESSION_MAX_AGE_DAYS, + expires_days=config.session_max_age_days, httponly=True, samesite="Lax", ) @@ -192,7 +191,7 @@ def set_session(handler, user_info: dict) -> None: def get_session(handler) -> dict | None: """Return the user dict from the signed cookie, or ``None``.""" raw = handler.get_signed_cookie( - SESSION_COOKIE, max_age_days=_SESSION_MAX_AGE_DAYS + SESSION_COOKIE, max_age_days=config.session_max_age_days ) if raw is None: return None From 2b06e898e2bef7ce9811fec78f5b0c49014a3a68 Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Sun, 31 May 2026 07:12:38 +0200 Subject: [PATCH 04/28] feat(auth): Drive incremental consent + server-side token hooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an incremental Google OAuth authorization flow that requests the `drive.file` scope on top of an existing login session: - `start_drive_consent()` — redirects with `access_type=offline` and `prompt=consent` so Google returns a refresh token, and marks the state cookie with `"flow": "drive"`; - `handle_callback()` — routes a `"flow": "drive"` callback to the token hooks instead of `set_session()`, leaving the login session untouched; - `register_drive_token_hook()` — lets a server application register an async callback receiving `(sub, email, refresh_token, scopes)`. The core only carries the flow and the hook registry: storing or encrypting the refresh token is the application's business. Landed without the `cryptography` and `google-auth` dependencies the original commit added: nothing in `pyplet/` or `tests/` imports them, and the JWKS verification runs on `authlib`, already a dependency. (Story 14.8) --- pyplet/server/oauth.py | 88 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/pyplet/server/oauth.py b/pyplet/server/oauth.py index b8ec986..f85f13b 100644 --- a/pyplet/server/oauth.py +++ b/pyplet/server/oauth.py @@ -116,6 +116,22 @@ def register_auth_check(fn) -> None: _extra_auth_checks.append(fn) +# Callbacks registered by a server application to handle Drive-consent +# tokens +# (Story 14.8 / CAP-8). Called after a drive-flow OAuth callback completes. +_drive_token_hooks: list = [] + + +def register_drive_token_hook(fn) -> None: + """Register an async callback called after a drive-consent OAuth callback. + + Called with (sub: str, email: str, refresh_token: str | None, scopes: str). + Registered hooks are called in order once the Drive token exchange + succeeds. + """ + _drive_token_hooks.append(fn) + + def auth_enabled() -> bool: """True when at least one authentication method (OAuth or magic-link) is configured.""" @@ -354,6 +370,59 @@ async def start_login(handler, provider: str) -> None: handler.redirect(auth_url) +async def start_drive_consent(handler, next_url: str = "/") -> None: + """Initiate an incremental OAuth authorization to add drive.file scope. + + Uses the same unified Web-application client as login (Q3). Adds + access_type=offline and prompt=consent to force a refresh_token response. + Stores "flow": "drive" in the state cookie so handle_callback routes + the response to the Drive token hook instead of set_session. + + Does not modify the login session (user stays logged in throughout). + + Args: + handler: Tornado RequestHandler. + next_url: URL to redirect to after Drive consent completes. + + Returns: + None (redirects the browser). + """ + meta = _PROVIDER_CONFIGS["google"] + oidc = await _fetch_oidc_config("google") + client_id = meta["client_id"]() + state = secrets.token_urlsafe(16) + + handler.set_signed_cookie( + _STATE_COOKIE, + json.dumps( + { + "state": state, + "next": next_url, + "provider": "google", + "flow": "drive", + } + ), + httponly=True, + samesite="Lax", + ) + + callback_url = _callback_url(handler) + params = { + "client_id": client_id, + "redirect_uri": callback_url, + "response_type": "code", + "scope": " ".join( + meta["scopes"] + ["https://www.googleapis.com/auth/drive.file"] + ), + "state": state, + "access_type": "offline", + "prompt": "consent", + "include_granted_scopes": "true", + } + auth_url = oidc["authorization_endpoint"] + "?" + urlencode(params) + handler.redirect(auth_url) + + async def handle_callback(handler) -> None: """ Complete the OAuth authorization-code flow. @@ -443,6 +512,25 @@ async def handle_callback(handler) -> None: _error(handler, 500, "Provider did not include an email in the token.") return + # Story 14.8 / CAP-8: Drive incremental consent callback — do NOT call + # set_session (the user is already logged in). Call drive token hooks + # to store the refresh token, then redirect back to the app. + if state_data.get("flow") == "drive": + refresh_token = tokens.get( + "refresh_token" + ) # None if Google omitted it + scopes = tokens.get("scope", "") + for hook in _drive_token_hooks: + try: + await hook( + user_info["sub"], user_info["email"], refresh_token, scopes + ) + except Exception as exc: + logger.error("Drive token hook failed: %s", exc) + handler.clear_cookie(_STATE_COOKIE) + handler.redirect(next_url) + return + set_session(handler, user_info) logger.info( "Login: %s (%s) via %s", From b31cb2bbd2616fff0dabb3ab50e1340e824bc320 Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Wed, 3 Jun 2026 23:27:21 +0200 Subject: [PATCH 05/28] Test: declare pytest-asyncio in the test extra chani's suite uses asyncio_mode=auto + @pytest.mark.asyncio but pytest-asyncio was undeclared, so a venv resync dropped it and 32 async tests errored under --strict-markers. Declaring it keeps it across syncs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --- pyproject.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/pyproject.toml b/pyproject.toml index ec845c8..5ed489b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -30,6 +30,7 @@ license-files = ["LICENSE"] test = [ "pytest>=8.0.0", "pytest-cov>=4.1.0", + "pytest-asyncio>=0.23.0", "selenium>=4.16.0", "pytest-xdist>=3.8.0", ] From 50fbfc1736118750ae4597c62e31dccbfa536d74 Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Mon, 8 Jun 2026 18:05:41 +0200 Subject: [PATCH 06/28] feat(auth): fail-closed startup policy and deny-by-default ACL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three ways a misdelivered auth config used to end in a server that served everything anonymously. On the production profile (`PYPLET_REQUIRE_AUTH=1`) each one now refuses the boot instead: - no auth method configured at all; - `auth_rules.json` missing; - magic-link enabled without the explicit `PYPLET_ALLOW_MAGICLINK=1` opt-in (magic-link mints a session for any e-mail that can receive the link). `is_app_permitted()` also flips to deny-by-default: with authentication enabled and no rules file, no app is visible. Authentication fully disabled (local dev, no provider) keeps allow-all, so an anonymous local run is unaffected; without `PYPLET_REQUIRE_AUTH` the server still starts and logs a WARNING that requests are served anonymously. `enforce_startup_auth_policy()` is called from `astart` once the app modules are loaded, so the policy sees every discovered application, and before the Tornado `Application` is built. +8 tests (`tests/oauth_test.py`). `apps/auth_rules.json` stays the framework example — this change does not ship any deployment's rules. (Story 17.6) --- README.md | 27 +++++- pyplet/server/_server.py | 6 ++ pyplet/server/config.py | 18 ++++ pyplet/server/oauth.py | 109 +++++++++++++++++++++-- tests/oauth_test.py | 187 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 337 insertions(+), 10 deletions(-) create mode 100644 tests/oauth_test.py diff --git a/README.md b/README.md index 1ec38ba..8f77c58 100644 --- a/README.md +++ b/README.md @@ -193,9 +193,8 @@ export OAUTH_MICROSOFT_TENANT=common # or your tenant ID ### Access control (ACL) -By default every authenticated user can see all apps. To restrict access, -create `apps/auth_rules.json` — a JSON array of -`["project/app regex", "email regex"]` pairs: +To restrict which apps each user can see, create `apps/auth_rules.json` — a +JSON array of `["project/app regex", "email regex"]` pairs: ```json [ @@ -211,6 +210,12 @@ If no rule matches, access is denied. Override the rules file path with `PYPLET_AUTH_RULES_FILE`. +**Deny-by-default (fail closed):** when authentication is enabled but the rules +file is **missing**, access is **denied** to every app — ship `auth_rules.json` +in your deploy artifact. (When auth is fully disabled — local dev with no +provider — a missing file still allows all apps, so an un-authenticated local +run works.) + ### Magic-link e-mail authentication As an alternative (or complement) to OAuth, users can sign in by entering their @@ -237,11 +242,27 @@ The ACL rules file applies to magic-link logins exactly the same way it does for OAuth: the user's e-mail address is matched against the `email_regex` column of each rule. +Because magic-link mints a session for **any** e-mail that can receive the +link, it is **refused at boot on the production profile** (`PYPLET_REQUIRE_AUTH=1`, +below) unless you opt in explicitly with `PYPLET_ALLOW_MAGICLINK=1`. + +### Production fail-closed startup (`PYPLET_REQUIRE_AUTH`) + +On any non-local deployment, set `PYPLET_REQUIRE_AUTH=1`. With it, the server +**refuses to boot** (exits non-zero with a logged error) rather than silently +serving anonymously when the auth config is misdelivered — specifically when +**no** auth method is configured, when `auth_rules.json` is **missing**, or +when magic-link is enabled **without** `PYPLET_ALLOW_MAGICLINK=1`. Without the +flag (the default), a deployment with no provider still starts but logs a loud +WARNING that every request is served anonymously. + ### Configuration reference | Variable | Description | | --- | --- | | `PYPLET_COOKIE_SECRET` | Secret for signing session cookies | +| `PYPLET_REQUIRE_AUTH` | Fail-closed switch: `1` refuses boot, default `0` | +| `PYPLET_ALLOW_MAGICLINK` | Opt magic-link IN on require-auth, default `0` | | **OAuth — Google** | | | `OAUTH_GOOGLE_CLIENT_ID` | Google OAuth2 client ID | | `OAUTH_GOOGLE_CLIENT_SECRET` | Google OAuth2 client secret | diff --git a/pyplet/server/_server.py b/pyplet/server/_server.py index b373b28..0262488 100644 --- a/pyplet/server/_server.py +++ b/pyplet/server/_server.py @@ -495,6 +495,12 @@ async def astart(): except Exception as e: logger.error(f"Failed to load module {path}: {e}", exc_info=True) + # Fail-closed auth policy (Story 17.6, PB-1): on the production profile, + # refuse to boot on a misdelivered auth config rather than serve + # anonymously. Runs once the modules are loaded, so the policy sees + # every discovered application. + oauth.enforce_startup_auth_policy(magiclink_enabled=magiclink.enabled()) + favicon_uri = None if config.favicon: # Relative paths (e.g. the default "../images/...") are resolved diff --git a/pyplet/server/config.py b/pyplet/server/config.py index 26f0b9c..aab541b 100644 --- a/pyplet/server/config.py +++ b/pyplet/server/config.py @@ -158,6 +158,24 @@ class PypletConfig: description='Tenant ID or "common" for multi-tenant apps.', env_var="OAUTH_MICROSOFT_TENANT", ) + require_auth = Param( + default="0", + description=( + "Production fail-CLOSED switch. Set to '1' to refuse boot when no " + "auth method is configured, when auth_rules.json is missing, or " + "when magic-link is enabled without PYPLET_ALLOW_MAGICLINK." + ), + env_var="PYPLET_REQUIRE_AUTH", + ) + allow_magiclink = Param( + default="0", + description=( + "Set to '1' to opt magic-link login IN on a production " + "(PYPLET_REQUIRE_AUTH) profile; otherwise a configured " + "MAGICLINK_SMTP_* refuses to boot." + ), + env_var="PYPLET_ALLOW_MAGICLINK", + ) # ── Magic-link e-mail auth ─────────────────────────────────────────────── magiclink_smtp_host = Param( diff --git a/pyplet/server/oauth.py b/pyplet/server/oauth.py index f85f13b..0f90a8d 100644 --- a/pyplet/server/oauth.py +++ b/pyplet/server/oauth.py @@ -41,9 +41,16 @@ [[".*", "@mycompany\\.com$"], ["public/demo", ".*"]] -If the file does not exist, **any authenticated user** can see **all** apps. - -Auth is silently disabled when no OAuth provider env vars are set. +Deny-by-default (Story 17.6, PB-1): when auth is **enabled** but the rules +file is missing, ACL **denies** all app access (fail closed) and logs an +ERROR. When auth is fully disabled (no provider — local dev), a missing file +still allows all apps so an un-authenticated local run works. + +Fail-closed startup (``PYPLET_REQUIRE_AUTH=1``): on this production profile +``enforce_startup_auth_policy()`` refuses to boot when no auth method is +configured, when ``auth_rules.json`` is missing, or when magic-link is enabled +without ``PYPLET_ALLOW_MAGICLINK=1``. Without the flag, auth is silently +disabled when no OAuth provider env vars are set (a loud WARNING is logged). """ from __future__ import annotations @@ -231,7 +238,7 @@ def clear_session(handler) -> None: # app_pattern is matched against the combined "project/app" string. _AclRule = tuple[re.Pattern, re.Pattern] _acl_rules: list[_AclRule] | None = None # None = "not loaded yet" -_acl_allow_all: bool = False # True when no rules file exists +_acl_allow_all: bool = False # True only when no rules file AND auth disabled def _load_acl_rules() -> None: @@ -240,9 +247,22 @@ def _load_acl_rules() -> None: rules_path = config.auth_rules_file if not os.path.isfile(rules_path): + if auth_enabled(): + # Fail CLOSED: auth is on but no rules file → deny all by default. + logger.error( + "No ACL rules file at %s while auth is enabled — denying all " + "app access by default (deny-by-default). Ship " + "auth_rules.json.", + rules_path, + ) + _acl_rules = [] + _acl_allow_all = False + return + # Auth fully disabled (local dev, no provider) — allow all so an + # un-authenticated local run still works. logger.info( - "No ACL rules file found at %s — all authenticated users" - " can access all apps.", + "No ACL rules file at %s and auth disabled — allowing all apps " + "(local dev).", rules_path, ) _acl_rules = [] @@ -291,7 +311,9 @@ def is_app_permitted(project: str, app: str, email: str) -> bool: Each rule's first regex is matched against the combined ``"project/app"`` string; the second is matched against the user's email address. Rules are evaluated in order; the first full match grants access. - If no rules file exists, access is always granted. + When auth is enabled but no rules file exists, access is denied + (deny-by-default, Story 17.6); the allow-all-on-missing-file path + survives only when auth is fully disabled (local dev). """ _ensure_acl_loaded() @@ -327,6 +349,79 @@ def permitted_apps(email: str) -> list[tuple[str, str]]: return result +# --------------------------------------------------------------------------- +# Fail-closed startup policy (Story 17.6, PB-1) +# --------------------------------------------------------------------------- + + +class AuthConfigError(RuntimeError): + """Raised at startup when a fail-closed auth policy is violated. + + Refusing to boot is intentional: a misdelivered auth config must + hard-fail rather than silently serve every app anonymously (PB-1). + """ + + +def enforce_startup_auth_policy(magiclink_enabled: bool = False) -> None: + """Fail-closed startup checks for the production profile (PB-1, + Story 17.6). + + On the production profile (``PYPLET_REQUIRE_AUTH=1``) raises + ``AuthConfigError`` — refusing to boot — when (1) no auth method is + configured, (2) auth is enabled but ``auth_rules.json`` is missing, or + (3) magic-link is configured without ``PYPLET_ALLOW_MAGICLINK=1``. Off the + production profile it logs a loud WARNING/ERROR instead of raising, so an + explicitly-open local dev run still starts. + + Args: + magiclink_enabled: whether magic-link auth is active. Passed in by the + caller (``_server.astart`` → ``magiclink.enabled()``) to avoid an + ``oauth``↔``magiclink`` circular import. + + Side effects: emits log records; reads ``config`` + env. + Raises: ``AuthConfigError`` to abort startup on a production-profile + breach. + """ + production = config.require_auth == "1" + + if not auth_enabled(): + if production: + raise AuthConfigError( + "PYPLET_REQUIRE_AUTH=1 but no authentication method is " + "configured — refusing to boot (would serve every app " + "anonymously). Configure a provider (e.g. " + "OAUTH_GOOGLE_CLIENT_ID/SECRET), or unset PYPLET_REQUIRE_AUTH " + "for an explicitly open deployment." + ) + logger.warning( + "Authentication is DISABLED (no provider configured) — every " + "request is served anonymously. Set PYPLET_REQUIRE_AUTH=1 with a " + "configured provider on any non-local deployment." + ) + return + + if not os.path.isfile(config.auth_rules_file): + msg = ( + "auth_rules.json not found at %s while authentication is enabled " + "— ACL DENIES all app access by default (fail closed)." + % config.auth_rules_file + ) + if production: + raise AuthConfigError( + msg + " Refusing to boot under PYPLET_REQUIRE_AUTH; ship " + "auth_rules.json in the deploy artifact." + ) + logger.error(msg) + + if production and magiclink_enabled and config.allow_magiclink != "1": + raise AuthConfigError( + "Magic-link (MAGICLINK_SMTP_*) is configured on the production " + "profile (PYPLET_REQUIRE_AUTH=1) — refusing to boot. Magic-link " + "mints a session for ANY email and bypasses the OAuth/ACL " + "boundary. Set PYPLET_ALLOW_MAGICLINK=1 to opt in explicitly." + ) + + # --------------------------------------------------------------------------- # OAuth login flow helpers (called by handlers in _server.py) # --------------------------------------------------------------------------- diff --git a/tests/oauth_test.py b/tests/oauth_test.py new file mode 100644 index 0000000..353b7f5 --- /dev/null +++ b/tests/oauth_test.py @@ -0,0 +1,187 @@ +"""Fail-closed auth policy tests for ``pyplet.server.oauth`` (Story 17.6, +PB-1). + +Covers SECURI-2 / SECURI-1 / SECURI-7: the ``PYPLET_REQUIRE_AUTH`` production +startup assertion (``enforce_startup_auth_policy``), the deny-by-default ACL +when auth is enabled but ``auth_rules.json`` is missing, and the magic-link +refuse-in-production gate (``PYPLET_ALLOW_MAGICLINK`` opt-in). + +These are sync ``def test_*`` functions using the ``monkeypatch`` fixture, +mirroring ``tests/config_test.py``. The helper under test is sync, so no +``@pytest.mark.asyncio`` / pytest-asyncio is required. + +ACL global-state hygiene: ``oauth._acl_rules`` / ``oauth._acl_allow_all`` are +module globals cached across calls; the autouse fixture resets them before and +after every test so loaded rules never leak between tests. +""" + +import json +import logging + +import pytest + +from pyplet.server import oauth + +# Provider / magic-link env vars that drive auth_enabled(). +_AUTH_ENV_VARS = ( + "OAUTH_GOOGLE_CLIENT_ID", + "OAUTH_GOOGLE_CLIENT_SECRET", + "OAUTH_MICROSOFT_CLIENT_ID", + "OAUTH_MICROSOFT_CLIENT_SECRET", + "MAGICLINK_SMTP_HOST", + "MAGICLINK_SMTP_USER", + "MAGICLINK_SMTP_PASSWORD", +) + +# Production-profile policy flags introduced by Story 17.6. +_POLICY_ENV_VARS = ("PYPLET_REQUIRE_AUTH", "PYPLET_ALLOW_MAGICLINK") + + +@pytest.fixture(autouse=True) +def _reset_acl_state(): + """Reset oauth's cached ACL module globals before and after each test.""" + oauth._acl_rules = None + oauth._acl_allow_all = False + yield + oauth._acl_rules = None + oauth._acl_allow_all = False + + +def _clear_auth_env(monkeypatch): + """Remove every provider / magic-link / policy env var (raising=False).""" + for var in _AUTH_ENV_VARS + _POLICY_ENV_VARS: + monkeypatch.delenv(var, raising=False) + + +def _write_rules(tmp_path): + """Write a real (well-formed) auth_rules.json and return its path.""" + rules = tmp_path / "auth_rules.json" + rules.write_text(json.dumps([[".*", "@example\\.com$"]])) + return rules + + +# --------------------------------------------------------------------------- +# AC4(a) — PYPLET_REQUIRE_AUTH production startup assertion (SECURI-2) +# --------------------------------------------------------------------------- + + +def test_enforce_raises_when_production_and_no_auth_method(monkeypatch): + """Production profile + no auth method configured → refuse to boot.""" + _clear_auth_env(monkeypatch) + monkeypatch.setenv("PYPLET_REQUIRE_AUTH", "1") + assert oauth.auth_enabled() is False + with pytest.raises(oauth.AuthConfigError): + oauth.enforce_startup_auth_policy(magiclink_enabled=False) + + +def test_enforce_warns_when_auth_off_and_not_production(monkeypatch, caplog): + """Auth off and not production → returns None (warns, does not raise).""" + _clear_auth_env(monkeypatch) + with caplog.at_level(logging.WARNING, logger="pyplet.server.oauth"): + result = oauth.enforce_startup_auth_policy(magiclink_enabled=False) + assert result is None + assert any(rec.levelno == logging.WARNING for rec in caplog.records) + + +# --------------------------------------------------------------------------- +# AC4(b) — deny-by-default ACL when auth on and rules file missing (SECURI-1) +# --------------------------------------------------------------------------- + + +def test_acl_denies_by_default_when_auth_on_and_no_rules_file( + monkeypatch, tmp_path +): + """Auth enabled but no rules file → is_app_permitted denies (fail + closed).""" + _clear_auth_env(monkeypatch) + monkeypatch.setenv("OAUTH_GOOGLE_CLIENT_ID", "x") + monkeypatch.setenv("OAUTH_GOOGLE_CLIENT_SECRET", "y") + monkeypatch.setenv("PYPLET_AUTH_RULES_FILE", str(tmp_path / "nope.json")) + oauth.reload_acl() + assert oauth.is_app_permitted("any", "app", "user@example.com") is False + + +def test_acl_allows_all_when_auth_disabled_and_no_rules_file( + monkeypatch, tmp_path +): + """Auth fully disabled (local dev) preserves allow-all on a missing + file.""" + _clear_auth_env(monkeypatch) + monkeypatch.setenv("PYPLET_AUTH_RULES_FILE", str(tmp_path / "nope.json")) + oauth.reload_acl() + assert oauth.is_app_permitted("any", "app", "user@example.com") is True + + +# --------------------------------------------------------------------------- +# AC4(c) — production + auth on + rules file missing → refuse to boot +# --------------------------------------------------------------------------- + + +def test_enforce_raises_when_production_auth_on_and_no_rules_file( + monkeypatch, tmp_path +): + """Production profile + auth on + missing rules file → refuse to boot.""" + _clear_auth_env(monkeypatch) + monkeypatch.setenv("PYPLET_REQUIRE_AUTH", "1") + monkeypatch.setenv("OAUTH_GOOGLE_CLIENT_ID", "x") + monkeypatch.setenv("OAUTH_GOOGLE_CLIENT_SECRET", "y") + monkeypatch.setenv("PYPLET_AUTH_RULES_FILE", str(tmp_path / "nope.json")) + with pytest.raises(oauth.AuthConfigError): + oauth.enforce_startup_auth_policy(magiclink_enabled=False) + + +# --------------------------------------------------------------------------- +# AC4(d) — magic-link refuse-in-production unless PYPLET_ALLOW_MAGICLINK +# (SECURI-7) +# --------------------------------------------------------------------------- + + +def test_enforce_raises_when_production_magiclink_without_optin( + monkeypatch, tmp_path +): + """Production profile + magic-link on + no opt-in → refuse to boot.""" + _clear_auth_env(monkeypatch) + rules = _write_rules(tmp_path) + monkeypatch.setenv("PYPLET_REQUIRE_AUTH", "1") + monkeypatch.setenv("OAUTH_GOOGLE_CLIENT_ID", "x") + monkeypatch.setenv("OAUTH_GOOGLE_CLIENT_SECRET", "y") + monkeypatch.setenv("PYPLET_AUTH_RULES_FILE", str(rules)) + with pytest.raises(oauth.AuthConfigError): + oauth.enforce_startup_auth_policy(magiclink_enabled=True) + + +def test_enforce_allows_magiclink_with_explicit_optin(monkeypatch, tmp_path): + """Production profile + magic-link on + PYPLET_ALLOW_MAGICLINK=1 → + boots.""" + _clear_auth_env(monkeypatch) + rules = _write_rules(tmp_path) + monkeypatch.setenv("PYPLET_REQUIRE_AUTH", "1") + monkeypatch.setenv("PYPLET_ALLOW_MAGICLINK", "1") + monkeypatch.setenv("OAUTH_GOOGLE_CLIENT_ID", "x") + monkeypatch.setenv("OAUTH_GOOGLE_CLIENT_SECRET", "y") + monkeypatch.setenv("PYPLET_AUTH_RULES_FILE", str(rules)) + result = oauth.enforce_startup_auth_policy(magiclink_enabled=True) + assert result is None + + +# --------------------------------------------------------------------------- +# Production happy path — regression guard for a real deployment +# (PYPLET_REQUIRE_AUTH=1 + OAuth configured + rules file present + no +# magic-link). The fail-closed helper must NOT take down a correctly-configured +# production server (the story's latent-safe guarantee, AC1/AC5). +# --------------------------------------------------------------------------- + + +def test_enforce_allows_fully_configured_production_profile( + monkeypatch, tmp_path +): + """Production profile fully configured (OAuth + rules file, no magic-link) + → boots (does not raise).""" + _clear_auth_env(monkeypatch) + rules = _write_rules(tmp_path) + monkeypatch.setenv("PYPLET_REQUIRE_AUTH", "1") + monkeypatch.setenv("OAUTH_GOOGLE_CLIENT_ID", "x") + monkeypatch.setenv("OAUTH_GOOGLE_CLIENT_SECRET", "y") + monkeypatch.setenv("PYPLET_AUTH_RULES_FILE", str(rules)) + result = oauth.enforce_startup_auth_policy(magiclink_enabled=False) + assert result is None From c9849ea5085031c32ef9027df0194d0210ebe1b7 Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Mon, 8 Jun 2026 18:57:12 +0200 Subject: [PATCH 07/28] feat(auth): Secure session cookie and a required persistent cookie secret Two cookie-level holes on a public deployment: - session and state cookies carried no `Secure` attribute, so a downgrade to http could leak them. `Secure` is now set whenever the deployment is https (derived from `PYPLET_URL`), and omitted on plain-http local dev so the dev loop keeps working. - with no `PYPLET_COOKIE_SECRET`, a random secret was minted at each start, which silently invalidated every session on restart. On the production profile the secret is now required; locally the random fallback stays and logs a WARNING. +14 tests (`tests/oauth_cookie_security_test.py`, `tests/oauth_test.py`). (Story 17.7) --- README.md | 9 +- pyplet/server/config.py | 9 ++ pyplet/server/oauth.py | 31 ++++ tests/oauth_cookie_security_test.py | 218 ++++++++++++++++++++++++++++ tests/oauth_test.py | 6 + 5 files changed, 272 insertions(+), 1 deletion(-) create mode 100644 tests/oauth_cookie_security_test.py diff --git a/README.md b/README.md index 8f77c58..c8369d9 100644 --- a/README.md +++ b/README.md @@ -176,7 +176,9 @@ secret. Set the callback URL to: **2. Set environment variables:** ```bash -# Required to sign session cookies (generate once and keep it stable): +# Required & persistent in production — generate once and keep it stable. +# Under PYPLET_REQUIRE_AUTH=1 the server refuses to boot when this is unset +# (a per-process random secret logs out every user on each restart): export PYPLET_COOKIE_SECRET=$(python -c "import secrets; print(secrets.token_hex(32))") # Google @@ -261,6 +263,7 @@ WARNING that every request is served anonymously. | Variable | Description | | --- | --- | | `PYPLET_COOKIE_SECRET` | Secret for signing session cookies | +| `PYPLET_SECURE_COOKIES` | Force `Secure` attribute on auth cookies: `1`/`0` | | `PYPLET_REQUIRE_AUTH` | Fail-closed switch: `1` refuses boot, default `0` | | `PYPLET_ALLOW_MAGICLINK` | Opt magic-link IN on require-auth, default `0` | | **OAuth — Google** | | @@ -281,6 +284,10 @@ WARNING that every request is served anonymously. | **ACL** | | | `PYPLET_AUTH_RULES_FILE` | ACL rules path (default: `apps/auth_rules.json`) | +`PYPLET_COOKIE_SECRET` must be persistent and is required under +`PYPLET_REQUIRE_AUTH=1` (the server refuses to boot when unset); +`PYPLET_SECURE_COOKIES`, when unset, follows the `PYPLET_URL` scheme. + ## Advanced Features ### DOM Manipulation diff --git a/pyplet/server/config.py b/pyplet/server/config.py index aab541b..44cc509 100644 --- a/pyplet/server/config.py +++ b/pyplet/server/config.py @@ -124,6 +124,15 @@ class PypletConfig: ), env_var="PYPLET_COOKIE_SECRET", ) + secure_cookies = Param( + default=None, + description=( + "Force the Secure attribute on auth cookies ('1'/'0'). " + "When unset, derived from the https scheme of PYPLET_URL " + "(the deployed origin)." + ), + env_var="PYPLET_SECURE_COOKIES", + ) session_max_age_days = Param( default=1, description=( diff --git a/pyplet/server/oauth.py b/pyplet/server/oauth.py index 0f90a8d..bf2a76f 100644 --- a/pyplet/server/oauth.py +++ b/pyplet/server/oauth.py @@ -199,6 +199,22 @@ def _decode_id_token_claims(id_token: str) -> dict: SESSION_COOKIE = "pyplet_user" +def _use_secure_cookies() -> bool: + """Whether auth cookies must carry the ``Secure`` attribute. + + Behind a TLS-terminating edge the VM receives plain http with xheaders + OFF, so ``handler.request.protocol`` is unreliably ``"http"`` even for an + https client (audit DEPLOY-2). Decide from config, never request.protocol: + an explicit ``PYPLET_SECURE_COOKIES`` wins; otherwise derive from the https + scheme of ``config.url``. Plain-http local dev (flag unset, no https url) + → False, so dev still sets cookies over http. + """ + flag = config.secure_cookies + if flag is not None and str(flag).strip() != "": + return str(flag).strip().lower() in ("1", "true", "yes", "on") + return (config.url or "").lower().startswith("https://") + + def set_session(handler, user_info: dict) -> None: """Write a signed session cookie containing *user_info*.""" payload = json.dumps({**user_info, "_ts": int(time.time())}) @@ -208,6 +224,7 @@ def set_session(handler, user_info: dict) -> None: expires_days=config.session_max_age_days, httponly=True, samesite="Lax", + secure=_use_secure_cookies(), ) @@ -421,6 +438,18 @@ def enforce_startup_auth_policy(magiclink_enabled: bool = False) -> None: "boundary. Set PYPLET_ALLOW_MAGICLINK=1 to opt in explicitly." ) + # Story 17.7 (PB-9 / SECURI-9): a persistent signing secret is mandatory in + # production — without it _server.py:325 falls back to a per-process + # secrets.token_hex(32) that invalidates every session on each restart. + if production and not config.oauth_cookie_secret: + raise AuthConfigError( + "PYPLET_REQUIRE_AUTH=1 but PYPLET_COOKIE_SECRET is unset — " + "refusing to boot with a per-process random cookie secret " + "(it would log out every user on each restart). " + "Set a persistent PYPLET_COOKIE_SECRET " + '(python -c "import secrets; print(secrets.token_hex(32))").' + ) + # --------------------------------------------------------------------------- # OAuth login flow helpers (called by handlers in _server.py) @@ -448,6 +477,7 @@ async def start_login(handler, provider: str) -> None: json.dumps({"state": state, "next": next_url, "provider": provider}), httponly=True, samesite="Lax", + secure=_use_secure_cookies(), ) callback_url = _callback_url(handler) @@ -499,6 +529,7 @@ async def start_drive_consent(handler, next_url: str = "/") -> None: ), httponly=True, samesite="Lax", + secure=_use_secure_cookies(), ) callback_url = _callback_url(handler) diff --git a/tests/oauth_cookie_security_test.py b/tests/oauth_cookie_security_test.py new file mode 100644 index 0000000..e83d791 --- /dev/null +++ b/tests/oauth_cookie_security_test.py @@ -0,0 +1,218 @@ +"""Cookie-security tests for ``pyplet.server.oauth`` (Story 17.7, PB-9). + +Covers DEPLOY-2 (the ``Secure`` attribute on all three auth cookies, gated on +the deployed origin) and SECURI-9 (fail-fast on a missing persistent +``PYPLET_COOKIE_SECRET`` on the production profile). + +Mirrors ``tests/config_test.py`` / ``tests/oauth_test.py``: env-driven via the +``monkeypatch`` fixture, no live server. ``_use_secure_cookies`` and +``set_session`` are sync; the two OAuth-state writers (``start_login`` / +``start_drive_consent``) are async and use ``@pytest.mark.asyncio`` (asyncio is +in STRICT mode — pytest-asyncio 1.4.0 is installed). + +The Secure decision is gated on ``config.url`` / ``PYPLET_SECURE_COOKIES`` and +**never** on ``handler.request.protocol`` (xheaders is off on the VM behind the +TLS-terminating edge, so ``request.protocol`` is unreliably ``"http"`` even for +https clients — audit DEPLOY-2 verifier note). +""" + +import json +from unittest.mock import MagicMock + +import pytest + +from pyplet.server import oauth + +# Env vars that drive the Secure decision and the production startup gate. +_COOKIE_ENV_VARS = ( + "PYPLET_SECURE_COOKIES", + "PYPLET_URL", + "PYPLET_COOKIE_SECRET", + "PYPLET_REQUIRE_AUTH", + "PYPLET_ALLOW_MAGICLINK", + "PYPLET_AUTH_RULES_FILE", + "OAUTH_GOOGLE_CLIENT_ID", + "OAUTH_GOOGLE_CLIENT_SECRET", + "OAUTH_MICROSOFT_CLIENT_ID", + "OAUTH_MICROSOFT_CLIENT_SECRET", + "MAGICLINK_SMTP_HOST", + "MAGICLINK_SMTP_USER", + "MAGICLINK_SMTP_PASSWORD", +) + + +@pytest.fixture(autouse=True) +def _clear_cookie_env(monkeypatch): + """Strip every relevant env var so each test starts from a known state.""" + for var in _COOKIE_ENV_VARS: + monkeypatch.delenv(var, raising=False) + yield + + +def _enable_oauth(monkeypatch): + """Set Google OAuth env vars so ``auth_enabled()`` is True.""" + monkeypatch.setenv("OAUTH_GOOGLE_CLIENT_ID", "client-id") + monkeypatch.setenv("OAUTH_GOOGLE_CLIENT_SECRET", "client-secret") + + +def _write_rules(tmp_path): + """Write a real (well-formed) auth_rules.json and return its path.""" + rules = tmp_path / "auth_rules.json" + rules.write_text(json.dumps([[".*", "@example\\.com$"]])) + return rules + + +async def _stub_oidc(provider): + """Async stand-in for ``oauth._fetch_oidc_config`` (no network).""" + return { + "authorization_endpoint": ( + "https://accounts.google.com/o/oauth2/v2/auth" + ) + } + + +# --------------------------------------------------------------------------- +# AC3(a) — _use_secure_cookies() decision logic +# --------------------------------------------------------------------------- + + +def test_secure_cookies_true_for_https_url(monkeypatch): + """https PYPLET_URL (the deployed origin) → Secure on.""" + monkeypatch.setenv("PYPLET_URL", "https://pyplet.example.com") + assert oauth._use_secure_cookies() is True + + +def test_secure_cookies_false_for_http_url(monkeypatch): + """Plain-http PYPLET_URL → Secure off (local dev still sets cookies).""" + monkeypatch.setenv("PYPLET_URL", "http://127.0.0.1:8080") + assert oauth._use_secure_cookies() is False + + +def test_secure_cookies_false_when_url_unset(monkeypatch): + """Neither flag nor PYPLET_URL set (local dev default) → Secure off.""" + assert oauth._use_secure_cookies() is False + + +def test_secure_cookies_flag_true_overrides_http_url(monkeypatch): + """Explicit PYPLET_SECURE_COOKIES=1 wins over a plain-http url.""" + monkeypatch.setenv("PYPLET_URL", "http://127.0.0.1:8080") + monkeypatch.setenv("PYPLET_SECURE_COOKIES", "1") + assert oauth._use_secure_cookies() is True + + +def test_secure_cookies_flag_false_overrides_https_url(monkeypatch): + """Explicit PYPLET_SECURE_COOKIES=0 wins over an https url.""" + monkeypatch.setenv("PYPLET_URL", "https://pyplet.example.com") + monkeypatch.setenv("PYPLET_SECURE_COOKIES", "0") + assert oauth._use_secure_cookies() is False + + +# --------------------------------------------------------------------------- +# AC3(b) — secure= forwarded to set_signed_cookie on all three sites +# --------------------------------------------------------------------------- + + +def test_set_session_forwards_secure_true_on_https(monkeypatch): + """The session cookie carries secure=True on an https origin.""" + monkeypatch.setenv("PYPLET_URL", "https://pyplet.example.com") + handler = MagicMock() + oauth.set_session(handler, {"sub": "u1", "email": "a@example.com"}) + assert handler.set_signed_cookie.call_args.kwargs.get("secure") is True + + +def test_set_session_no_secure_on_plain_http(monkeypatch): + """The session cookie does not carry Secure in plain-http local dev.""" + monkeypatch.setenv("PYPLET_URL", "http://127.0.0.1:8080") + handler = MagicMock() + oauth.set_session(handler, {"sub": "u1", "email": "a@example.com"}) + assert not handler.set_signed_cookie.call_args.kwargs.get("secure") + + +@pytest.mark.asyncio +async def test_start_login_forwards_secure_true_on_https(monkeypatch): + """The login OAuth-state cookie carries secure=True on an https origin.""" + monkeypatch.setenv("PYPLET_URL", "https://pyplet.example.com") + _enable_oauth(monkeypatch) + monkeypatch.setattr(oauth, "_fetch_oidc_config", _stub_oidc) + handler = MagicMock() + handler.get_argument.return_value = "/" + await oauth.start_login(handler, "google") + assert handler.set_signed_cookie.call_args.kwargs.get("secure") is True + + +@pytest.mark.asyncio +async def test_start_drive_consent_forwards_secure_true_on_https(monkeypatch): + """The drive-consent OAuth-state cookie carries secure=True on https.""" + monkeypatch.setenv("PYPLET_URL", "https://pyplet.example.com") + _enable_oauth(monkeypatch) + monkeypatch.setattr(oauth, "_fetch_oidc_config", _stub_oidc) + handler = MagicMock() + await oauth.start_drive_consent(handler, "/") + assert handler.set_signed_cookie.call_args.kwargs.get("secure") is True + + +@pytest.mark.asyncio +async def test_start_login_no_secure_on_plain_http(monkeypatch): + """The login OAuth-state cookie does not carry Secure in plain-http dev.""" + monkeypatch.setenv("PYPLET_URL", "http://127.0.0.1:8080") + _enable_oauth(monkeypatch) + monkeypatch.setattr(oauth, "_fetch_oidc_config", _stub_oidc) + handler = MagicMock() + handler.get_argument.return_value = "/" + await oauth.start_login(handler, "google") + assert not handler.set_signed_cookie.call_args.kwargs.get("secure") + + +@pytest.mark.asyncio +async def test_start_drive_consent_no_secure_on_plain_http(monkeypatch): + """The drive-consent OAuth-state cookie omits Secure in plain-http dev.""" + monkeypatch.setenv("PYPLET_URL", "http://127.0.0.1:8080") + _enable_oauth(monkeypatch) + monkeypatch.setattr(oauth, "_fetch_oidc_config", _stub_oidc) + handler = MagicMock() + await oauth.start_drive_consent(handler, "/") + assert not handler.set_signed_cookie.call_args.kwargs.get("secure") + + +# --------------------------------------------------------------------------- +# AC3(c) — production fail-fast on a missing persistent PYPLET_COOKIE_SECRET +# (SECURI-9): the 4th check inside Story 17.6's enforce_startup_auth_policy(). +# --------------------------------------------------------------------------- + + +def test_enforce_raises_when_production_and_no_cookie_secret( + monkeypatch, tmp_path +): + """PYPLET_REQUIRE_AUTH=1 + auth on + unset secret → refuse to boot. + + A real rules file is supplied so 17.6's missing-rules check does not + mask the cookie-secret check; magic-link is off so its check is skipped. + """ + _enable_oauth(monkeypatch) + monkeypatch.setenv("PYPLET_REQUIRE_AUTH", "1") + monkeypatch.setenv("PYPLET_AUTH_RULES_FILE", str(_write_rules(tmp_path))) + monkeypatch.delenv("PYPLET_COOKIE_SECRET", raising=False) + assert oauth.auth_enabled() is True + with pytest.raises(oauth.AuthConfigError): + oauth.enforce_startup_auth_policy(magiclink_enabled=False) + + +def test_enforce_allows_when_cookie_secret_set(monkeypatch, tmp_path): + """Production profile with a persistent secret set → boots.""" + _enable_oauth(monkeypatch) + monkeypatch.setenv("PYPLET_REQUIRE_AUTH", "1") + monkeypatch.setenv("PYPLET_AUTH_RULES_FILE", str(_write_rules(tmp_path))) + monkeypatch.setenv("PYPLET_COOKIE_SECRET", "a" * 64) + result = oauth.enforce_startup_auth_policy(magiclink_enabled=False) + assert result is None + + +def test_enforce_allows_when_not_production_without_secret( + monkeypatch, tmp_path +): + """Local dev (PYPLET_REQUIRE_AUTH unset) + no secret → boots (no raise).""" + _enable_oauth(monkeypatch) + monkeypatch.setenv("PYPLET_AUTH_RULES_FILE", str(_write_rules(tmp_path))) + monkeypatch.delenv("PYPLET_COOKIE_SECRET", raising=False) + result = oauth.enforce_startup_auth_policy(magiclink_enabled=False) + assert result is None diff --git a/tests/oauth_test.py b/tests/oauth_test.py index 353b7f5..e5a342d 100644 --- a/tests/oauth_test.py +++ b/tests/oauth_test.py @@ -160,6 +160,9 @@ def test_enforce_allows_magiclink_with_explicit_optin(monkeypatch, tmp_path): monkeypatch.setenv("OAUTH_GOOGLE_CLIENT_ID", "x") monkeypatch.setenv("OAUTH_GOOGLE_CLIENT_SECRET", "y") monkeypatch.setenv("PYPLET_AUTH_RULES_FILE", str(rules)) + # Story 17.7 (PB-9): a fully-configured production profile also requires a + # persistent PYPLET_COOKIE_SECRET, else the 4th check refuses to boot. + monkeypatch.setenv("PYPLET_COOKIE_SECRET", "x" * 64) result = oauth.enforce_startup_auth_policy(magiclink_enabled=True) assert result is None @@ -183,5 +186,8 @@ def test_enforce_allows_fully_configured_production_profile( monkeypatch.setenv("OAUTH_GOOGLE_CLIENT_ID", "x") monkeypatch.setenv("OAUTH_GOOGLE_CLIENT_SECRET", "y") monkeypatch.setenv("PYPLET_AUTH_RULES_FILE", str(rules)) + # Story 17.7 (PB-9): a fully-configured production profile also requires a + # persistent PYPLET_COOKIE_SECRET, else the 4th check refuses to boot. + monkeypatch.setenv("PYPLET_COOKIE_SECRET", "x" * 64) result = oauth.enforce_startup_auth_policy(magiclink_enabled=False) assert result is None From a27cf85c140960cb7d30e2bd95e40850a1dc47ca Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Mon, 8 Jun 2026 20:27:19 +0200 Subject: [PATCH 08/28] docs(auth): state the cookie-secret and session-TTL semantics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PYPLET_COOKIE_SECRET` said only "sessions survive until restart", which hides the two things an operator needs: the secret is minted *per process*, and the production profile refuses to boot without it. Spelled out in both the `Param` description and the `oauth.py` module docstring. Also documents `PYPLET_SESSION_TTL_DAYS` in the README configuration reference — it was introduced earlier in this branch without a table entry. --- README.md | 1 + pyplet/server/config.py | 5 +++-- pyplet/server/oauth.py | 4 +++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index c8369d9..6ee4b74 100644 --- a/README.md +++ b/README.md @@ -266,6 +266,7 @@ WARNING that every request is served anonymously. | `PYPLET_SECURE_COOKIES` | Force `Secure` attribute on auth cookies: `1`/`0` | | `PYPLET_REQUIRE_AUTH` | Fail-closed switch: `1` refuses boot, default `0` | | `PYPLET_ALLOW_MAGICLINK` | Opt magic-link IN on require-auth, default `0` | +| `PYPLET_SESSION_TTL_DAYS` | Session cookie lifetime in days, default `1` | | **OAuth — Google** | | | `OAUTH_GOOGLE_CLIENT_ID` | Google OAuth2 client ID | | `OAUTH_GOOGLE_CLIENT_SECRET` | Google OAuth2 client secret | diff --git a/pyplet/server/config.py b/pyplet/server/config.py index 44cc509..4e23bc3 100644 --- a/pyplet/server/config.py +++ b/pyplet/server/config.py @@ -119,8 +119,9 @@ class PypletConfig: oauth_cookie_secret = Param( default=None, description=( - "Signs session cookies. Without this, a " - "random secret is generated at startup." + "Signs session cookies. Without this, a random secret is " + "generated per process (all sessions drop on restart); under " + "PYPLET_REQUIRE_AUTH=1 the server refuses to boot if unset." ), env_var="PYPLET_COOKIE_SECRET", ) diff --git a/pyplet/server/oauth.py b/pyplet/server/oauth.py index bf2a76f..c197ea4 100644 --- a/pyplet/server/oauth.py +++ b/pyplet/server/oauth.py @@ -24,7 +24,9 @@ PYPLET_COOKIE_SECRET Signs session cookies. Generate with: python -c "import secrets; print(secrets.token_hex(32))" - Without this, sessions survive only until the server restarts. + Without this, a random secret is generated per process, so sessions + survive only until the server restarts; under PYPLET_REQUIRE_AUTH=1 + the server refuses to boot if unset. Per-app access control (ACL) ------------------------------ From f2352eead577782fa4522c1369cd35e217ecaa37 Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Tue, 9 Jun 2026 08:43:21 +0200 Subject: [PATCH 09/28] feat(auth): verify the OIDC id_token signature against the provider JWKS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_decode_id_token_claims()` base64-decoded the id_token payload and trusted it. A forged token — anything shaped like a JWT — was accepted, so the login identity came from unauthenticated input. The token is now verified with `authlib` against the provider's JWKS (fetched from the OIDC discovery document, cached per provider, refreshed on an unknown `kid`), checking the signature, `iss`, `aud` and expiry before any claim is read. `authlib` was already a dependency; no new one is added. +10 tests (`tests/oauth_id_token_test.py`). (Story 18.19) --- pyplet/server/oauth.py | 144 ++++++++++++++++++++--- tests/oauth_id_token_test.py | 220 +++++++++++++++++++++++++++++++++++ 2 files changed, 346 insertions(+), 18 deletions(-) create mode 100644 tests/oauth_id_token_test.py diff --git a/pyplet/server/oauth.py b/pyplet/server/oauth.py index c197ea4..cc3c8f0 100644 --- a/pyplet/server/oauth.py +++ b/pyplet/server/oauth.py @@ -106,6 +106,12 @@ # OIDC discovery-doc cache _oidc_cache: dict[str, dict] = {} +# JWKS cache, keyed by provider (parallel to ``_oidc_cache``). The provider's +# signing keys rotate (Google rotates roughly daily), so a verify failure that +# looks like rotation triggers a single forced refetch — see +# ``_verify_id_token_claims``. +_jwks_cache: dict[str, dict] = {} + def enabled_providers() -> list[str]: """Return the names of providers that have a client_id configured.""" @@ -172,26 +178,127 @@ async def _fetch_oidc_config(provider: str) -> dict: return doc +async def _fetch_jwks(provider: str, *, force_refresh: bool = False) -> dict: + """Fetch (and cache) *provider*'s JWKS — its JSON Web Key Set. + + Reads ``jwks_uri`` from the already-cached OIDC discovery document and + GETs it with the same ``httpx`` pattern as :func:`_fetch_oidc_config`. + The result is cached per provider; ``force_refresh=True`` bypasses the + cache to pick up a rotated signing key (Google rotates its JWKS keys). + """ + if not force_refresh and provider in _jwks_cache: + return _jwks_cache[provider] + + oidc = await _fetch_oidc_config(provider) + jwks_uri = oidc["jwks_uri"] + + async with httpx.AsyncClient() as client: + resp = await client.get(jwks_uri, timeout=10) + resp.raise_for_status() + jwks = resp.json() + + _jwks_cache[provider] = jwks + logger.debug("Fetched JWKS for %s from %s", provider, jwks_uri) + return jwks + + # --------------------------------------------------------------------------- -# JWT / id_token decoding +# id_token signature verification (SECURI-8, Story 18.19) # --------------------------------------------------------------------------- -def _decode_id_token_claims(id_token: str) -> dict: +def _accepted_issuers(issuer: str) -> list[str]: + """Return the set of acceptable ``iss`` values for *issuer*. + + Google issues ``iss`` as either the ``https://accounts.google.com`` form or + the bare ``accounts.google.com`` host; the discovery document's ``issuer`` + is the ``https://`` form, so accept both. Microsoft's ``iss`` matches its + discovery ``issuer`` exactly, so for it this is effectively a singleton. """ - Decode JWT payload without signature verification. + out = [issuer] + if issuer.startswith("https://"): + out.append(issuer.removeprefix("https://")) + return out + - Safe here because the token is received directly from the provider - over TLS; no MITM is possible. For higher-assurance deployments - swap in full JWKS verification via ``authlib`` or ``python-jose``. +def _verify_id_token_against_jwks( + id_token: str, jwks: dict, *, issuer: str, audience: str +) -> dict: + """Verify the ``id_token`` signature against *jwks*; validate its claims. + + Pure and synchronous (no ``httpx``, no ``config``) so the crypto is + unit-testable with crafted JWTs and a stubbed JWKS — no async, no network. + Restricts the accepted signing algorithm to RS256 (Google and Microsoft + both sign with RS256) to close algorithm-confusion, resolves the signing + key from *jwks* by ``kid``, and validates ``iss``/``aud``/``exp``. + + Args: + id_token: The compact-serialized JWT from the token endpoint. + jwks: The provider JWKS (``{"keys": [...]}`` from ``jwks_uri``). + issuer: The discovery ``issuer`` — the expected ``iss`` (the bare-host + Google variant is also accepted, see :func:`_accepted_issuers`). + audience: The provider ``client_id`` — the expected ``aud``. + + Returns: + The validated claims as a plain ``dict``. + + Raises: + JoseError: On a bad/absent signature, a wrong/missing ``iss``/``aud``, + an expired/absent ``exp``, or a non-RS256 algorithm. + ValueError: When no JWKS key matches the token's ``kid`` (rotation). """ - import base64 + from authlib.jose import JsonWebKey, JsonWebToken + + key_set = JsonWebKey.import_key_set(jwks) + claims = JsonWebToken(["RS256"]).decode( + id_token, + key_set, + claims_options={ + "iss": {"essential": True, "values": _accepted_issuers(issuer)}, + "aud": {"essential": True, "value": audience}, + "exp": {"essential": True}, + }, + ) + claims.validate() + return dict(claims) + + +async def _verify_id_token_claims(id_token: str, provider: str) -> dict: + """Verify *id_token* against *provider*'s JWKS and return its claims. + + Async wrapper around the pure :func:`_verify_id_token_against_jwks`: + resolves the expected ``issuer`` (discovery ``issuer``) and ``audience`` + (the provider ``client_id``), fetches the JWKS, and verifies. On a + signature/key-resolution failure that looks like key rotation (a + ``BadSignatureError`` or an unknown-``kid`` ``ValueError``) it refetches + the JWKS **once** and retries — Google rotates its signing keys, so a + freshly rotated key may post-date the cache. Claim failures (wrong or + expired ``iss``/``aud``/``exp``) are NOT retried (the key already + resolved; a refetch cannot help) and propagate immediately. + + Raises: + Exception: Any verification failure. ``handle_callback``'s ``try`` + wraps it into the standard error page — no session is set. + """ + from authlib.jose.errors import BadSignatureError + + oidc = await _fetch_oidc_config(provider) + issuer = oidc["issuer"] + audience = _PROVIDER_CONFIGS[provider]["client_id"]() - parts = id_token.split(".") - if len(parts) != 3: - raise ValueError("Invalid JWT format") - payload_b64 = parts[1] + "=" * (-len(parts[1]) % 4) - return json.loads(base64.urlsafe_b64decode(payload_b64)) + jwks = await _fetch_jwks(provider) + try: + return _verify_id_token_against_jwks( + id_token, jwks, issuer=issuer, audience=audience + ) + except (BadSignatureError, ValueError): + # Signature/key-resolution miss — refetch once in case the signing key + # rotated past our cache, then retry. A genuinely bad signature fails + # again on the fresh keys and propagates. + jwks = await _fetch_jwks(provider, force_refresh=True) + return _verify_id_token_against_jwks( + id_token, jwks, issuer=issuer, audience=audience + ) # --------------------------------------------------------------------------- @@ -555,9 +662,10 @@ async def handle_callback(handler) -> None: """ Complete the OAuth authorization-code flow. - Validates state, exchanges the code for tokens, decodes the id_token, - sets the session cookie, and redirects to the originally requested URL. - Called by ``OAuthCallbackHandler`` in ``_server.py``. + Validates state, exchanges the code for tokens, verifies the id_token + signature against the provider JWKS, sets the session cookie, and redirects + to the originally requested URL. Called by ``OAuthCallbackHandler`` in + ``_server.py``. """ # --- Validate CSRF state --- raw_state = handler.get_signed_cookie(_STATE_COOKIE) @@ -622,10 +730,10 @@ async def handle_callback(handler) -> None: return try: - claims = _decode_id_token_claims(id_token) + claims = await _verify_id_token_claims(id_token, provider) except Exception as exc: - logger.error("id_token decode failed: %s", exc) - _error(handler, 500, "Could not decode the identity token.") + logger.error("id_token verification failed: %s", exc) + _error(handler, 500, "Could not verify the identity token.") return user_info = { diff --git a/tests/oauth_id_token_test.py b/tests/oauth_id_token_test.py new file mode 100644 index 0000000..cda8008 --- /dev/null +++ b/tests/oauth_id_token_test.py @@ -0,0 +1,220 @@ +"""id_token JWKS signature-verification tests (Story 18.19, SECURI-8). + +Exercises the pure, synchronous ``oauth._verify_id_token_against_jwks`` with +crafted JWTs signed by a test RSA key and a JWKS built from that key's public +half — no async, no network. Mirrors ``tests/oauth_test.py`` (sync ``def +test_*`` + ``monkeypatch``); ``authlib`` (already a dependency) mints the keys +and signs the tokens. + +A valid token verifies; a bad signature, a wrong ``aud``, a wrong ``iss``, an +expired ``exp``, or a non-RS256 algorithm is rejected. Google's bare-host +``iss`` variant (``accounts.google.com``) is accepted. The async wrapper +``_verify_id_token_claims`` is covered too — the discovery + JWKS fetches are +stubbed so no real HTTP happens — including the key-rotation single-refetch +path and the "do NOT refetch on a claim failure" amplification guard. + +``authlib.jose`` emits an ``AuthlibDeprecationWarning`` (superseded by +``joserfc`` in authlib 2.0 — migrating is out of scope); the module-level +filter keeps it from failing under a warnings-as-errors run. +""" + +from __future__ import annotations + +import asyncio +import json +import time + +import pytest +from authlib.jose import JsonWebKey, JsonWebToken +from authlib.jose.errors import JoseError + +from pyplet.server import oauth + +pytestmark = pytest.mark.filterwarnings( + "ignore:authlib.jose module is deprecated" +) + +_RS = JsonWebToken(["RS256"]) +_ISSUER = "https://accounts.google.com" +_AUDIENCE = "client-id.apps.googleusercontent.com" + + +def _make_key() -> JsonWebKey: + """Return a fresh private RSA ``JsonWebKey`` (a signing key).""" + return JsonWebKey.generate_key("RSA", 2048, is_private=True) + + +def _jwks(key: JsonWebKey, kid: str = "k1") -> dict: + """Build a JWKS dict (``{"keys": [...]}``) from *key*'s public half.""" + return {"keys": [key.as_dict(is_private=False, kid=kid)]} + + +def _sign(key: JsonWebKey, claims: dict, *, kid: str = "k1") -> str: + """Sign *claims* into a compact RS256 JWT with *key* (kid in header).""" + token = _RS.encode({"alg": "RS256", "kid": kid}, claims, key) + return token.decode() if isinstance(token, bytes) else token + + +def _claims(**overrides) -> dict: + """A baseline valid claim set; override individual fields per test.""" + base = { + "iss": _ISSUER, + "aud": _AUDIENCE, + "exp": int(time.time()) + 3600, + "sub": "1234567890", + "email": "user@example.com", + "name": "Test User", + } + base.update(overrides) + return base + + +# --------------------------------------------------------------------------- +# Pure verifier — the crafted-JWT-vs-stubbed-JWKS cases (a)-(f) + alg restrict +# --------------------------------------------------------------------------- + + +def test_valid_token_returns_claims(): + """(a) Good signature + correct iss/aud + future exp ⇒ claims returned.""" + key = _make_key() + token = _sign(key, _claims()) + out = oauth._verify_id_token_against_jwks( + token, _jwks(key), issuer=_ISSUER, audience=_AUDIENCE + ) + assert out["sub"] == "1234567890" + assert out["email"] == "user@example.com" + + +def test_bad_signature_rejected(): + """(b) Signed by a DIFFERENT key than the JWKS advertises ⇒ rejected.""" + signing_key = _make_key() + advertised_key = _make_key() + token = _sign(signing_key, _claims()) # kid=k1, signed by signing_key + jwks = _jwks(advertised_key) # kid=k1 maps to a different public key + with pytest.raises(JoseError): + oauth._verify_id_token_against_jwks( + token, jwks, issuer=_ISSUER, audience=_AUDIENCE + ) + + +def test_wrong_audience_rejected(): + """(c) aud does not match the configured client_id ⇒ rejected.""" + key = _make_key() + token = _sign(key, _claims(aud="someone-else.apps.googleusercontent.com")) + with pytest.raises(JoseError): + oauth._verify_id_token_against_jwks( + token, _jwks(key), issuer=_ISSUER, audience=_AUDIENCE + ) + + +def test_wrong_issuer_rejected(): + """(d) iss is not an accepted issuer ⇒ rejected.""" + key = _make_key() + token = _sign(key, _claims(iss="https://evil.example.com")) + with pytest.raises(JoseError): + oauth._verify_id_token_against_jwks( + token, _jwks(key), issuer=_ISSUER, audience=_AUDIENCE + ) + + +def test_expired_token_rejected(): + """(e) exp is in the past ⇒ rejected.""" + key = _make_key() + token = _sign(key, _claims(exp=int(time.time()) - 60)) + with pytest.raises(JoseError): + oauth._verify_id_token_against_jwks( + token, _jwks(key), issuer=_ISSUER, audience=_AUDIENCE + ) + + +def test_google_bare_host_issuer_accepted(): + """(f) Google's bare-host iss (accounts.google.com) verifies against the + https:// discovery issuer.""" + key = _make_key() + token = _sign(key, _claims(iss="accounts.google.com")) + out = oauth._verify_id_token_against_jwks( + token, _jwks(key), issuer=_ISSUER, audience=_AUDIENCE + ) + assert out["sub"] == "1234567890" + + +def test_non_rs256_algorithm_rejected(): + """An HS256-signed token (alg-confusion payload) is rejected — the RS256 + whitelist closes alg-confusion even with a guessable HMAC secret.""" + rsa_key = _make_key() + pub = rsa_key.as_dict(is_private=False, kid="k1") + # Canonical attack: forge an HS256 token using the public-key material as + # the HMAC secret. The verifier must reject on the algorithm alone. + forged = JsonWebToken(["HS256"]).encode( + {"alg": "HS256", "kid": "k1"}, _claims(), json.dumps(pub).encode() + ) + forged = forged.decode() if isinstance(forged, bytes) else forged + with pytest.raises((JoseError, ValueError)): + oauth._verify_id_token_against_jwks( + forged, _jwks(rsa_key), issuer=_ISSUER, audience=_AUDIENCE + ) + + +# --------------------------------------------------------------------------- +# Async wrapper — stubbed discovery + JWKS fetches (no real HTTP) +# --------------------------------------------------------------------------- + + +def _stub_fetches(monkeypatch, jwks_for): + """Stub ``_fetch_oidc_config`` + ``_fetch_jwks`` + the google client_id. + + ``jwks_for`` is a callable ``(force_refresh: bool) -> dict`` so a test can + return a different keyset on the forced refetch (key-rotation simulation). + Returns a ``calls`` dict counting ``_fetch_jwks`` invocations. + """ + calls = {"jwks": 0} + + async def _fake_oidc(provider): + return {"issuer": _ISSUER, "jwks_uri": "https://example/jwks"} + + async def _fake_jwks(provider, *, force_refresh=False): + calls["jwks"] += 1 + return jwks_for(force_refresh) + + monkeypatch.setattr(oauth, "_fetch_oidc_config", _fake_oidc) + monkeypatch.setattr(oauth, "_fetch_jwks", _fake_jwks) + monkeypatch.setitem( + oauth._PROVIDER_CONFIGS["google"], "client_id", lambda: _AUDIENCE + ) + return calls + + +def test_async_wrapper_verifies_with_stubbed_fetches(monkeypatch): + """Async wrapper resolves issuer/audience + JWKS and verifies a token.""" + key = _make_key() + token = _sign(key, _claims()) + calls = _stub_fetches(monkeypatch, lambda _force: _jwks(key)) + out = asyncio.run(oauth._verify_id_token_claims(token, "google")) + assert out["sub"] == "1234567890" + assert calls["jwks"] == 1 # verified on the first (cached) keyset + + +def test_async_wrapper_refetches_jwks_on_rotation(monkeypatch): + """A signature miss against the cached keyset triggers exactly one forced + refetch; the rotated key then verifies.""" + old_key = _make_key() + new_key = _make_key() + token = _sign(new_key, _claims()) # signed by the rotated key (kid=k1) + calls = _stub_fetches( + monkeypatch, + lambda force: _jwks(new_key) if force else _jwks(old_key), + ) + out = asyncio.run(oauth._verify_id_token_claims(token, "google")) + assert out["sub"] == "1234567890" + assert calls["jwks"] == 2 # initial cached miss + one forced refetch + + +def test_async_wrapper_does_not_refetch_on_claim_failure(monkeypatch): + """A wrong-aud token (valid signature) must NOT trigger a JWKS refetch — + refetching cannot fix a claim failure and is an amplification vector.""" + key = _make_key() + token = _sign(key, _claims(aud="wrong-audience")) + calls = _stub_fetches(monkeypatch, lambda _force: _jwks(key)) + with pytest.raises(JoseError): + asyncio.run(oauth._verify_id_token_claims(token, "google")) + assert calls["jwks"] == 1 # NO refetch on a claim failure From 03cbffba614a03a63b9f758cbd671535df3311e3 Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Tue, 9 Jun 2026 01:14:48 +0200 Subject: [PATCH 10/28] feat(server): unauthenticated /healthz liveness route MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A load balancer, systemd unit or k8s probe had no way to tell a live process from a dead one without authenticating: every route sat behind the auth gate or the catch-all redirect to `/login`. `GET /healthz` answers 200 with a small JSON body. `HealthzHandler` is deliberately a plain `RequestHandler` — it does NOT subclass `_AuthMixin`, so the probe adds no authenticated surface — and it is registered before the terminal `r"/.*"` redirect, which stays last. Liveness only: process-up, no dependency checks (a readiness probe belongs to the application). +3 tests (`tests/healthz_test.py`). (Story 18.6) --- pyplet/server/_server.py | 18 +++++++++ tests/healthz_test.py | 85 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+) create mode 100644 tests/healthz_test.py diff --git a/pyplet/server/_server.py b/pyplet/server/_server.py index 0262488..c3dea55 100644 --- a/pyplet/server/_server.py +++ b/pyplet/server/_server.py @@ -282,6 +282,23 @@ async def get(self): self.redirect("/") +class HealthzHandler(tornado.web.RequestHandler): + """ + GET /healthz — unauthenticated process-liveness probe. + + Deliberately a plain handler (NOT ``_AuthMixin``): a liveness probe must + answer for an LB / systemd / k8s without a session, even when auth is + enabled. Process-up only — it performs NO database / provider / event-loop + checks (deep readiness is the app's ``/readyz`` route). Lives in core's + static ``_app_spec`` so it answers for every pyplet app, even when an app + module failed to import (``astart`` swallows app-import errors). + """ + + async def get(self): + self.set_header("Content-Type", "application/json") + self.write({"status": "ok"}) + + class OAuthLoginHandler(tornado.web.RequestHandler): """ GET /oauth/login?provider=<name> — kick off the OAuth flow. @@ -376,6 +393,7 @@ async def aclose(self): tornado.web.StaticFileHandler, {"path": os.path.join(config.apps, "../pyodide")}, ), + (r"/healthz", HealthzHandler), (r"/", IndexHandler), (r"/about", AboutHandler), (r"/login", LoginHandler), diff --git a/tests/healthz_test.py b/tests/healthz_test.py new file mode 100644 index 0000000..1587652 --- /dev/null +++ b/tests/healthz_test.py @@ -0,0 +1,85 @@ +"""Liveness route tests (Story 18.6 — OBSERV-3). + +WHAT THIS PROVES +================ +Story 18.6 adds an **unauthenticated** ``GET /healthz`` liveness route to the +pyplet-core Tornado handler table (``_server._app_spec["handlers"]``) so an +LB / systemd / k8s probe can tell a live process from a dead one without +authenticating. The route is process-up only (no DB / provider / loop checks — +those belong in an application-level ``/readyz``). This test pins three +properties: + + (a) **answers 200 unauthenticated** — a plain handler (NOT ``_AuthMixin``) + so ``GET /healthz`` returns 200 with a small JSON body and does NOT + 302→``/``→``/login``; + (b) **routed before the catch-all** — ``/healthz`` precedes the terminal + ``r"/.*"`` ``RedirectHandler`` in ``_app_spec["handlers"]`` (else the + catch-all would shadow it into a redirect), and the catch-all stays last; + (c) **unauthenticated by construction** — ``HealthzHandler`` does not + subclass ``_AuthMixin`` (the auth gate), so no auth surface is added. + +These are sync ``def test_*`` functions; the HTTP assertion uses Tornado's +``AsyncHTTPTestCase`` harness which ships with the existing ``tornado`` dep — +no new dependency. +""" + +from tornado.testing import AsyncHTTPTestCase +from tornado.web import Application + +from pyplet.server._server import HealthzHandler, _app_spec, _AuthMixin + + +def _handler_patterns() -> list[str]: + """Pattern strings of ``_app_spec['handlers']``, in declared order.""" + return [entry[0] for entry in _app_spec["handlers"]] + + +def test_healthz_precedes_catchall(): + """``/healthz`` is registered before the terminal ``r"/.*"`` catch-all. + + The catch-all ``RedirectHandler`` must stay the LAST entry; a ``/healthz`` + listed earlier is matched first, so the probe gets 200 instead of a + 302→``/``. If this inverts, the route silently becomes a redirect. + """ + patterns = _handler_patterns() + assert r"/healthz" in patterns, "/healthz route missing from _app_spec" + assert r"/.*" in patterns, "catch-all r'/.*' missing from _app_spec" + assert patterns.index(r"/healthz") < patterns.index(r"/.*"), ( + "/healthz must precede the catch-all r'/.*' or it gets shadowed " + "into a redirect" + ) + assert patterns[-1] == r"/.*", "the catch-all r'/.*' must stay last" + + +def test_healthz_handler_is_unauthenticated_by_construction(): + """``HealthzHandler`` must NOT subclass ``_AuthMixin`` (no auth gate). + + A liveness probe must answer without a session; inheriting the auth mixin + would gate it behind ``_require_auth`` (302/401). This is the structural + guarantee that no auth surface is added. + """ + assert not issubclass(HealthzHandler, _AuthMixin), ( + "HealthzHandler must not subclass _AuthMixin — /healthz is " + "unauthenticated by construction" + ) + + +class HealthzRouteTest(AsyncHTTPTestCase): + """Real-HTTP assertion: ``GET /healthz`` answers 200, no auth.""" + + def get_app(self) -> Application: + # Build a real Tornado app from the core handler table. astart() is + # NOT called, so no app-declared routes are spliced — /healthz is a + # static entry in _app_spec and is reachable on its own. + return Application(**_app_spec) + + def test_healthz_returns_200_without_auth(self): + """``GET /healthz`` ⇒ 200 + JSON body, no redirect, no cookie sent.""" + # follow_redirects=False so a (regression) 302→/login surfaces as a + # 3xx code rather than being silently followed to the login page. + resp = self.fetch("/healthz", follow_redirects=False) + assert resp.code == 200, ( + f"/healthz should answer 200 unauthenticated, got {resp.code}" + ) + body = resp.body.decode("utf-8") + assert "ok" in body, f"expected a tiny 'ok' body, got {body!r}" From fce2356b95219d72962bd032586f44e3ee664ccd Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Tue, 9 Jun 2026 06:58:13 +0200 Subject: [PATCH 11/28] feat(server): PYPLET_WS_MAX_MESSAGE_MB knob for the WebSocket frame size MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tornado caps a WebSocket message at 10 MB and closes the connection past it, which an app moving large binary frames hits with no recourse other than patching the server. `websocket_max_message_size` is now part of `_app_spec`, driven by `PYPLET_WS_MAX_MESSAGE_MB`. The default is raised to 40 MB — enough for a 25 MB base64'd upload (~33 MB on the wire) with headroom — so this does change the shipped limit; lower it if a deployment wants Tornado's 10 MB. +4 tests (`tests/config_test.py`, `tests/server_settings_test.py`). (Story 18.17) --- README.md | 1 + pyplet/server/_server.py | 5 +++++ pyplet/server/config.py | 11 +++++++++++ tests/config_test.py | 32 +++++++++++++++++++++++++++++++- tests/server_settings_test.py | 27 +++++++++++++++++++++++++++ 5 files changed, 75 insertions(+), 1 deletion(-) create mode 100644 tests/server_settings_test.py diff --git a/README.md b/README.md index 6ee4b74..ca9ab6b 100644 --- a/README.md +++ b/README.md @@ -369,6 +369,7 @@ Available configuration options: - `--debug` / `PYPLET_DEBUG` - Debug mode (default: `1`) - `--pyodide-url` / `PYPLET_PYODIDE` - Pyodide CDN URL - `--url` / `PYPLET_URL` - Custom URL override +- `PYPLET_WS_MAX_MESSAGE_MB` - Max WebSocket frame size MB (default: `40`) See the [Authentication](#authentication) section for OAuth-related variables. diff --git a/pyplet/server/_server.py b/pyplet/server/_server.py index c3dea55..454f836 100644 --- a/pyplet/server/_server.py +++ b/pyplet/server/_server.py @@ -438,6 +438,11 @@ async def aclose(self): # server still works without PYPLET_COOKIE_SECRET # (sessions lost on restart). "cookie_secret": config.oauth_cookie_secret or secrets.token_hex(32), + # Max WebSocket frame size. Tornado defaults to ~10 MB, which a base64'd + # document upload exceeds (killing the socket before app code runs). The + # default 40 MB carries a 25 MB upload (~33 MB frame) with headroom; raise + # PYPLET_WS_MAX_MESSAGE_MB in lockstep with any app's per-document cap. + "websocket_max_message_size": config.ws_max_message_mb * 1024 * 1024, } diff --git a/pyplet/server/config.py b/pyplet/server/config.py index 4e23bc3..21a769c 100644 --- a/pyplet/server/config.py +++ b/pyplet/server/config.py @@ -82,6 +82,17 @@ class PypletConfig: type_cast=int, env_var="PYPLET_PORT", ) + ws_max_message_mb = Param( + default=40, + description=( + "Max WebSocket frame size in MB (Tornado " + "websocket_max_message_size). 40 MB carries a 25 MB base64'd " + "upload (~33 MB frame) with headroom. Raise in lockstep with " + "any app's per-document upload cap." + ), + type_cast=int, + env_var="PYPLET_WS_MAX_MESSAGE_MB", + ) # pyodide_url = Param( # default="https://cdn.jsdelivr.net/pyodide/v0.29.0/full/pyodide.js", # description="The URL to fetch Pyodide from.", diff --git a/tests/config_test.py b/tests/config_test.py index 1f8b26c..8bc11ac 100644 --- a/tests/config_test.py +++ b/tests/config_test.py @@ -1,7 +1,7 @@ import pytest # Assuming this is your import path: -from pyplet.server.config import Param +from pyplet.server.config import Param, PypletConfig class MockConfig: @@ -102,3 +102,33 @@ def test_resolution_priority(self, config, monkeypatch): # 3. Instance explicit set takes precedence over environment config.port = 6000 assert config.port == 6000 + + +class TestWsMaxMessageMbParam: + """Story 18.17: the PYPLET_WS_MAX_MESSAGE_MB knob on the real config. + + Mirrors the ``port`` Param idiom above but on the concrete + ``PypletConfig`` so the real env var name is exercised. The env override is + verified on the Param here, not on the import-frozen ``_app_spec``. + """ + + @pytest.fixture + def config(self): + """A fresh PypletConfig instance for each test.""" + return PypletConfig() + + def test_default_is_40(self, config): + """Default frame cap is 40 MB (carries a 25 MB base64'd upload).""" + assert config.ws_max_message_mb == 40 + + def test_env_var_name(self): + """The Param binds the PYPLET_WS_MAX_MESSAGE_MB env variable.""" + assert ( + PypletConfig.ws_max_message_mb.env_var + == "PYPLET_WS_MAX_MESSAGE_MB" + ) + + def test_env_override_casts_to_int(self, config, monkeypatch): + """An env override wins over the default and is cast to int.""" + monkeypatch.setenv("PYPLET_WS_MAX_MESSAGE_MB", "64") + assert config.ws_max_message_mb == 64 diff --git a/tests/server_settings_test.py b/tests/server_settings_test.py new file mode 100644 index 0000000..921d369 --- /dev/null +++ b/tests/server_settings_test.py @@ -0,0 +1,27 @@ +"""WebSocket frame-size setting (Story 18.17 — SCALIN-4). + +pyplet-core previously set no ``websocket_max_message_size`` on its Tornado +``Application``, so the WS handler kept Tornado's ~10 MB default and a base64'd +document upload exceeded the frame and killed the socket before app code ran. +Story 18.17 adds the setting to ``_app_spec`` from the new +``PYPLET_WS_MAX_MESSAGE_MB`` knob (default 40 MB). + +The env override is covered at the ``Param`` level in ``config_test.py``. +``_app_spec`` is frozen at module-import time, so this test pins only the +concrete default on the frozen spec (a computed comparison against +``config.ws_max_message_mb`` would be flaky under a prior env monkeypatch). +""" + +from pyplet.server._server import _app_spec + + +def test_app_spec_sets_websocket_max_message_size(): + """``_app_spec`` carries ``websocket_max_message_size`` (40 MB default). + + Asserts the concrete default (``40 * 1024 * 1024``), NOT + ``config.ws_max_message_mb * 1024 * 1024``: ``_app_spec`` is built once at + import while the ``Param`` re-reads the env at access time, so the computed + comparison would be flaky if a prior test monkeypatched the env var. + """ + assert "websocket_max_message_size" in _app_spec + assert _app_spec["websocket_max_message_size"] == 40 * 1024 * 1024 From c363c993f8ecb3d213a0d1f25ae0035b6dd32636 Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Tue, 9 Jun 2026 07:43:30 +0200 Subject: [PATCH 12/28] =?UTF-8?q?feat(server):=20Tornado=20production=20ha?= =?UTF-8?q?rdening=20=E2=80=94=20debug=20off,=20xheaders,=20check=5Forigin?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three ways the Tornado layer stayed in dev posture on a public deployment: - `PYPLET_DEBUG` defaults to `1`, so a deployment that forgot to set it ran autoreload and served traceback pages. On the production profile (`PYPLET_REQUIRE_AUTH=1`) `enforce_startup_debug_policy()` now refuses the boot with `DebugConfigError`. Off that profile it is a no-op, so the local dev loop keeps debug + autoreload. - behind a reverse proxy the app saw the proxy's IP and scheme: `app.listen(..., xheaders=True)` trusts `X-Forwarded-For` / `-Proto`. No proxy in local dev means the headers are absent and behaviour is unchanged. - WebSocket `check_origin` accepted any origin. It now allows same-origin (Tornado's default) plus the deployed `PYPLET_URL` host, so an edge that rewrites `Host` still works while a foreign origin is refused. The debug policy runs next to the auth policy in `astart`, before the Tornado `Application` is built. +10 tests (`tests/prod_hardening_test.py`). (Story 18.18) --- pyplet/server/_server.py | 86 ++++++++++++++- tests/prod_hardening_test.py | 195 +++++++++++++++++++++++++++++++++++ 2 files changed, 280 insertions(+), 1 deletion(-) create mode 100644 tests/prod_hardening_test.py diff --git a/pyplet/server/_server.py b/pyplet/server/_server.py index 454f836..65fcfca 100644 --- a/pyplet/server/_server.py +++ b/pyplet/server/_server.py @@ -11,6 +11,7 @@ import sys import textwrap import types +import urllib.parse from pathlib import Path from typing import Dict, Optional, Tuple @@ -354,6 +355,39 @@ class ServerWebSocket(_AuthMixin, tornado.websocket.WebSocketHandler): closing_message = pyplet.WebSocket.closing_message _is_ws = True + def check_origin(self, origin: str) -> bool: + """Allow same-origin WS upgrades OR the deployed ``PYPLET_URL`` origin. + + Story 18.18 (SECURI-4). Tornado's default ``check_origin`` accepts only + a request whose ``Origin`` host equals the ``Host`` header — which the + edge can break by rewriting ``Host``. We additionally allow an origin + whose host matches the configured deployed origin (``config.url`` / + ``PYPLET_URL``), compared host-only so the edge's scheme/port do not + matter. When ``PYPLET_URL`` is unset (local dev) we fall back to + Tornado's default same-origin result, so ``localhost`` still connects. + + Caveat (documented): Tornado invokes ``check_origin`` ONLY when an + ``Origin`` header is present, so an Origin-less (non-browser) upgrade + is not blocked here — the real gate against anonymous access remains + ``_AuthMixin._require_auth`` in ``open``. + + Args: + origin: The request's ``Origin`` header value. + + Returns: + ``True`` to accept the cross-origin upgrade, ``False`` to reject + (Tornado answers the handshake with 403). + """ + if super().check_origin(origin): + return True + allowed = config.url + if not allowed: + return False + return ( + urllib.parse.urlparse(origin).hostname + == urllib.parse.urlparse(allowed).hostname + ) + async def open(self, project_name, app_name): user = self._require_auth(project_name, app_name) if user is None: @@ -504,6 +538,49 @@ def _load_server_module(path: str) -> str: return module.__name__ +# --------------------------------------------------------------------------- +# Fail-closed startup policy — production debug guard (Story 18.18, DEPLOY-8) +# --------------------------------------------------------------------------- + + +class DebugConfigError(RuntimeError): + """Raised at startup when the production profile runs with debug on. + + Refusing to boot is intentional: Tornado debug mode enables autoreload and + full traceback pages, which must never be exposed on the production profile + (``PYPLET_REQUIRE_AUTH=1``). Mirrors ``oauth.AuthConfigError``. + """ + + +def enforce_startup_debug_policy() -> None: + """Refuse to boot the production profile with Tornado debug mode on + (DEPLOY-8, Story 18.18). + + On the production profile (``PYPLET_REQUIRE_AUTH=1``) raises + ``DebugConfigError`` when ``PYPLET_DEBUG=1`` (the ``config.py`` default), + because debug mode enables autoreload and exposes traceback pages — leaking + source/stack and re-exec'ing on file change. Off the production profile + (``PYPLET_REQUIRE_AUTH`` unset/``0``) it is a no-op, so debug + autoreload + stay available for the everyday local dev loop. + + The gate is ``config.require_auth`` — NOT ``oauth.auth_enabled()`` — + deliberately mirroring ``oauth.enforce_startup_auth_policy``'s own + production gate. Gating on ``auth_enabled()`` would brick the authenticated + dev loop (a provider client-id set + debug + autoreload), which is a + daily-driver, not production. + + Side effects: reads ``config``. + Raises: ``DebugConfigError`` to abort boot on a production-profile breach. + """ + if config.require_auth == "1" and config.debug == "1": + raise DebugConfigError( + "PYPLET_REQUIRE_AUTH=1 (production profile) but PYPLET_DEBUG=1 — " + "refusing to boot. Tornado debug mode enables autoreload and " + "exposes traceback pages. Set PYPLET_DEBUG=0 in production, or " + "unset PYPLET_REQUIRE_AUTH for an explicitly open local-dev run." + ) + + async def astart(): # Load all server applications FIRST: importing each *_server.py # fires ServerApplication.__init_subclass__, which registers the @@ -524,6 +601,10 @@ async def astart(): # every discovered application. oauth.enforce_startup_auth_policy(magiclink_enabled=magiclink.enabled()) + # Story 18.18 (DEPLOY-8): on the production profile, refuse to boot with + # Tornado debug on (autoreload + traceback pages must never ship to prod). + enforce_startup_debug_policy() + favicon_uri = None if config.favicon: # Relative paths (e.g. the default "../images/...") are resolved @@ -546,7 +627,10 @@ async def astart(): _app_spec["favicon_data_uri"] = favicon_uri app = tornado.web.Application(**_app_spec) - app.listen(config.port, config.address) + # Story 18.18 (DEPLOY-8): trust the edge's X-Forwarded-For / -Proto so the + # app sees the real client IP + https scheme behind the reverse proxy. No + # proxy in local dev ⇒ those headers are absent ⇒ behavior unchanged. + app.listen(config.port, config.address, xheaders=True) url = config.url or f"http://{config.address}:{config.port}" logger.info(f"Pyplet server started on {url}") diff --git a/tests/prod_hardening_test.py b/tests/prod_hardening_test.py new file mode 100644 index 0000000..1be38fb --- /dev/null +++ b/tests/prod_hardening_test.py @@ -0,0 +1,195 @@ +"""Tornado production-hardening tests (Story 18.18). + +WHAT THIS PROVES +================ +Story 18.18 hardens the single shared Tornado app (``_app_spec`` + the one +``ServerWebSocket``) that fronts EVERY pyplet app behind the edge. Three +pyplet-core changes are pinned here (DEPLOY-8 / SECURI-4): + + (1) **debug asserted OFF in production** — ``enforce_startup_debug_policy`` + refuses to boot when the production profile is active + (``PYPLET_REQUIRE_AUTH=1``) AND ``PYPLET_DEBUG=1`` (the config default), + because Tornado debug mode ships autoreload + traceback pages. It is a + no-op off the production profile, so the everyday (even authenticated) + local dev loop keeps debug + autoreload. Gating mirrors + ``oauth.enforce_startup_auth_policy`` (``require_auth``, NOT + ``auth_enabled()``). + (2) **``check_origin`` allowlist** — ``ServerWebSocket.check_origin`` allows + the same-origin case (Tornado default) OR an origin whose host matches + the deployed ``PYPLET_URL`` host (the edge may rewrite ``Host``); a + foreign origin is rejected; with ``PYPLET_URL`` unset it falls back to + the default same-origin check (localhost dev still connects). + (3) **``xheaders=True`` on listen** — ``astart`` passes ``xheaders=True`` to + ``app.listen`` so the edge's ``X-Forwarded-For`` / ``X-Forwarded-Proto`` + are trusted (real client IP + https scheme behind the proxy). + +These are sync ``def test_*`` functions using ``monkeypatch`` (env-driven +``config``), mirroring ``tests/oauth_test.py``; the xheaders assertion drives +the ``astart`` path with a captured, sentinel-stopped ``Application.listen`` so +the real (blocking) server never starts. +""" + +import asyncio +from types import SimpleNamespace + +import pytest + +from pyplet.server import _server + +# --------------------------------------------------------------------------- +# Env hygiene +# --------------------------------------------------------------------------- + +_HARDENING_ENV_VARS = ("PYPLET_REQUIRE_AUTH", "PYPLET_DEBUG", "PYPLET_URL") + + +def _clear_hardening_env(monkeypatch): + """Remove the env vars that drive the three hardening checks.""" + for var in _HARDENING_ENV_VARS: + monkeypatch.delenv(var, raising=False) + + +# --------------------------------------------------------------------------- +# AC1 — debug asserted OFF in production (enforce_startup_debug_policy) +# --------------------------------------------------------------------------- + + +def test_debug_policy_raises_when_production_and_debug_on(monkeypatch): + """Production profile (require_auth=1) + debug=1 → refuse to boot.""" + _clear_hardening_env(monkeypatch) + monkeypatch.setenv("PYPLET_REQUIRE_AUTH", "1") + monkeypatch.setenv("PYPLET_DEBUG", "1") + with pytest.raises(_server.DebugConfigError): + _server.enforce_startup_debug_policy() + + +def test_debug_policy_allows_production_with_debug_off(monkeypatch): + """Production profile + debug=0 → boots (the correct prod combination).""" + _clear_hardening_env(monkeypatch) + monkeypatch.setenv("PYPLET_REQUIRE_AUTH", "1") + monkeypatch.setenv("PYPLET_DEBUG", "0") + assert _server.enforce_startup_debug_policy() is None + + +def test_debug_policy_allows_when_not_production_even_with_debug_on( + monkeypatch, +): + """require_auth=0 + debug=1 → no raise (local dev keeps autoreload).""" + _clear_hardening_env(monkeypatch) + monkeypatch.setenv("PYPLET_REQUIRE_AUTH", "0") + monkeypatch.setenv("PYPLET_DEBUG", "1") + assert _server.enforce_startup_debug_policy() is None + + +def test_debug_policy_allows_when_require_auth_unset_and_debug_on(monkeypatch): + """require_auth unset (default '0') + debug=1 → no raise. + + The authenticated dev loop (a provider client-id set, but require_auth + unset) must still boot with debug + autoreload — the gate is + ``require_auth``, NOT ``auth_enabled()``. + """ + _clear_hardening_env(monkeypatch) + monkeypatch.setenv("PYPLET_DEBUG", "1") + assert _server.enforce_startup_debug_policy() is None + + +# --------------------------------------------------------------------------- +# AC3 — check_origin allowlist on ServerWebSocket +# --------------------------------------------------------------------------- + + +def _make_ws(host_header): + """Build a bare ``ServerWebSocket`` with only a ``Host`` request header. + + ``check_origin`` (ours + Tornado's default) reads only + ``self.request.headers['Host']``, so ``__new__`` (skipping the full + handler/connection init) plus a stub request is sufficient to exercise the + method logic directly. + """ + ws = _server.ServerWebSocket.__new__(_server.ServerWebSocket) + ws.request = SimpleNamespace(headers={"Host": host_header}) + return ws + + +def test_check_origin_same_origin_allowed_without_allowlist(monkeypatch): + """No PYPLET_URL → falls back to Tornado's default same-origin check.""" + _clear_hardening_env(monkeypatch) + ws = _make_ws("localhost:8080") + assert ws.check_origin("http://localhost:8080") is True + + +def test_check_origin_foreign_rejected_without_allowlist(monkeypatch): + """No PYPLET_URL → cross-origin upgrade rejected (same-origin only).""" + _clear_hardening_env(monkeypatch) + ws = _make_ws("localhost:8080") + assert ws.check_origin("http://evil.example.com") is False + + +def test_check_origin_deployed_origin_allowed_when_host_rewritten(monkeypatch): + """PYPLET_URL host is allowed even if the edge rewrote the Host header.""" + _clear_hardening_env(monkeypatch) + monkeypatch.setenv("PYPLET_URL", "https://deployed.example.com") + ws = _make_ws("internal-upstream:8080") # edge rewrote Host + assert ws.check_origin("https://deployed.example.com") is True + + +def test_check_origin_foreign_rejected_with_allowlist(monkeypatch): + """A foreign origin is rejected even with a configured allowlist.""" + _clear_hardening_env(monkeypatch) + monkeypatch.setenv("PYPLET_URL", "https://deployed.example.com") + ws = _make_ws("internal-upstream:8080") + assert ws.check_origin("https://evil.example.com") is False + + +def test_check_origin_same_origin_allowed_with_allowlist(monkeypatch): + """Same-origin still allowed via Tornado's default even when an allowlist + is configured — the ``super().check_origin`` branch short-circuits before + the ``PYPLET_URL`` host comparison (AC3 case (a) with the allowlist on). + The origin matches the Host header but NOT the PYPLET_URL host, so only + the same-origin branch can grant it.""" + _clear_hardening_env(monkeypatch) + monkeypatch.setenv("PYPLET_URL", "https://deployed.example.com") + ws = _make_ws("localhost:8080") + assert ws.check_origin("http://localhost:8080") is True + + +# --------------------------------------------------------------------------- +# AC2 — xheaders=True on app.listen (astart path) +# --------------------------------------------------------------------------- + + +def test_astart_listens_with_xheaders(monkeypatch, tmp_path): + """``astart`` calls ``app.listen(..., xheaders=True)``. + + The real ``astart`` binds a port then blocks forever on + ``asyncio.Event().wait()``; here ``Application.listen`` is replaced by a + capture-and-stop double, the app-module glob targets an empty dir, and + the two startup policies are neutralized, so the coroutine reaches the + listen call and exits via the sentinel without standing up a server. + """ + captured = {} + + class _StopListen(Exception): + pass + + def _fake_listen(self, port, address, **kwargs): + captured["kwargs"] = kwargs + raise _StopListen + + # Empty apps dir → the server-module glob imports nothing. + monkeypatch.setattr(_server.config, "apps", str(tmp_path)) + # No app instances → no route splice into the shared _app_spec. + monkeypatch.setattr(_server, "server_applications", {}) + # Neutralize both fail-closed startup policies for this listen-path test. + monkeypatch.setattr( + _server.oauth, "enforce_startup_auth_policy", lambda **kw: None + ) + monkeypatch.setattr(_server, "enforce_startup_debug_policy", lambda: None) + monkeypatch.setattr( + _server.tornado.web.Application, "listen", _fake_listen + ) + + with pytest.raises(_StopListen): + asyncio.run(_server.astart()) + + assert captured["kwargs"].get("xheaders") is True From 292ffca41510a585029ad0edd33dc0ccc73bd59b Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Tue, 9 Jun 2026 11:05:05 +0200 Subject: [PATCH 13/28] docs(readme): document the production-profile guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The README described the fail-closed auth switch but none of the guards that came with it: the debug refusal, `xheaders`, the WebSocket origin check and the `id_token` JWKS verification. Also corrects two stale lines: the `PYPLET_DEBUG` default now carries its production caveat, and `PYPLET_WS_MAX_MESSAGE_MB` was missing from the CLI/env list. Documentation only. Carried in this branch for convenience — it depends on nothing here and can be dropped without affecting the code. --- README.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index ca9ab6b..a9bb079 100644 --- a/README.md +++ b/README.md @@ -258,6 +258,16 @@ when magic-link is enabled **without** `PYPLET_ALLOW_MAGICLINK=1`. Without the flag (the default), a deployment with no provider still starts but logs a loud WARNING that every request is served anonymously. +Three further production-profile guards ship with this posture. The server +**refuses to boot when `PYPLET_DEBUG=1` under `PYPLET_REQUIRE_AUTH=1`** — +Tornado debug mode enables autoreload and exposes traceback pages, so set +`PYPLET_DEBUG=0` in production. Behind a TLS-terminating reverse proxy, +`app.listen` trusts `X-Forwarded-For`/`X-Forwarded-Proto` (`xheaders`) and +WebSocket upgrades are origin-checked against the `PYPLET_URL` host +(same-origin when `PYPLET_URL` is unset). At login, OIDC `id_token`s are +verified against the provider JWKS (RS256 signature, issuer, audience and +expiry) before a session is established. + ### Configuration reference | Variable | Description | @@ -366,7 +376,7 @@ Available configuration options: - `--address` / `PYPLET_ADDR` - Server address (default: `127.0.0.1`) - `--port` / `PYPLET_PORT` - Server port (default: `8080`) - `--apps` / `PYPLET_APPS` - Apps directory (default: `apps`) -- `--debug` / `PYPLET_DEBUG` - Debug mode (default: `1`) +- `--debug` / `PYPLET_DEBUG` - Debug mode (default `1`; must be `0` in prod) - `--pyodide-url` / `PYPLET_PYODIDE` - Pyodide CDN URL - `--url` / `PYPLET_URL` - Custom URL override - `PYPLET_WS_MAX_MESSAGE_MB` - Max WebSocket frame size MB (default: `40`) From 50d24f1ed2e01332c67587279ff317d28f8e2044 Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Tue, 18 Aug 2026 19:01:47 +0200 Subject: [PATCH 14/28] fix(tests): boot the e2e server anonymously on a free port MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways the e2e suite tested the wrong thing: - it bound the default port 8080, routinely already held by another local dev server, so the whole suite silently exercised that server instead; - it inherited whatever OAuth / magic-link credentials an app in `apps/` loads from its own `.env` at import time, which flips `auth_enabled()` to True — every page then redirects to `/login` and the DOM assertions time out. The fixture now asks the OS for a free ephemeral port and blanks the auth env for the child (empty strings, not `pop`, so an app-level `load_dotenv(override=False)` cannot re-populate them). The blind `time.sleep(3)` is replaced by a connect loop with a 30 s deadline that fails fast if the child died during import. --- tests/conftest.py | 73 ++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 63 insertions(+), 10 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index f4d8eb4..a453015 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -5,6 +5,7 @@ import asyncio import multiprocessing import os +import socket import time import pytest @@ -15,23 +16,75 @@ from pyplet.server._server import astart from pyplet.server.config import config - -def run_server(): - """Run the Pyplet server in a separate process.""" +# The e2e suite must boot ANONYMOUSLY and deterministically. Any app in +# apps/ may load developer OAuth credentials from its own local .env at +# import time during server boot, flipping auth_enabled() to +# True — every app page then 302s to /login and the DOM assertions time out. +# Empty strings (not pop) are deliberate: the keys stay PRESENT in the +# environment, so an app-level load_dotenv(override=False) ("shell wins") +# cannot re-populate them from its .env, while the empty value stays falsy +# for oauth.enabled_providers() / magiclink.enabled(). +_ANON_AUTH_ENV = { + "PYPLET_REQUIRE_AUTH": "", + "PYPLET_ALLOW_MAGICLINK": "", + "OAUTH_GOOGLE_CLIENT_ID": "", + "OAUTH_GOOGLE_CLIENT_SECRET": "", + "OAUTH_MICROSOFT_CLIENT_ID": "", + "OAUTH_MICROSOFT_CLIENT_SECRET": "", + "MAGICLINK_SMTP_HOST": "", +} + + +def _free_port() -> int: + """Ask the OS for a free ephemeral port. + + The default port (8080) is routinely held by another dev server on + contributor machines — binding it makes the whole e2e suite + silently exercise the WRONG server (404s / stale pages), so the suite + must always run on its own fresh port. + """ + with socket.socket() as sock: + sock.bind((config.address, 0)) + return sock.getsockname()[1] + + +def run_server(port: int): + """Run the Pyplet server in a separate process (anonymous mode).""" + os.environ.update(_ANON_AUTH_ENV) + os.environ["PYPLET_PORT"] = str(port) asyncio.run(astart()) @pytest.fixture(scope="session") def server(): - """Start the Pyplet server for the test session.""" - # Start server in a separate process - server_process = multiprocessing.Process(target=run_server, daemon=True) + """Start the Pyplet server for the test session on a free port.""" + port = _free_port() + server_process = multiprocessing.Process( + target=run_server, args=(port,), daemon=True + ) server_process.start() - # Wait for server to be ready - time.sleep(3) - - yield f"http://{config.address}:{config.port}" + # Wait for the server to actually accept connections (fail fast if the + # child died, e.g. import error) instead of a blind sleep. + deadline = time.monotonic() + 30 + while True: + if not server_process.is_alive(): + raise RuntimeError( + "Pyplet test server process exited during startup" + ) + try: + with socket.create_connection((config.address, port), timeout=1): + break + except OSError: + if time.monotonic() > deadline: + server_process.terminate() + raise RuntimeError( + f"Pyplet test server not reachable on port {port} " + "after 30s" + ) + time.sleep(0.2) + + yield f"http://{config.address}:{port}" # Cleanup server_process.terminate() From 1162fd6142a1d7f2aae9a5e9914a2f77d2823b24 Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Tue, 26 May 2026 13:26:42 +0200 Subject: [PATCH 15/28] feat(server): routes() hook for app-declared Tornado handlers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An application had no way to serve an HTTP endpoint of its own: the handler table was fixed at import time in `_app_spec`, so anything app-specific had to be patched into the core. `ServerApplication.routes()` (default `[]`) lets an app return `(url_regex, HandlerClass, init_kwargs)` tuples. `_merge_app_declared_routes()` splices them into `_app_spec["handlers"]` just before the terminal catch-all `r"/.*"` redirect, which stays last — a route landing after it would be shadowed into a 302. The merge runs in `astart()` after the app modules are loaded (that is what populates `server_applications`) and before the Tornado `Application` is built: merging afterwards would leave the running server untouched. A failing `routes()` is logged and skipped, so one broken app cannot take the others down. Apps that do not override it get the empty default and the handler table is unchanged. Handlers declared this way do NOT go through `_AuthMixin` — an app that wants its route auth-gated subclasses the mixin explicitly. +1 test (`tests/server_routes_hook_test.py`): declares an app exposing `routes()`, asserts the route answers 200 (not a 302 from the catch-all) and that the catch-all is still the last entry. Checked against two mutations — splicing after the catch-all, and merging after `Application` — both turn it red. --- pyplet/server/_server.py | 77 ++++++++++++++++++++ tests/server_routes_hook_test.py | 117 +++++++++++++++++++++++++++++++ 2 files changed, 194 insertions(+) create mode 100644 tests/server_routes_hook_test.py diff --git a/pyplet/server/_server.py b/pyplet/server/_server.py index 65fcfca..4d0eab0 100644 --- a/pyplet/server/_server.py +++ b/pyplet/server/_server.py @@ -581,6 +581,47 @@ def enforce_startup_debug_policy() -> None: ) +def _merge_app_declared_routes() -> list[tuple]: + """Splice app-declared ``routes()`` into ``_app_spec["handlers"]``. + + Every registered application is asked for its ``routes()`` and the + result is inserted BEFORE the catch-all ``r"/.*"`` redirect, which is + the LAST entry of ``_app_spec["handlers"]`` (see the module-level + definition) — a route listed after it would be shadowed into a + redirect and never reached. Insertion is a ``[-1:-1]`` slice + assignment, so the catch-all stays last. + + Called from ``astart()`` once the app modules are loaded (that is what + populates ``server_applications``) and before the Tornado + ``Application`` is built from ``_app_spec`` — a merge after the + Application exists would have no effect on the running server. + + A failing ``routes()`` is logged and skipped so one broken app cannot + take the others down. + + Returns: + The handler tuples that were spliced in (empty list if none). + """ + app_declared_handlers: list[tuple] = [] + for instance in server_applications.values(): + try: + app_declared_handlers.extend(instance.routes()) + except Exception as e: + logger.error( + "Failed to read routes() from %s: %s", + type(instance).__name__, + e, + exc_info=True, + ) + if app_declared_handlers: + _app_spec["handlers"][-1:-1] = app_declared_handlers + logger.info( + "Registered %d app-declared route(s) before catch-all redirect", + len(app_declared_handlers), + ) + return app_declared_handlers + + async def astart(): # Load all server applications FIRST: importing each *_server.py # fires ServerApplication.__init_subclass__, which registers the @@ -605,6 +646,10 @@ async def astart(): # Tornado debug on (autoreload + traceback pages must never ship to prod). enforce_startup_debug_policy() + # Merge the routes each app declares into the handler table before the + # Tornado Application is built from _app_spec. + _merge_app_declared_routes() + favicon_uri = None if config.favicon: # Relative paths (e.g. the default "../images/...") are resolved @@ -1175,6 +1220,38 @@ def poly_issubclass(cls, class_or_tuple): ) handler.write_html(tree) + def routes(self) -> list[tuple[str, type, dict]]: + """Return additional Tornado handlers contributed by this app. + + Each tuple is ``(url_regex, RequestHandlerClass, init_kwargs)`` — + the same shape Tornado accepts in :class:`tornado.web.Application`'s + ``handlers`` argument. Pyplet merges these into the global handler + list at ``astart()`` time, inserted BEFORE the catch-all + ``r"/.*"`` redirect so the routes are reachable. Default returns + an empty list; apps that need custom HTTP routes (e.g. an + asset-serving ``/apps/<project>/<app>/assets/...`` endpoint) + override this. + + Tornado handler kwargs: + Handlers that need app-scoped state (paths, caches, etc.) + should accept those values via a Tornado ``initialize(...)`` + method and receive them as the third tuple element. + + Returns: + Empty list by default. Override in subclasses to declare + routes. + + Notes: + - Pyplet invokes ``routes()`` ONCE per app at server startup, + after the app instance has been registered via + ``__init_subclass__``. The returned list is not re-read on + subsequent requests. + - Handlers added via ``routes()`` do NOT go through the + ``_AuthMixin`` by default — auth-gated routes must explicitly + subclass ``_AuthMixin`` from this module. + """ + return [] + def __init_subclass__(cls): qualname = cls.__module__.split(".") if ( diff --git a/tests/server_routes_hook_test.py b/tests/server_routes_hook_test.py new file mode 100644 index 0000000..18e8968 --- /dev/null +++ b/tests/server_routes_hook_test.py @@ -0,0 +1,117 @@ +"""Structural test for the app-declared ``routes()`` hook. + +WHAT THIS PROVES +================ +``ServerApplication.routes()`` lets an app declare its own Tornado handlers. +The feature is worth exactly what the merge into ``_app_spec["handlers"]`` is +worth, and that merge is silent on failure: a route spliced AFTER the terminal +catch-all ``r"/.*"`` redirect, or merged after the Tornado ``Application`` was +built, leaves a server that boots, logs nothing and answers 302 on every +declared route. The rest of the suite cannot see it — nothing else exercises +``_merge_app_declared_routes()``. + +So this test declares an app exposing ``routes()`` and pins the two properties +that make the hook real: + + (a) **the declared route is reachable** — ``GET`` on it answers 200 from the + app's own handler, not a 302 from the catch-all; + (b) **the catch-all stays last** — the declared route precedes ``r"/.*"`` + in ``_app_spec["handlers"]`` and ``r"/.*"`` is still the final entry, so + unknown paths keep redirecting. + +The module-level ``_app_spec`` and ``server_applications`` are mutated, so both +are snapshotted in ``setUp`` and restored in ``tearDown``. +""" + +from tornado.testing import AsyncHTTPTestCase +from tornado.web import Application, RequestHandler + +from pyplet.server._server import ( + ServerApplication, + _app_spec, + _merge_app_declared_routes, + server_applications, +) + +_PROBE_PATTERN = r"/__routes_hook_probe__" +_PROBE_BODY = "declared-route-reached" + + +class _ProbeHandler(RequestHandler): + """Minimal handler an app would contribute through ``routes()``.""" + + def get(self): + self.write(_PROBE_BODY) + + +class _ProbeApp(ServerApplication): + """App declaring one custom route. + + Defined in a test module, so ``__init_subclass__`` does not register it + (registration only fires for ``_pyplet_apps.<project>.<app>_server`` + modules); the instance is put in ``server_applications`` explicitly. + """ + + def routes(self): + return [(_PROBE_PATTERN, _ProbeHandler, {})] + + +class AppDeclaredRouteTest(AsyncHTTPTestCase): + """The declared route is reachable and the catch-all stays last.""" + + def setUp(self): + self._saved_handlers = list(_app_spec["handlers"]) + self._saved_apps = dict(server_applications) + server_applications[("probe_project", "probe_app")] = _ProbeApp() + super().setUp() + + def tearDown(self): + super().tearDown() + _app_spec["handlers"][:] = self._saved_handlers + server_applications.clear() + server_applications.update(self._saved_apps) + + def get_app(self) -> Application: + # Same sequence as astart(): merge the app-declared routes into + # _app_spec FIRST, then build the Application from it. Merging after + # this point would not reach the running server. + _merge_app_declared_routes() + return Application(**_app_spec) + + def test_declared_route_reachable_and_catchall_last(self): + """Declared route answers 200; ``r"/.*"`` remains the last entry.""" + patterns = [entry[0] for entry in _app_spec["handlers"]] + + assert _PROBE_PATTERN in patterns, ( + "routes() was not merged into _app_spec['handlers'] — the hook " + "is dead" + ) + assert r"/.*" in patterns, "catch-all r'/.*' missing from _app_spec" + assert patterns.index(_PROBE_PATTERN) < patterns.index(r"/.*"), ( + "a declared route listed after the catch-all is shadowed into a " + "redirect" + ) + assert patterns[-1] == r"/.*", ( + "the catch-all r'/.*' must stay the last handler, or unknown " + "paths stop redirecting" + ) + + # follow_redirects=False so a regression surfaces as a 3xx code + # instead of being silently followed to the login/index page. + resp = self.fetch(_PROBE_PATTERN, follow_redirects=False) + assert resp.code == 200, ( + f"declared route should answer 200, got {resp.code} — the " + "catch-all or the auth gate took the request" + ) + assert resp.body.decode("utf-8") == _PROBE_BODY, ( + "the app's own handler did not answer" + ) + + # The catch-all still works for anything undeclared. + unknown = self.fetch( + "/__nothing_declares_this__", follow_redirects=False + ) + assert unknown.code in (301, 302), ( + f"unknown path should still hit the catch-all redirect, got " + f"{unknown.code}" + ) From e7bbaad510fa9be7d1b6b11f4d4dd7e997c7b8df Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Thu, 9 Jul 2026 15:09:25 +0200 Subject: [PATCH 16/28] feat(server): thread the WS-resolved login onto ServerWebSocket ``ServerWebSocket.open`` already resolved the caller's identity via ``_require_auth`` but discarded it before launching the app loop, so an app had no way to know who was on the socket. Capture it as ``self.login`` right after the resolution and before ``websocket_server_loop`` starts, so a consumer app can read it back with ``getattr(ws, "login", None)`` and thread it into its own session state. The auth-disabled sentinel's empty-string email is threaded verbatim: normalizing it to a stable anonymous literal is an app-side concern, not the framework's. +3 tests (`tests/server_login_test.py`): resolved user, anonymous sentinel, and unauthenticated close (no login set). --- pyplet/server/_server.py | 1 + tests/server_login_test.py | 103 +++++++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+) create mode 100644 tests/server_login_test.py diff --git a/pyplet/server/_server.py b/pyplet/server/_server.py index 4d0eab0..935a6ba 100644 --- a/pyplet/server/_server.py +++ b/pyplet/server/_server.py @@ -393,6 +393,7 @@ async def open(self, project_name, app_name): if user is None: self.close(1008, "Unauthorized") return + self.login = user["email"] application = server_applications[project_name, app_name] self.queue = asyncio.Queue() diff --git a/tests/server_login_test.py b/tests/server_login_test.py new file mode 100644 index 0000000..146d084 --- /dev/null +++ b/tests/server_login_test.py @@ -0,0 +1,103 @@ +"""Thread the resolved login into ``ServerWebSocket``. + +``ServerWebSocket.open`` already resolves the caller's identity via +``_require_auth`` but used to discard it. This pins the one line added there: +right after a successful ``_require_auth`` resolution, ``self.login`` is set +to the resolved user's ``email`` — before the app's ``websocket_server_loop`` +is launched — so a consumer app can read +it back via ``getattr(ws, "login", None)``. + +Mirrors ``tests/prod_hardening_test.py``'s ``_make_ws`` style: a bare +``ServerWebSocket.__new__`` instance with methods/attributes monkeypatched +directly, no real Tornado request/connection setup. ``open`` calls +``asyncio.create_task(application.websocket_server_loop(self))``, so each +test flushes the newly-created task (diffing ``asyncio.all_tasks()`` before +and after) so it does not leak as an "unawaited task" warning; asyncio is in +STRICT mode (pytest-asyncio 1.4.0), so every async test carries +``@pytest.mark.asyncio``. +""" + +import asyncio +from types import SimpleNamespace + +import pytest + +from pyplet.server import _server + + +def _make_ws() -> _server.ServerWebSocket: + """Build a bare ``ServerWebSocket`` with no real Tornado init.""" + return _server.ServerWebSocket.__new__(_server.ServerWebSocket) + + +async def _flush_new_tasks(before: set) -> None: + """Await any task ``open()`` scheduled via ``asyncio.create_task``.""" + after = asyncio.all_tasks() - before + if after: + await asyncio.gather(*after) + + +@pytest.mark.asyncio +async def test_open_captures_login_from_resolved_user(monkeypatch): + """A real resolved user dict lands on ``self.login`` as its ``email``.""" + ws = _make_ws() + resolved_user = { + "email": "alice@example.com", + "name": "Alice", + "provider": "google", + } + monkeypatch.setattr(ws, "_require_auth", lambda *a, **kw: resolved_user) + + async def _noop_loop(_ws): + return None + + fake_app = SimpleNamespace(websocket_server_loop=_noop_loop) + monkeypatch.setattr( + _server, "server_applications", {("proj", "app"): fake_app} + ) + + before = asyncio.all_tasks() + await ws.open("proj", "app") + await _flush_new_tasks(before) + + assert ws.login == "alice@example.com" + + +@pytest.mark.asyncio +async def test_open_captures_anonymous_sentinel_email_verbatim(monkeypatch): + """The auth-disabled sentinel's empty-string email is threaded as-is. + + Normalizing the empty-string sentinel to a stable anonymous literal is + an app-side concern — the shared framework must NOT special-case it here, + it just threads whatever ``_require_auth`` gave it. + """ + ws = _make_ws() + anonymous_user = {"email": "", "name": "anonymous", "provider": None} + monkeypatch.setattr(ws, "_require_auth", lambda *a, **kw: anonymous_user) + + async def _noop_loop(_ws): + return None + + fake_app = SimpleNamespace(websocket_server_loop=_noop_loop) + monkeypatch.setattr( + _server, "server_applications", {("proj", "app"): fake_app} + ) + + before = asyncio.all_tasks() + await ws.open("proj", "app") + await _flush_new_tasks(before) + + assert ws.login == "" + + +@pytest.mark.asyncio +async def test_open_unauthorized_never_sets_login(monkeypatch): + """When ``_require_auth`` rejects the request, ``self.login`` stays + unset.""" + ws = _make_ws() + monkeypatch.setattr(ws, "_require_auth", lambda *a, **kw: None) + monkeypatch.setattr(ws, "close", lambda *a, **kw: None) + + await ws.open("proj", "app") + + assert getattr(ws, "login", None) is None From a1a852871da572f863fae90d14df1f2b4d78ce70 Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Thu, 9 Jul 2026 15:58:15 +0200 Subject: [PATCH 17/28] fix(server): restore permessage-deflate get_compression_options override MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ServerWebSocket.get_compression_options override (compression_level 6) present on the qs-port branches (f083dda / b469ed4) was never carried onto chani_v3. Without it Tornado's base implementation returns None, which disables permessage-deflate, so the QualiSpectra story-5.7 regression guards fail (unit test_permessage_deflate_options.py + integration test_permessage_deflate_handshake.py): get_compression_options returns None instead of a dict and the WS handshake advertises no Sec-WebSocket-Extensions. Restores the exact canonical override so the handshake negotiates the extension again. Byte-identical to f083dda. Surfaced by the epic-13 story-13.1 verify gate (not a story-13.1 regression — a pre-existing gap on chani_v3). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --- pyplet/server/_server.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/pyplet/server/_server.py b/pyplet/server/_server.py index 935a6ba..f0d1aaf 100644 --- a/pyplet/server/_server.py +++ b/pyplet/server/_server.py @@ -388,6 +388,20 @@ def check_origin(self, origin: str) -> bool: == urllib.parse.urlparse(allowed).hostname ) + def get_compression_options(self): + """Enable WebSocket ``permessage-deflate`` compression. + + Returning a (possibly empty) dict opts the connection into Tornado's + per-message deflate extension; the client offers the extension and this + handler accepts it during the handshake. ``compression_level`` 6 is + zlib's default speed/ratio trade-off — a good fit for the app's + chatty JSON/text frames without excessive CPU per message. + + Returns: + A dict of compression options enabling ``permessage-deflate``. + """ + return {"compression_level": 6} + async def open(self, project_name, app_name): user = self._require_auth(project_name, app_name) if user is None: From b6d4cc62223773a546e3107dc9c0937817815328 Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Tue, 30 Jun 2026 11:08:11 +0200 Subject: [PATCH 18/28] feat(server,client): boot splash during the Pyodide bootstrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The window stayed blank for the whole PyScript/Pyodide startup, with no signal that anything was loading. Server side: `#container` now carries a self-contained spinner from the very first HTML response. No Tailwind classes (Tailwind loads later, client-side) and no external assets — inline styles plus one `<style>` block for the keyframes, so it renders before any stylesheet arrives, and it honours `prefers-reduced-motion`. Client side: `bootstrap_client` removes `#pyplet-boot-splash` by id once the app module has loaded, so an app that appends to `#container` (instead of replacing its contents, which drops the splash implicitly) does not leave it spinning. No automated test: the splash only exists under a real Pyodide boot, which `make test` does not exercise. Verified in-browser. --- pyplet/client/__init__.py | 9 +++++++++ pyplet/server/_server.py | 25 ++++++++++++++++++++++++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/pyplet/client/__init__.py b/pyplet/client/__init__.py index 72e97cd..826e9b2 100644 --- a/pyplet/client/__init__.py +++ b/pyplet/client/__init__.py @@ -501,6 +501,15 @@ async def bootstrap_client(prefix, project_name, app_name, deps=()): ): await client_application.client_init() + # Drop the boot splash (server-rendered into #container) now that the app + # module has loaded and any client_init UI has mounted. Apps that replace + # #container's contents already removed it; this by-id removal also covers + # apps that append to #container, so the spinner never lingers. Gate on + # truthiness — getElementById yields a falsy JS null when already gone. + splash = js.document.getElementById("pyplet-boot-splash") + if splash: + splash.remove() + if ( client_application.__class__.websocket_client_loop is not ClientApplication.websocket_client_loop diff --git a/pyplet/server/_server.py b/pyplet/server/_server.py index f0d1aaf..b312771 100644 --- a/pyplet/server/_server.py +++ b/pyplet/server/_server.py @@ -1218,10 +1218,33 @@ def poly_issubclass(cls, class_or_tuple): ) } + # Boot splash: a self-contained spinner shown inside #container from + # the very first HTML response, covering the blank window while + # PyScript/Pyodide and the transpiled app load. It carries no Tailwind + # classes (Tailwind loads later, client-side) and no external assets — + # only inline styles plus one <style> block for the rotation keyframes. + # Apps that replace #container's contents (rather than appending to + # them) drop it implicitly; bootstrap_client also removes + # #pyplet-boot-splash by id once the app module has loaded, so apps + # that append to #container don't leave it spinning. A fixed, + # viewport-centered wrapper avoids any navbar-height assumption. + boot_splash = markupsafe.Markup( # nosec + '<div id="pyplet-boot-splash" style="position:fixed;inset:0;' + 'display:flex;align-items:center;justify-content:center">' + "<style>@keyframes pyplet-spin{to{transform:rotate(360deg)}}" + "@media (prefers-reduced-motion:reduce)" + "{#pyplet-boot-splash .pyplet-spinner{animation:none}}</style>" + '<div class="pyplet-spinner" style="width:40px;height:40px;' + "border:4px solid #dee2e6;border-top-color:#6c757d;" + 'border-radius:50%;animation:pyplet-spin 0.8s linear infinite">' + "</div>" + "</div>" + ) + content = { "head": head_content, "body": [ - div(id="container"), + div(id="container")[boot_splash], markupsafe.Markup( # nosec f"<script type='{script_tag}' " f"config='{json.dumps(py_config)}'" From 54f53386dd5fd94db77ecb6761ea29e8f73bf3a8 Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Mon, 13 Jul 2026 16:01:04 +0200 Subject: [PATCH 19/28] fix(client): self-heal a missing micropip at boot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A reload after the app has been used can restore a partial Pyodide package set from PyScript's IndexedDB cache (`@pyscript.fs` plus the Pyodide package cache), leaving micropip unregistered — `import micropip` then raises `ModuleNotFoundError` and the app never boots. The only known workaround was to clear IndexedDB by hand. Recover exactly as the error message advises: load the package through the Pyodide JS API and retry the import. A cold boot never enters the handler, so the normal path is unchanged. No automated test: reproducing it needs a real Pyodide runtime with a dirtied IndexedDB, which `make test` does not exercise. Verified in-browser. --- pyplet/client/__init__.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/pyplet/client/__init__.py b/pyplet/client/__init__.py index 826e9b2..cc56bea 100644 --- a/pyplet/client/__init__.py +++ b/pyplet/client/__init__.py @@ -460,7 +460,21 @@ async def bootstrap_client(prefix, project_name, app_name, deps=()): for dep in deps: mip.install(dep) else: - import micropip + try: + import micropip + except ModuleNotFoundError: + # micropip ships with Pyodide, but a reload after the app has + # already been used can restore a partial/inconsistent Pyodide + # package set from PyScript's IndexedDB cache (@pyscript.fs + + # the pyodide package cache), leaving micropip unregistered so + # `import micropip` raises ModuleNotFoundError at boot. Recover + # exactly as the error message itself advises: pull the package + # in via the Pyodide JS API and retry. This self-heals a stale + # or dirty cache without forcing the user to clear IndexedDB. + import pyodide_js + + await pyodide_js.loadPackage("micropip") + import micropip await micropip.install(list(deps)) From 19ab4827ef1f6728fd260b737d1f7d9f0a3bdc61 Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Tue, 25 Aug 2026 12:27:05 +0200 Subject: [PATCH 20/28] oauth: replace Google/Drive hardcoding with a provider registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The OIDC engine carried one application's requirements: a `_PROVIDERS` dict with Google and Microsoft endpoints inline, `register_drive_token_hook()`, `start_drive_consent()` with `drive.file` + `access_type=offline` frozen in the source, and a `state["flow"] == "drive"` branch in the callback. An app needing a different provider or a different extra scope had to patch the framework. Two generic entry points replace them: register_provider(name, spec) - an OIDC provider (label, discovery URL, client id/secret, scopes, auth params). Values may be zero-arg callables, so a spec reads config lazily instead of freezing an env var at import. register_consent_flow(name, flow) - an incremental-consent flow, routed by `state["flow"]`, with an `on_complete` hook receiving the raw token response. The engine itself is untouched: discovery, state cookie, code exchange, JWKS verification, signed session cookie and the fail-closed startup policy all keep their behaviour. It now branches on no provider name. Google and Microsoft move out to `pyplet/server/oauth_providers.py` as shipped presets, registered at import; a re-registration replaces, which is how an app overrides one. `config.py` keeps `oauth_google_client_id` and friends — they are the presets' configuration and are read by the CLI and the login template. Validation moves to registration time: a spec missing a required key raises ValueError naming the app, rather than surfacing as a broken production login. A state cookie naming an unregistered flow is refused with a 400 instead of falling through into an unrequested login. Proven equivalent, not assumed: the migrated `drive` flow builds an authorize request byte-for-byte identical to the one `start_drive_consent()` produced (same endpoint, same seven parameters, same joined scope string) - checked by running both implementations side by side. Tests: 230 -> 250 (20 new in tests/oauth_registry_test.py; none removed). The migration for the one consumer of the Drive hooks (apps/chani, a separate repo) is documented in docs/api/oauth-providers.md and NOT applied here. --- docs/api/oauth-providers.md | 121 +++++++++ pyplet/server/oauth.py | 382 ++++++++++++++++++-------- pyplet/server/oauth_providers.py | 57 ++++ pyplet/server/templates.py | 7 +- tests/oauth_cookie_security_test.py | 37 ++- tests/oauth_registry_test.py | 397 ++++++++++++++++++++++++++++ zensical.toml | 1 + 7 files changed, 887 insertions(+), 115 deletions(-) create mode 100644 docs/api/oauth-providers.md create mode 100644 pyplet/server/oauth_providers.py create mode 100644 tests/oauth_registry_test.py diff --git a/docs/api/oauth-providers.md b/docs/api/oauth-providers.md new file mode 100644 index 0000000..0298da7 --- /dev/null +++ b/docs/api/oauth-providers.md @@ -0,0 +1,121 @@ +# OAuth providers and consent flows + +`pyplet.server.oauth` is a provider-agnostic OIDC engine. It owns discovery, +the CSRF state cookie, the code exchange, JWKS signature verification, the +signed session cookie and the fail-closed startup policy — and it branches on +no provider name and hardcodes no vendor endpoint. + +Everything vendor-specific lives in one of two places: + +- **`pyplet.server.oauth_providers`** — the presets Pyplet ships (Google, + Microsoft/Entra ID), registered into the engine at import. +- **Your application** — anything else, registered from your own module at + import time. + +## Registering a provider + +```python +from pyplet.server import oauth + +oauth.register_provider("corporate", { + "label": "Corporate SSO", # login-button text + "openid_config_url": "https://id.corp.example/.well-known/openid-configuration", + "client_id": lambda: os.environ.get("CORP_CLIENT_ID", ""), + "client_secret": lambda: os.environ.get("CORP_CLIENT_SECRET", ""), + "scopes": ["openid", "email", "profile"], + "auth_params": {"prompt": "select_account"}, # optional +}) +``` + +`openid_config_url`, `client_id` and `client_secret` are required; a spec +missing one raises `ValueError` **at registration**, so the traceback names the +app instead of surfacing as a broken login in production. + +Any value may be a zero-argument callable, resolved at use time. That is how a +spec reads configuration lazily rather than freezing an env var at import. + +A provider only appears on the login page once its `client_id` resolves to +something non-empty — registration is not configuration. Registering an +existing name **replaces** it, which is how an app overrides a shipped preset +(apps are loaded before the Tornado app is built, so an app-side registration +always wins). + +## Incremental consent + +An incremental-consent flow re-runs the authorization-code round-trip on top of +an existing login to obtain **extra scopes** — and, if it asks for offline +access, a refresh token — without touching the session. The user stays logged +in throughout. + +```python +async def _store_token(handler, user_info, tokens): + """Called once the callback has verified the id_token.""" + await save_refresh_token( + user_info["sub"], tokens.get("refresh_token"), tokens.get("scope", "") + ) + +oauth.register_consent_flow("files", { + "provider": "corporate", + "scopes": ["https://api.corp.example/auth/files"], + "auth_params": {"access_type": "offline", "prompt": "consent"}, + "on_complete": _store_token, +}) +``` + +Start it from a Tornado handler: + +```python +await oauth.start_consent(handler, "files", next_url="/apps/me/back-here") +``` + +`start_consent` stores `"flow": "files"` in the state cookie; `handle_callback` +reads it back and hands the **raw token response** to `on_complete` instead of +calling `set_session`. The engine does not interpret `refresh_token` or +`scope` — what the extra scopes are for is entirely the application's business. + +Two deliberate behaviours: + +- An exception from `on_complete` is logged and swallowed, then the browser is + redirected anyway. It is mid-redirect from the provider; a bookkeeping + failure must not strand it on an error page. +- A state cookie naming an **unregistered** flow is refused with a 400. Falling + through would turn a consent round-trip into an unrequested login. + +## Migrating off the Drive-specific API + +The engine previously carried a Google-Drive-shaped path: `register_drive_token_hook()`, +`start_drive_consent()`, and a `state["flow"] == "drive"` branch in the +callback, with `drive.file`/`access_type=offline` hardcoded. That was one +application's requirement living in the framework. It is replaced by the +generic pair above. + +| Removed | Replacement | +| --- | --- | +| `register_drive_token_hook(fn)` | `register_consent_flow(name, {...,"on_complete": fn})` | +| `start_drive_consent(handler, next_url)` | `start_consent(handler, name, next_url)` | +| hardcoded `state["flow"] == "drive"` | any registered flow name | +| hardcoded `drive.file` scope + `access_type=offline` | the flow's `scopes` / `auth_params` | + +The hook signature changes from `(sub, email, refresh_token, scopes)` to +`(handler, user_info, tokens)`. Adapt an existing hook in place: + +```python +oauth.register_consent_flow("drive", { + "provider": "google", + "scopes": ["https://www.googleapis.com/auth/drive.file"], + "auth_params": { + "access_type": "offline", + "prompt": "consent", + "include_granted_scopes": "true", + }, + "on_complete": lambda handler, user_info, tokens: existing_hook( + user_info["sub"], + user_info["email"], + tokens.get("refresh_token"), + tokens.get("scope", ""), + ), +}) +``` + +The authorization request this produces is byte-for-byte the one +`start_drive_consent` used to build. diff --git a/pyplet/server/oauth.py b/pyplet/server/oauth.py index cc3c8f0..e800e4e 100644 --- a/pyplet/server/oauth.py +++ b/pyplet/server/oauth.py @@ -8,16 +8,23 @@ goes through an auth check. Unauthenticated users are shown a login page; after a successful OAuth flow a signed session cookie is issued. -Supported providers +Providers +--------- +This module is the provider-agnostic OIDC **engine**: it branches on no +provider name and hardcodes no vendor endpoint. Providers live in an open +registry, filled by :func:`register_provider`; the presets Pyplet ships (and +the env vars that configure them) live in :mod:`pyplet.server.oauth_providers` +and are registered at the bottom of this module. An application adds — or +overrides — a provider by calling :func:`register_provider` at import time. + +Incremental consent ------------------- -Google - OAUTH_GOOGLE_CLIENT_ID - OAUTH_GOOGLE_CLIENT_SECRET - -Microsoft / Entra ID - OAUTH_MICROSOFT_CLIENT_ID - OAUTH_MICROSOFT_CLIENT_SECRET - OAUTH_MICROSOFT_TENANT (default: "common") +A flow registered with :func:`register_consent_flow` and started with +:func:`start_consent` re-runs the authorization-code round-trip on top of an +existing login to obtain **extra scopes** (and, when it asks for offline +access, a refresh token) without touching the session. The engine routes the +callback back to the flow by the ``"flow"`` field of the state cookie, so what +those scopes are for is entirely the application's business. Always required when auth is enabled ------------------------------------- @@ -63,12 +70,14 @@ import re import secrets import time +from collections.abc import Mapping from typing import Any from urllib.parse import urlencode, urljoin import httpx from .config import config +from .oauth_providers import BUILTIN_PROVIDERS logger = logging.getLogger("pyplet.server.oauth") @@ -76,38 +85,95 @@ # Provider registry # --------------------------------------------------------------------------- -_GOOGLE_OPENID_CONFIG_URL = ( - "https://accounts.google.com/.well-known/openid-configuration" -) -_MICROSOFT_OPENID_CONFIG_URL = ( - "https://login.microsoftonline.com/" - "{tenant}/v2.0/.well-known/openid-configuration" -) - -_PROVIDER_CONFIGS: dict[str, dict[str, Any]] = { - "google": { - "label": "Google", - "openid_config_url": _GOOGLE_OPENID_CONFIG_URL, - "client_id": lambda: config.oauth_google_client_id, - "client_secret": lambda: config.oauth_google_client_secret, - "scopes": ["openid", "email", "profile"], - }, - "microsoft": { - "label": "Microsoft", - "openid_config_url": lambda: _MICROSOFT_OPENID_CONFIG_URL.format( - tenant=config.oauth_microsoft_tenant - ), - "client_id": lambda: config.oauth_microsoft_client_id, - "client_secret": lambda: config.oauth_microsoft_client_secret, - "scopes": ["openid", "email", "profile"], - }, -} +# Registered OIDC providers, keyed by name. Deliberately EMPTY here: the engine +# below ships no provider of its own and never branches on a provider name. +# Pyplet's own presets are registered at the bottom of this module, from the +# ``oauth_providers`` catalog, through the same public entry point an +# application uses. +_PROVIDER_CONFIGS: dict[str, dict[str, Any]] = {} + +# Spec keys without which a login cannot even be attempted. Checked at +# registration so a malformed spec fails at import — where the traceback names +# the offending app — instead of at the first login attempt in production. +_REQUIRED_PROVIDER_KEYS = ("openid_config_url", "client_id", "client_secret") + +# Applied when a spec omits them. ``select_account`` is plain OIDC (it stops a +# provider silently re-using its own ambient session), not vendor knowledge. +_DEFAULT_SCOPES = ("openid", "email", "profile") +_DEFAULT_AUTH_PARAMS = {"prompt": "select_account"} + + +def register_provider(name: str, spec: Mapping[str, Any]) -> None: + """Register the OIDC provider *name*, replacing any spec already there. + + Args: + name: Registry key. Also the ``?provider=`` value accepted by + ``/oauth/login`` and the ``"provider"`` field recorded in the + session cookie. + spec: The provider description. Required keys: + + ``openid_config_url`` + URL of the OIDC discovery document. + ``client_id`` / ``client_secret`` + The OAuth2 client credentials. + + Optional keys: + + ``label`` + Human-readable name for the login button (default: + ``name.title()``). + ``scopes`` + Scopes requested at login (default: openid/email/profile). + ``auth_params`` + Extra authorization-endpoint query params, replacing the + engine default ``{"prompt": "select_account"}``. + + Any value may be a zero-argument callable, which the engine + resolves at use time — that is how a spec reads ``config`` (hence + the environment) lazily rather than freezing it at import. + + Raises: + ValueError: When a required key is missing. + """ + missing = [key for key in _REQUIRED_PROVIDER_KEYS if key not in spec] + if missing: + raise ValueError( + f"OAuth provider {name!r} is missing required spec key(s): " + f"{', '.join(missing)}" + ) + if name in _PROVIDER_CONFIGS: + logger.debug("Replacing the registered OAuth provider %r", name) + _PROVIDER_CONFIGS[name] = dict(spec) + + +def _provider(name: str) -> dict[str, Any]: + """Return the spec registered under *name*. + + Raises: + KeyError: When nothing is registered under that name — with the + registered names in the message, since an unknown provider is + almost always a missing ``register_provider()`` call at boot. + """ + try: + return _PROVIDER_CONFIGS[name] + except KeyError: + raise KeyError( + f"No OAuth provider registered under {name!r} " + f"(registered: {sorted(_PROVIDER_CONFIGS) or 'none'})" + ) from None + + +def _spec_value(spec: Mapping[str, Any], key: str, default: Any = None) -> Any: + """Read *key* from a provider/flow spec, calling it when it is callable.""" + value = spec.get(key, default) + return value() if callable(value) else value + # OIDC discovery-doc cache _oidc_cache: dict[str, dict] = {} -# JWKS cache, keyed by provider (parallel to ``_oidc_cache``). The provider's -# signing keys rotate (Google rotates roughly daily), so a verify failure that +# JWKS cache, keyed by provider (parallel to ``_oidc_cache``). Providers rotate +# their signing keys (major ones roughly daily), so a verify failure that # looks like rotation triggers a single forced refetch — see # ``_verify_id_token_claims``. _jwks_cache: dict[str, dict] = {} @@ -116,10 +182,22 @@ def enabled_providers() -> list[str]: """Return the names of providers that have a client_id configured.""" return [ - name for name, meta in _PROVIDER_CONFIGS.items() if meta["client_id"]() + name + for name, meta in _PROVIDER_CONFIGS.items() + if _spec_value(meta, "client_id") ] +def provider_label(name: str) -> str: + """Human-readable name for *name*, for the login button. + + Falls back to a title-cased name, so a provider registered by an app + renders sensibly without declaring a label. + """ + spec = _PROVIDER_CONFIGS.get(name, {}) + return _spec_value(spec, "label") or name.title() + + # Extra auth-enabled checks registered by other modules (e.g. magiclink) # to avoid circular imports. Call register_auth_check() at import time. _extra_auth_checks: list = [] @@ -131,20 +209,57 @@ def register_auth_check(fn) -> None: _extra_auth_checks.append(fn) -# Callbacks registered by a server application to handle Drive-consent -# tokens -# (Story 14.8 / CAP-8). Called after a drive-flow OAuth callback completes. -_drive_token_hooks: list = [] +# Incremental-consent flows registered by a server application, keyed by name. +# A flow re-runs the authorization-code round-trip on top of an existing login +# to obtain extra scopes without touching the session; ``handle_callback`` +# routes the response to the flow's completion callback — instead of +# ``set_session`` — keyed by the ``"flow"`` field of the state cookie. +_CONSENT_FLOWS: dict[str, dict[str, Any]] = {} +_REQUIRED_FLOW_KEYS = ("provider", "on_complete") -def register_drive_token_hook(fn) -> None: - """Register an async callback called after a drive-consent OAuth callback. - Called with (sub: str, email: str, refresh_token: str | None, scopes: str). - Registered hooks are called in order once the Drive token exchange - succeeds. +def register_consent_flow(name: str, flow: Mapping[str, Any]) -> None: + """Register the incremental-consent flow *name*, replacing any existing. + + Args: + name: Flow key — stored in the state cookie's ``"flow"`` field and + passed to :func:`start_consent`. + flow: The flow description. Required keys: + + ``provider`` + Name of a registered provider to run the flow against. + ``on_complete`` + ``async (handler, user_info, tokens) -> None``, awaited once + the callback has verified the id_token. It receives the raw + token response, so a flow that asked for offline access reads + its own ``refresh_token``/``scope`` out of it — the engine + does not interpret those. Exceptions are logged and swallowed: + a failing flow must not strand the browser mid-redirect. + + Optional keys: + + ``scopes`` + Extra scopes, appended to the provider's login scopes. + ``auth_params`` + Extra authorization-endpoint params, merged over the + provider's (e.g. whatever the provider wants in order to + return a refresh token). + + As for a provider spec, any value may be a zero-argument callable. + + Raises: + ValueError: When a required key is missing. """ - _drive_token_hooks.append(fn) + missing = [key for key in _REQUIRED_FLOW_KEYS if key not in flow] + if missing: + raise ValueError( + f"OAuth consent flow {name!r} is missing required spec key(s): " + f"{', '.join(missing)}" + ) + if name in _CONSENT_FLOWS: + logger.debug("Replacing the registered consent flow %r", name) + _CONSENT_FLOWS[name] = dict(flow) def auth_enabled() -> bool: @@ -163,10 +278,7 @@ async def _fetch_oidc_config(provider: str) -> dict: if provider in _oidc_cache: return _oidc_cache[provider] - meta = _PROVIDER_CONFIGS[provider] - url = meta["openid_config_url"] - if callable(url): - url = url() + url = _spec_value(_provider(provider), "openid_config_url") async with httpx.AsyncClient() as client: resp = await client.get(url, timeout=10) @@ -184,7 +296,7 @@ async def _fetch_jwks(provider: str, *, force_refresh: bool = False) -> dict: Reads ``jwks_uri`` from the already-cached OIDC discovery document and GETs it with the same ``httpx`` pattern as :func:`_fetch_oidc_config`. The result is cached per provider; ``force_refresh=True`` bypasses the - cache to pick up a rotated signing key (Google rotates its JWKS keys). + cache to pick up a rotated signing key. """ if not force_refresh and provider in _jwks_cache: return _jwks_cache[provider] @@ -210,10 +322,10 @@ async def _fetch_jwks(provider: str, *, force_refresh: bool = False) -> dict: def _accepted_issuers(issuer: str) -> list[str]: """Return the set of acceptable ``iss`` values for *issuer*. - Google issues ``iss`` as either the ``https://accounts.google.com`` form or - the bare ``accounts.google.com`` host; the discovery document's ``issuer`` - is the ``https://`` form, so accept both. Microsoft's ``iss`` matches its - discovery ``issuer`` exactly, so for it this is effectively a singleton. + Some providers issue ``iss`` as the bare host while their discovery + document advertises the ``https://`` form, so accept both spellings of the + discovered issuer. For a provider whose ``iss`` matches its discovery + ``issuer`` exactly this is effectively a singleton. """ out = [issuer] if issuer.startswith("https://"): @@ -228,15 +340,16 @@ def _verify_id_token_against_jwks( Pure and synchronous (no ``httpx``, no ``config``) so the crypto is unit-testable with crafted JWTs and a stubbed JWKS — no async, no network. - Restricts the accepted signing algorithm to RS256 (Google and Microsoft - both sign with RS256) to close algorithm-confusion, resolves the signing - key from *jwks* by ``kid``, and validates ``iss``/``aud``/``exp``. + Restricts the accepted signing algorithm to RS256 (the algorithm every + provider Pyplet ships a preset for signs with) to close + algorithm-confusion, resolves the signing key from *jwks* by ``kid``, and + validates ``iss``/``aud``/``exp``. Args: id_token: The compact-serialized JWT from the token endpoint. jwks: The provider JWKS (``{"keys": [...]}`` from ``jwks_uri``). issuer: The discovery ``issuer`` — the expected ``iss`` (the bare-host - Google variant is also accepted, see :func:`_accepted_issuers`). + variant is also accepted, see :func:`_accepted_issuers`). audience: The provider ``client_id`` — the expected ``aud``. Returns: @@ -271,7 +384,7 @@ async def _verify_id_token_claims(id_token: str, provider: str) -> dict: (the provider ``client_id``), fetches the JWKS, and verifies. On a signature/key-resolution failure that looks like key rotation (a ``BadSignatureError`` or an unknown-``kid`` ``ValueError``) it refetches - the JWKS **once** and retries — Google rotates its signing keys, so a + the JWKS **once** and retries — providers rotate their signing keys, so a freshly rotated key may post-date the cache. Claim failures (wrong or expired ``iss``/``aud``/``exp``) are NOT retried (the key already resolved; a refetch cannot help) and propagate immediately. @@ -284,7 +397,7 @@ async def _verify_id_token_claims(id_token: str, provider: str) -> dict: oidc = await _fetch_oidc_config(provider) issuer = oidc["issuer"] - audience = _PROVIDER_CONFIGS[provider]["client_id"]() + audience = _spec_value(_provider(provider), "client_id") jwks = await _fetch_jwks(provider) try: @@ -515,8 +628,9 @@ def enforce_startup_auth_policy(magiclink_enabled: bool = False) -> None: raise AuthConfigError( "PYPLET_REQUIRE_AUTH=1 but no authentication method is " "configured — refusing to boot (would serve every app " - "anonymously). Configure a provider (e.g. " - "OAUTH_GOOGLE_CLIENT_ID/SECRET), or unset PYPLET_REQUIRE_AUTH " + "anonymously). Configure an OAuth provider (see " + "pyplet.server.oauth_providers for the shipped presets and " + "their env vars) or magic-link, or unset PYPLET_REQUIRE_AUTH " "for an explicitly open deployment." ) logger.warning( @@ -574,10 +688,10 @@ async def start_login(handler, provider: str) -> None: Saves a CSRF state token in a cookie, then redirects the browser to the provider's authorization endpoint. """ - meta = _PROVIDER_CONFIGS[provider] + meta = _provider(provider) oidc = await _fetch_oidc_config(provider) - client_id = meta["client_id"]() + client_id = _spec_value(meta, "client_id") state = secrets.token_urlsafe(16) next_url = handler.get_argument("next", "/") @@ -594,9 +708,9 @@ async def start_login(handler, provider: str) -> None: "client_id": client_id, "redirect_uri": callback_url, "response_type": "code", - "scope": " ".join(meta["scopes"]), + "scope": " ".join(_provider_scopes(meta)), "state": state, - "prompt": "select_account", + **_provider_auth_params(meta), } auth_url = oidc["authorization_endpoint"] + "?" + urlencode(params) @@ -604,26 +718,33 @@ async def start_login(handler, provider: str) -> None: handler.redirect(auth_url) -async def start_drive_consent(handler, next_url: str = "/") -> None: - """Initiate an incremental OAuth authorization to add drive.file scope. +async def start_consent(handler, name: str, next_url: str = "/") -> None: + """Initiate the registered incremental-consent flow *name*. - Uses the same unified Web-application client as login (Q3). Adds - access_type=offline and prompt=consent to force a refresh_token response. - Stores "flow": "drive" in the state cookie so handle_callback routes - the response to the Drive token hook instead of set_session. + Re-runs the authorization-code flow against the flow's provider, using the + same OAuth client as login, asking for the provider's login scopes PLUS the + flow's extra scopes. Stores ``"flow": name`` in the state cookie so + :func:`handle_callback` hands the tokens to the flow's ``on_complete`` + instead of calling :func:`set_session`. - Does not modify the login session (user stays logged in throughout). + Does not modify the login session (the user stays logged in throughout). Args: handler: Tornado RequestHandler. - next_url: URL to redirect to after Drive consent completes. + name: A flow registered with :func:`register_consent_flow`. + next_url: URL to redirect to once consent completes. Returns: None (redirects the browser). + + Raises: + KeyError: When no such flow (or its provider) is registered. """ - meta = _PROVIDER_CONFIGS["google"] - oidc = await _fetch_oidc_config("google") - client_id = meta["client_id"]() + flow = _consent_flow(name) + provider = flow["provider"] + meta = _provider(provider) + oidc = await _fetch_oidc_config(provider) + client_id = _spec_value(meta, "client_id") state = secrets.token_urlsafe(16) handler.set_signed_cookie( @@ -632,8 +753,8 @@ async def start_drive_consent(handler, next_url: str = "/") -> None: { "state": state, "next": next_url, - "provider": "google", - "flow": "drive", + "provider": provider, + "flow": name, } ), httponly=True, @@ -642,19 +763,20 @@ async def start_drive_consent(handler, next_url: str = "/") -> None: ) callback_url = _callback_url(handler) + scopes = _provider_scopes(meta) + list( + _spec_value(flow, "scopes", ()) or () + ) params = { "client_id": client_id, "redirect_uri": callback_url, "response_type": "code", - "scope": " ".join( - meta["scopes"] + ["https://www.googleapis.com/auth/drive.file"] - ), + "scope": " ".join(scopes), "state": state, - "access_type": "offline", - "prompt": "consent", - "include_granted_scopes": "true", + **_provider_auth_params(meta), + **(_spec_value(flow, "auth_params", {}) or {}), } auth_url = oidc["authorization_endpoint"] + "?" + urlencode(params) + logger.debug("Redirecting to %s consent flow %r", provider, name) handler.redirect(auth_url) @@ -700,7 +822,7 @@ async def handle_callback(handler) -> None: return # --- Exchange code for tokens --- - meta = _PROVIDER_CONFIGS[provider] + meta = _provider(provider) oidc = await _fetch_oidc_config(provider) callback_url = _callback_url(handler) @@ -708,8 +830,8 @@ async def handle_callback(handler) -> None: "grant_type": "authorization_code", "code": code, "redirect_uri": callback_url, - "client_id": meta["client_id"](), - "client_secret": meta["client_secret"](), + "client_id": _spec_value(meta, "client_id"), + "client_secret": _spec_value(meta, "client_secret"), } try: @@ -748,21 +870,25 @@ async def handle_callback(handler) -> None: _error(handler, 500, "Provider did not include an email in the token.") return - # Story 14.8 / CAP-8: Drive incremental consent callback — do NOT call - # set_session (the user is already logged in). Call drive token hooks - # to store the refresh token, then redirect back to the app. - if state_data.get("flow") == "drive": - refresh_token = tokens.get( - "refresh_token" - ) # None if Google omitted it - scopes = tokens.get("scope", "") - for hook in _drive_token_hooks: - try: - await hook( - user_info["sub"], user_info["email"], refresh_token, scopes - ) - except Exception as exc: - logger.error("Drive token hook failed: %s", exc) + # Incremental-consent callback — do NOT call set_session (the user is + # already logged in). Hand the raw token response to the flow that started + # this round-trip, then redirect back to the app. A flow that raises is + # logged and swallowed: the browser is mid-redirect and must not be + # stranded on an error page by a bookkeeping failure. + flow_name = state_data.get("flow") + if flow_name: + flow = _CONSENT_FLOWS.get(flow_name) + if flow is None: + _error( + handler, + 400, + f"Unknown consent flow: {flow_name!r}. Please try again.", + ) + return + try: + await flow["on_complete"](handler, user_info, tokens) + except Exception as exc: + logger.error("Consent flow %r failed: %s", flow_name, exc) handler.clear_cookie(_STATE_COOKIE) handler.redirect(next_url) return @@ -783,6 +909,31 @@ async def handle_callback(handler) -> None: # --------------------------------------------------------------------------- +def _consent_flow(name: str) -> dict[str, Any]: + """Return the consent flow registered under *name*. + + Raises: + KeyError: When nothing is registered under that name. + """ + try: + return _CONSENT_FLOWS[name] + except KeyError: + raise KeyError( + f"No OAuth consent flow registered under {name!r} " + f"(registered: {sorted(_CONSENT_FLOWS) or 'none'})" + ) from None + + +def _provider_scopes(meta: Mapping[str, Any]) -> list[str]: + """Login scopes for a provider spec, defaulting to plain OIDC.""" + return list(_spec_value(meta, "scopes") or _DEFAULT_SCOPES) + + +def _provider_auth_params(meta: Mapping[str, Any]) -> dict[str, str]: + """Extra authorization-endpoint params for a provider spec.""" + return dict(_spec_value(meta, "auth_params") or _DEFAULT_AUTH_PARAMS) + + def _callback_url(handler) -> str: base = config.url or f"{handler.request.protocol}://{handler.request.host}" return urljoin(base, "/oauth/callback") @@ -796,3 +947,22 @@ def _error(handler, status: int, message: str) -> None: f'<p><a href="/">Back to home</a></p>' f"</body></html>" ) + + +# --------------------------------------------------------------------------- +# Built-in providers +# --------------------------------------------------------------------------- +# Registered at import, generically, from the shipped catalog — nothing above +# names a provider. An env-only deployment therefore keeps its login page with +# no application code, while an app is free to override any of these (or add +# its own) by calling register_provider() at its own import time, which runs +# later: apps are loaded before the Tornado app is built. + + +def _register_builtin_providers() -> None: + """Fill the registry from :mod:`pyplet.server.oauth_providers`.""" + for name, spec in BUILTIN_PROVIDERS.items(): + register_provider(name, spec) + + +_register_builtin_providers() diff --git a/pyplet/server/oauth_providers.py b/pyplet/server/oauth_providers.py new file mode 100644 index 0000000..5cfcf06 --- /dev/null +++ b/pyplet/server/oauth_providers.py @@ -0,0 +1,57 @@ +""" +pyplet.server.oauth_providers +============================= + +The OIDC provider **presets** Pyplet ships out of the box. + +``pyplet.server.oauth`` is the provider-agnostic engine: discovery, the CSRF +state cookie, the code exchange, JWKS signature verification, the signed +session cookie and the fail-closed startup policy. It names no provider — it +holds an open registry instead, and this module is the catalog that pre-fills +it at import so an env-only deployment (set ``OAUTH_GOOGLE_CLIENT_ID`` + +``OAUTH_GOOGLE_CLIENT_SECRET``, get a "Continue with Google" button) keeps +working with zero application code. + +The split is the point: the engine has no branch on a provider name, so +**adding a provider is data, not a code change**. Add a preset here to ship one +with the framework, or call ``oauth.register_provider()`` from an application +at import time to add — or override — one without touching the framework at +all. Registering under an existing name replaces that preset. + +Every spec value may be a zero-argument callable: the engine resolves it at +use time, so a preset reads ``config`` (hence the environment) lazily, when +the flow runs, instead of freezing it at import time. +""" + +from __future__ import annotations + +from typing import Any + +from .config import config + +_GOOGLE_OPENID_CONFIG_URL = ( + "https://accounts.google.com/.well-known/openid-configuration" +) +_MICROSOFT_OPENID_CONFIG_URL = ( + "https://login.microsoftonline.com/" + "{tenant}/v2.0/.well-known/openid-configuration" +) + +BUILTIN_PROVIDERS: dict[str, dict[str, Any]] = { + "google": { + "label": "Google", + "openid_config_url": _GOOGLE_OPENID_CONFIG_URL, + "client_id": lambda: config.oauth_google_client_id, + "client_secret": lambda: config.oauth_google_client_secret, + "scopes": ["openid", "email", "profile"], + }, + "microsoft": { + "label": "Microsoft", + "openid_config_url": lambda: _MICROSOFT_OPENID_CONFIG_URL.format( + tenant=config.oauth_microsoft_tenant + ), + "client_id": lambda: config.oauth_microsoft_client_id, + "client_secret": lambda: config.oauth_microsoft_client_secret, + "scopes": ["openid", "email", "profile"], + }, +} diff --git a/pyplet/server/templates.py b/pyplet/server/templates.py index 41f1418..07e20dc 100644 --- a/pyplet/server/templates.py +++ b/pyplet/server/templates.py @@ -208,7 +208,10 @@ def default_navbar( ], } -_PROVIDER_LABEL = {"google": "Google", "microsoft": "Microsoft"} +# Labels come from the provider registry (oauth.provider_label), so a provider +# an application registers renders its own name here without touching this +# module. Only the icons stay local: they are presentation assets, and an +# unknown provider simply renders label-only. def login_template(handler: RequestHandler) -> Node: @@ -222,7 +225,7 @@ def login_template(handler: RequestHandler) -> Node: # ── OAuth provider buttons ────────────────────────────────────────── for provider in providers: icon_node = _PROVIDER_ICONS.get(provider, "") - label_text = _PROVIDER_LABEL.get(provider, provider.title()) + label_text = oauth.provider_label(provider) card_children.append( a( ".btn.btn-outline-secondary.btn-lg.w-100" diff --git a/tests/oauth_cookie_security_test.py b/tests/oauth_cookie_security_test.py index e83d791..d18aae0 100644 --- a/tests/oauth_cookie_security_test.py +++ b/tests/oauth_cookie_security_test.py @@ -7,7 +7,7 @@ Mirrors ``tests/config_test.py`` / ``tests/oauth_test.py``: env-driven via the ``monkeypatch`` fixture, no live server. ``_use_secure_cookies`` and ``set_session`` are sync; the two OAuth-state writers (``start_login`` / -``start_drive_consent``) are async and use ``@pytest.mark.asyncio`` (asyncio is +``start_consent``) are async and use ``@pytest.mark.asyncio`` (asyncio is in STRICT mode — pytest-asyncio 1.4.0 is installed). The Secure decision is gated on ``config.url`` / ``PYPLET_SECURE_COOKIES`` and @@ -71,6 +71,27 @@ async def _stub_oidc(provider): } +def _register_consent_flow(monkeypatch, name="extra-scope"): + """Register a throwaway incremental-consent flow and return its name. + + The cookie contract under test belongs to ``start_consent`` itself, not to + any particular flow, so this registers a minimal one against the built-in + ``google`` provider. Restored on teardown via ``monkeypatch.setitem`` so + the module-level registry does not leak between tests. + """ + monkeypatch.setitem( + oauth._CONSENT_FLOWS, + name, + {"provider": "google", "on_complete": _noop_complete}, + ) + return name + + +async def _noop_complete(handler, user_info, tokens): + """Consent-flow completion that does nothing (cookie tests ignore it).""" + return None + + # --------------------------------------------------------------------------- # AC3(a) — _use_secure_cookies() decision logic # --------------------------------------------------------------------------- @@ -141,13 +162,14 @@ async def test_start_login_forwards_secure_true_on_https(monkeypatch): @pytest.mark.asyncio -async def test_start_drive_consent_forwards_secure_true_on_https(monkeypatch): - """The drive-consent OAuth-state cookie carries secure=True on https.""" +async def test_start_consent_forwards_secure_true_on_https(monkeypatch): + """The consent-flow OAuth-state cookie carries secure=True on https.""" monkeypatch.setenv("PYPLET_URL", "https://pyplet.example.com") _enable_oauth(monkeypatch) monkeypatch.setattr(oauth, "_fetch_oidc_config", _stub_oidc) + flow = _register_consent_flow(monkeypatch) handler = MagicMock() - await oauth.start_drive_consent(handler, "/") + await oauth.start_consent(handler, flow, "/") assert handler.set_signed_cookie.call_args.kwargs.get("secure") is True @@ -164,13 +186,14 @@ async def test_start_login_no_secure_on_plain_http(monkeypatch): @pytest.mark.asyncio -async def test_start_drive_consent_no_secure_on_plain_http(monkeypatch): - """The drive-consent OAuth-state cookie omits Secure in plain-http dev.""" +async def test_start_consent_no_secure_on_plain_http(monkeypatch): + """The consent-flow OAuth-state cookie omits Secure in plain-http dev.""" monkeypatch.setenv("PYPLET_URL", "http://127.0.0.1:8080") _enable_oauth(monkeypatch) monkeypatch.setattr(oauth, "_fetch_oidc_config", _stub_oidc) + flow = _register_consent_flow(monkeypatch) handler = MagicMock() - await oauth.start_drive_consent(handler, "/") + await oauth.start_consent(handler, flow, "/") assert not handler.set_signed_cookie.call_args.kwargs.get("secure") diff --git a/tests/oauth_registry_test.py b/tests/oauth_registry_test.py new file mode 100644 index 0000000..ff134fc --- /dev/null +++ b/tests/oauth_registry_test.py @@ -0,0 +1,397 @@ +"""Registry tests for ``pyplet.server.oauth`` — providers and consent flows. + +``oauth`` is the provider-agnostic OIDC engine: it branches on no provider name +and hardcodes no vendor endpoint. What used to be a hardcoded provider dict and +a hardcoded Drive-consent path is now two open registries, filled through +:func:`oauth.register_provider` and :func:`oauth.register_consent_flow`. + +These tests pin that genericity from the outside — every one of them registers +a **fictional** provider/flow, never a shipped preset, so a test passing here +proves the engine really does work for a provider it has never heard of. The +observable behaviour previously covered only for Google/Drive is covered here +against that fictional provider: the authorize URL, the state cookie's +``flow`` field, the callback routing to the flow instead of the session, and +the "a failing flow must not strand the browser" guarantee. + +No network: ``_fetch_oidc_config`` and the token-exchange ``httpx`` client are +stubbed. Async tests use ``@pytest.mark.asyncio`` (STRICT mode). +""" + +import json +from unittest.mock import MagicMock +from urllib.parse import parse_qs, urlparse + +import pytest + +from pyplet.server import oauth + +_AUTH_ENDPOINT = "https://id.example.test/authorize" +_TOKEN_ENDPOINT = "https://id.example.test/token" + +# A provider the framework has never heard of, declared entirely by a test. +_SPEC = { + "label": "Example ID", + "openid_config_url": ( + "https://id.example.test/.well-known/openid-configuration" + ), + "client_id": "example-client", + "client_secret": "example-secret", + "scopes": ["openid", "email"], +} + + +@pytest.fixture(autouse=True) +def _isolate_registries(monkeypatch): + """Give each test its own copy of both registries. + + ``register_provider``/``register_consent_flow`` mutate module-level dicts, + so without this a test that registers or overrides would leak into the + next one (and into the rest of the session). + """ + monkeypatch.setattr( + oauth, "_PROVIDER_CONFIGS", dict(oauth._PROVIDER_CONFIGS) + ) + monkeypatch.setattr(oauth, "_CONSENT_FLOWS", dict(oauth._CONSENT_FLOWS)) + yield + + +def _handler(**arguments): + """A Tornado-handler double whose ``get_argument`` serves *arguments*.""" + handler = MagicMock() + handler.get_argument.side_effect = lambda name, default=None: ( + arguments.get(name, default) + ) + return handler + + +def _redirect_params(handler): + """Parse the query of the URL the handler was redirected to.""" + url = handler.redirect.call_args.args[0] + return {k: v[0] for k, v in parse_qs(urlparse(url).query).items()} + + +def _state_cookie(handler): + """Decode the JSON written into the signed OAuth-state cookie.""" + return json.loads(handler.set_signed_cookie.call_args.args[1]) + + +async def _stub_oidc(provider): + """Async stand-in for ``oauth._fetch_oidc_config`` (no network).""" + return { + "authorization_endpoint": _AUTH_ENDPOINT, + "token_endpoint": _TOKEN_ENDPOINT, + "issuer": "https://id.example.test", + } + + +# --------------------------------------------------------------------------- +# register_provider +# --------------------------------------------------------------------------- + + +def test_register_provider_makes_it_enabled_and_labelled(): + """A registered provider with a client_id shows up as an enabled one.""" + oauth.register_provider("example", _SPEC) + assert "example" in oauth.enabled_providers() + assert oauth.provider_label("example") == "Example ID" + + +def test_register_provider_without_client_id_is_not_enabled(): + """A registered-but-unconfigured provider offers no login button. + + Same contract the shipped presets rely on: registration is not + configuration, an empty client_id keeps it off the login page. + """ + oauth.register_provider("example", {**_SPEC, "client_id": ""}) + assert "example" not in oauth.enabled_providers() + + +def test_provider_label_falls_back_to_the_name(): + """A spec without a label still renders sensibly on the login page.""" + spec = {k: v for k, v in _SPEC.items() if k != "label"} + oauth.register_provider("example", spec) + assert oauth.provider_label("example") == "Example" + + +@pytest.mark.parametrize( + "missing", ["openid_config_url", "client_id", "client_secret"] +) +def test_register_provider_rejects_an_incomplete_spec(missing): + """A spec missing a required key fails at registration, not at login.""" + spec = {k: v for k, v in _SPEC.items() if k != missing} + with pytest.raises(ValueError, match=missing): + oauth.register_provider("example", spec) + assert "example" not in oauth._PROVIDER_CONFIGS + + +def test_register_provider_overrides_a_previous_registration(): + """Re-registering a name replaces it — how an app overrides a preset.""" + oauth.register_provider("example", _SPEC) + oauth.register_provider("example", {**_SPEC, "label": "Corporate SSO"}) + assert oauth.provider_label("example") == "Corporate SSO" + assert len([n for n in oauth._PROVIDER_CONFIGS if n == "example"]) == 1 + + +def test_spec_values_may_be_callables_resolved_at_use_time(): + """A callable spec value is resolved per use, not frozen at registration. + + This is what lets a spec read config/env lazily: the client_id below only + becomes non-empty after registration, and the provider becomes enabled + without re-registering. + """ + box = {"client_id": ""} + oauth.register_provider( + "example", {**_SPEC, "client_id": lambda: box["client_id"]} + ) + assert "example" not in oauth.enabled_providers() + box["client_id"] = "configured-later" + assert "example" in oauth.enabled_providers() + + +@pytest.mark.asyncio +async def test_start_login_uses_the_registered_spec(monkeypatch): + """The authorize URL comes wholly from the spec — no vendor default.""" + monkeypatch.setattr(oauth, "_fetch_oidc_config", _stub_oidc) + oauth.register_provider( + "example", {**_SPEC, "auth_params": {"prompt": "login"}} + ) + handler = _handler(next="/somewhere") + + await oauth.start_login(handler, "example") + + params = _redirect_params(handler) + assert handler.redirect.call_args.args[0].startswith(_AUTH_ENDPOINT) + assert params["client_id"] == "example-client" + assert params["scope"] == "openid email" + assert params["prompt"] == "login" # spec auth_params beat the default + assert _state_cookie(handler)["next"] == "/somewhere" + + +@pytest.mark.asyncio +async def test_start_login_on_an_unregistered_provider_raises(monkeypatch): + """An unregistered name fails loudly, naming what IS registered.""" + monkeypatch.setattr(oauth, "_fetch_oidc_config", _stub_oidc) + with pytest.raises(KeyError, match="nope"): + await oauth.start_login(_handler(), "nope") + + +# --------------------------------------------------------------------------- +# register_consent_flow / start_consent +# --------------------------------------------------------------------------- + + +async def _noop_complete(handler, user_info, tokens): + """Consent completion that records nothing.""" + return None + + +def _register_flow(**overrides): + """Register the fictional provider plus a consent flow against it.""" + oauth.register_provider("example", _SPEC) + flow = { + "provider": "example", + "scopes": ["https://api.example.test/auth/files"], + "auth_params": {"access_type": "offline", "prompt": "consent"}, + "on_complete": _noop_complete, + } + flow.update(overrides) + oauth.register_consent_flow("files", flow) + return flow + + +@pytest.mark.parametrize("missing", ["provider", "on_complete"]) +def test_register_consent_flow_rejects_an_incomplete_spec(missing): + """A flow missing a required key fails at registration.""" + flow = {"provider": "example", "on_complete": _noop_complete} + del flow[missing] + with pytest.raises(ValueError, match=missing): + oauth.register_consent_flow("files", flow) + assert "files" not in oauth._CONSENT_FLOWS + + +@pytest.mark.asyncio +async def test_start_consent_adds_flow_scopes_and_params(monkeypatch): + """Consent asks the provider's scopes PLUS the flow's, with its params.""" + monkeypatch.setattr(oauth, "_fetch_oidc_config", _stub_oidc) + _register_flow() + handler = _handler() + + await oauth.start_consent(handler, "files", "/back-here") + + params = _redirect_params(handler) + assert params["scope"] == ( + "openid email https://api.example.test/auth/files" + ) + assert params["access_type"] == "offline" + assert params["prompt"] == "consent" # flow params beat the provider's + assert params["client_id"] == "example-client" # same client as login + + +@pytest.mark.asyncio +async def test_start_consent_marks_the_flow_in_the_state_cookie(monkeypatch): + """The state cookie carries the flow name — what routes the callback.""" + monkeypatch.setattr(oauth, "_fetch_oidc_config", _stub_oidc) + _register_flow() + handler = _handler() + + await oauth.start_consent(handler, "files", "/back-here") + + cookie = _state_cookie(handler) + assert cookie["flow"] == "files" + assert cookie["provider"] == "example" + assert cookie["next"] == "/back-here" + + +@pytest.mark.asyncio +async def test_start_consent_on_an_unregistered_flow_raises(monkeypatch): + """An unregistered flow fails loudly rather than redirecting nowhere.""" + monkeypatch.setattr(oauth, "_fetch_oidc_config", _stub_oidc) + with pytest.raises(KeyError, match="nope"): + await oauth.start_consent(_handler(), "nope", "/") + + +# --------------------------------------------------------------------------- +# handle_callback — consent routing +# --------------------------------------------------------------------------- + + +def _stub_callback(monkeypatch, tokens=None): + """Stub discovery, the token exchange and id_token verification.""" + tokens = tokens or { + "id_token": "opaque", + "refresh_token": "refresh-abc", + "scope": "openid email https://api.example.test/auth/files", + } + monkeypatch.setattr(oauth, "_fetch_oidc_config", _stub_oidc) + + async def _verify(id_token, provider): + return {"sub": "u-1", "email": "User@Example.test", "name": "User"} + + monkeypatch.setattr(oauth, "_verify_id_token_claims", _verify) + + class _Resp: + def raise_for_status(self): + pass + + def json(self): + return tokens + + class _Client: + async def __aenter__(self): + return self + + async def __aexit__(self, *exc): + return False + + async def post(self, url, data=None, timeout=None): + return _Resp() + + monkeypatch.setattr(oauth.httpx, "AsyncClient", _Client) + return tokens + + +def _callback_handler(flow="files"): + """A handler mid-callback: matching state cookie, code in the query.""" + state = {"state": "s-1", "next": "/back-here", "provider": "example"} + if flow: + state["flow"] = flow + handler = _handler(state="s-1", code="the-code") + handler.get_signed_cookie.return_value = json.dumps(state) + return handler + + +@pytest.mark.asyncio +async def test_callback_hands_the_tokens_to_the_flow(monkeypatch): + """A consent callback reaches on_complete with the raw token response.""" + tokens = _stub_callback(monkeypatch) + seen = {} + + async def _complete(handler, user_info, tok): + seen["user_info"] = user_info + seen["tokens"] = tok + + _register_flow(on_complete=_complete) + handler = _callback_handler() + + await oauth.handle_callback(handler) + + assert seen["tokens"] == tokens # incl. refresh_token + granted scope + assert seen["user_info"]["sub"] == "u-1" + assert seen["user_info"]["email"] == "user@example.test" # normalised + handler.redirect.assert_called_once_with("/back-here") + + +@pytest.mark.asyncio +async def test_callback_does_not_touch_the_session_on_a_consent_flow( + monkeypatch, +): + """Incremental consent must not re-issue the login session cookie.""" + _stub_callback(monkeypatch) + sessions = [] + monkeypatch.setattr( + oauth, "set_session", lambda h, info: sessions.append(info) + ) + _register_flow() + + await oauth.handle_callback(_callback_handler()) + + assert sessions == [] + + +@pytest.mark.asyncio +async def test_callback_without_a_flow_still_logs_in(monkeypatch): + """The plain login callback is untouched by the consent routing.""" + _stub_callback(monkeypatch) + sessions = [] + monkeypatch.setattr( + oauth, "set_session", lambda h, info: sessions.append(info) + ) + oauth.register_provider("example", _SPEC) + + handler = _callback_handler(flow=None) + await oauth.handle_callback(handler) + + assert [info["email"] for info in sessions] == ["user@example.test"] + handler.redirect.assert_called_once_with("/back-here") + + +@pytest.mark.asyncio +async def test_a_failing_flow_still_redirects_the_browser(monkeypatch): + """A raising on_complete is logged and swallowed, never a dead end. + + The browser is mid-redirect from the provider; a bookkeeping failure in + the app must not strand it on an error page. + """ + _stub_callback(monkeypatch) + + async def _boom(handler, user_info, tokens): + raise RuntimeError("could not store the token") + + _register_flow(on_complete=_boom) + handler = _callback_handler() + + await oauth.handle_callback(handler) + + handler.redirect.assert_called_once_with("/back-here") + + +@pytest.mark.asyncio +async def test_callback_for_an_unregistered_flow_is_refused(monkeypatch): + """A state cookie naming an unknown flow is a 400, never a silent login. + + Without this the engine would fall through to set_session and turn a + consent round-trip into an unrequested login. + """ + _stub_callback(monkeypatch) + sessions = [] + monkeypatch.setattr( + oauth, "set_session", lambda h, info: sessions.append(info) + ) + oauth.register_provider("example", _SPEC) + + handler = _callback_handler(flow="never-registered") + await oauth.handle_callback(handler) + + handler.set_status.assert_called_once_with(400) + assert sessions == [] + handler.redirect.assert_not_called() diff --git a/zensical.toml b/zensical.toml index e64d528..96c0239 100644 --- a/zensical.toml +++ b/zensical.toml @@ -88,6 +88,7 @@ nav = [ { "Bootstrap Helpers" = "api/bootstrap.md" }, { "Client Runtime" = "api/client.md" }, { "Server" = "api/server.md" }, + { "OAuth Providers" = "api/oauth-providers.md" }, { "WebSocket Protocol" = "api/websocket.md" }, { "Client Utils" = "api/client-utils.md" } ]}, From f9abcac2d298d559b0909b58987557445b4f1529 Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Tue, 25 Aug 2026 14:29:07 +0200 Subject: [PATCH 21/28] docs(oauth): drop the false "byte-for-byte" migration claim The drive-consent example claimed the authorization request it produces is byte-for-byte the one start_drive_consent built. It is not: the Google preset declares no auth_params, so _provider_auth_params falls back to _DEFAULT_AUTH_PARAMS ({"prompt": "select_account"}), which claims the "prompt" slot before the flow's auth_params are merged in. The flow's "prompt": "consent" then overwrites that earlier slot, hoisting prompt ahead of access_type in the encoded query string. The parameter set and every value are identical (8 params, the dicts compare equal); only the encoding order differs, which is not significant to OAuth. Reword to claim equivalence rather than byte identity, so the chani team reading this as a migration reference is not misled into diffing request bytes. --- docs/api/oauth-providers.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/api/oauth-providers.md b/docs/api/oauth-providers.md index 0298da7..cf04005 100644 --- a/docs/api/oauth-providers.md +++ b/docs/api/oauth-providers.md @@ -117,5 +117,7 @@ oauth.register_consent_flow("drive", { }) ``` -The authorization request this produces is byte-for-byte the one -`start_drive_consent` used to build. +The authorization request this produces is equivalent to the one +`start_drive_consent` used to build — same endpoint, same scopes, same +parameters and values. Only the order in which the query string encodes +them differs, which OAuth does not treat as significant. From 7128406259e6f97dac5622b910d001b76a555016 Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Fri, 3 Jul 2026 13:15:00 +0200 Subject: [PATCH 22/28] fix(security): confine app static route to per-app static/ dir (path traversal) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The /apps/<project>/static/<tail> route registered a single Tornado StaticFileHandler rooted at the whole apps/ tree with a one-group regex. Tornado's validate_absolute_path only prevents escaping root (apps/), so a '..' in the captured tail escaped the per-app static/ dir while staying under apps/ — an unauthenticated path traversal leaking every app's *_server.py source and auth_rules.json (the ACL). Fix: capture project and file tail as two groups and root a new AppStaticFileHandler at <apps>/<project>/static per request, so a traversal (raw or percent-encoded) can no longer resolve outside the requested app's own static/ dir. Apps with no static/ dir 404 instead of crashing. Adds a self-contained AsyncHTTPTestCase regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019iFv2TeDxX86EDqgaQyqZG (cherry picked from commit 27495cb311aa16a5a9872287797e60c3119eac19) --- pyplet/server/_server.py | 44 ++++++++- tests/app_static_traversal_test.py | 152 +++++++++++++++++++++++++++++ 2 files changed, 191 insertions(+), 5 deletions(-) create mode 100644 tests/app_static_traversal_test.py diff --git a/pyplet/server/_server.py b/pyplet/server/_server.py index b312771..16a8469 100644 --- a/pyplet/server/_server.py +++ b/pyplet/server/_server.py @@ -131,6 +131,38 @@ def set_extra_headers(self, path: str) -> None: self.set_header("Cache-Control", "no-cache") +class AppStaticFileHandler(tornado.web.StaticFileHandler): + """Serve an app's static assets, confined to ``<apps>/<project>/static``. + + The route captures the app *project* and the file *tail* as two separate + groups (``/apps/<project>/static/<tail>``) and this handler roots itself at + that one app's ``static/`` directory per request. Tornado's + ``validate_absolute_path`` only guarantees a request cannot escape + ``self.root``; rooting the handler at the whole ``apps/`` tree (the old + behaviour) let a ``..`` in the tail climb out of ``static/`` into a sibling + app's server source (``*_server.py``) or the ACL file (``auth_rules.json``) + while staying under ``apps/`` — an unauthenticated path traversal. Rooting + per-app closes it: a ``..`` (raw or percent-encoded, which Tornado + url-decodes before matching) can no longer resolve to anything outside the + requested app's own ``static/`` dir (attempts get 403/404). Apps with no + ``static/`` dir simply 404 rather than crashing the server. + """ + + def initialize(self, apps_root: str) -> None: + # StaticFileHandler.initialize requires a ``path``; the real + # per-request root is (re)computed in get() once ``project`` is known. + super().initialize(path=apps_root) + self._apps_root = apps_root + + async def get( # noqa: A003 - Tornado handler hook + self, project: str, path: str, include_body: bool = True + ) -> None: + # Confine this request to the requested app's own static/ dir so a + # traversal in ``path`` cannot escape into the wider apps/ tree. + self.root = os.path.join(self._apps_root, project, "static") + await super().get(path, include_body=include_body) + + # --------------------------------------------------------------------------- # Application handlers # --------------------------------------------------------------------------- @@ -453,11 +485,13 @@ async def aclose(self): (r"/auth/verify", MagicLinkVerifyHandler), # App static resources (static files) ( - # ONE capture group covering the app name, - # the static folder, and the filename - r"/apps/([a-zA-Z_][a-zA-Z0-9_]*/static/.*)", - tornado.web.StaticFileHandler, - {"path": config.apps}, + # Capture the app project and the file tail SEPARATELY so the + # handler can root itself at <apps>/<project>/static/ per request + # (see AppStaticFileHandler): a ".." in the tail then cannot escape + # the requested app's own static/ dir into the apps/ tree. + r"/apps/([a-zA-Z_][a-zA-Z0-9_]*)/static/(.*)", + AppStaticFileHandler, + {"apps_root": config.apps}, ), # App upload endpoint (for upload() and upload_area()) ( diff --git a/tests/app_static_traversal_test.py b/tests/app_static_traversal_test.py new file mode 100644 index 0000000..a0c8d38 --- /dev/null +++ b/tests/app_static_traversal_test.py @@ -0,0 +1,152 @@ +"""pyplet-CORE app-static path-traversal tests (qs-port security hardening). + +WHAT THIS PROVES +================ +The ``/apps/<project>/static/<tail>`` route serves each app's static assets. +It used to register a single Tornado ``StaticFileHandler`` rooted at the whole +``apps/`` tree with a ONE-group regex +(``/apps/([a-zA-Z_][a-zA-Z0-9_]*/static/.*)``). Tornado's +``validate_absolute_path`` only prevents escaping ``root`` (``apps/``), so a +``..`` in the captured tail escaped the intended per-app ``static/`` dir while +staying under ``apps/`` — an unauthenticated path traversal that leaked every +app's server source (``*_server.py``) and the ACL file (``auth_rules.json``). + +The fix roots the handler at ``<apps>/<project>/static`` PER REQUEST +(``AppStaticFileHandler`` + a two-group regex). This pins: + + (a) a legitimate static file still serves (200); + (b) ``..`` traversal to a sibling ``*_server.py`` is refused (NOT 200); + (c) ``..`` traversal up to ``auth_rules.json`` is refused (NOT 200); + (d) the percent-encoded ``%2e%2e`` variant is also refused (NOT 200); + (e) the route regex captures project + tail as two groups (structural guard). + +Self-contained: a temp apps/ tree is built with one app that has a ``static/`` +file, plus a sibling ``*_server.py`` and an ``auth_rules.json`` as traversal +targets. Uses Tornado's ``AsyncHTTPTestCase`` harness (shipped with the +existing ``tornado`` dep — no new dependency), mirroring ``healthz_test.py``. +""" + +import os +import tempfile + +from tornado.testing import AsyncHTTPTestCase +from tornado.web import Application + +from pyplet.server._server import AppStaticFileHandler, _app_spec + + +def _handler_patterns() -> list[str]: + """Pattern strings of ``_app_spec['handlers']``, in declared order.""" + return [entry[0] for entry in _app_spec["handlers"]] + + +def test_app_static_route_captures_project_and_tail_separately(): + """The static route must capture project + tail as two groups. + + A single group spanning ``<project>/static/<tail>`` cannot be rooted + per-app, which is exactly what let ``..`` escape. This structural guard + fails if someone reverts the regex to the vulnerable one-group form. + """ + patterns = _handler_patterns() + static_routes = [p for p in patterns if "/static/" in p] + assert static_routes == [r"/apps/([a-zA-Z_][a-zA-Z0-9_]*)/static/(.*)"], ( + "app static route must capture project and file tail separately; " + f"got {static_routes!r}" + ) + + +class AppStaticTraversalTest(AsyncHTTPTestCase): + """Real-HTTP assertions against a temp apps/ tree with a traversal bait.""" + + def setUp(self): + # Build the temp apps/ tree BEFORE AsyncHTTPTestCase.setUp() calls + # get_app(), which needs self._apps_root. + self._tmp = tempfile.TemporaryDirectory() + apps_root = self._tmp.name + self._apps_root = apps_root + + app_static = os.path.join(apps_root, "DemoApp", "static") + os.makedirs(app_static) + with open(os.path.join(app_static, "d3.v7.min.js"), "w") as f: + f.write("// legit static asset\n") + + # Bait files a traversal would target. + with open( + os.path.join(apps_root, "DemoApp", "DemoApp_server.py"), "w" + ) as f: + f.write("SECRET = 'server-side source that must not leak'\n") + with open(os.path.join(apps_root, "auth_rules.json"), "w") as f: + f.write('{"acl": "must not leak"}\n') + + super().setUp() + + def tearDown(self): + super().tearDown() + self._tmp.cleanup() + + def get_app(self) -> Application: + return Application( + [ + ( + r"/apps/([a-zA-Z_][a-zA-Z0-9_]*)/static/(.*)", + AppStaticFileHandler, + {"apps_root": self._apps_root}, + ), + ] + ) + + def test_legit_static_file_is_served(self): + """A real per-app static asset still returns 200.""" + resp = self.fetch("/apps/DemoApp/static/d3.v7.min.js") + assert resp.code == 200, ( + f"legit static file should serve 200, got {resp.code}" + ) + assert b"legit static asset" in resp.body + + def test_traversal_to_sibling_server_source_blocked(self): + """``..`` up to the sibling ``*_server.py`` must NOT return 200.""" + resp = self.fetch( + "/apps/DemoApp/static/../DemoApp_server.py", + follow_redirects=False, + ) + assert resp.code != 200, ( + "traversal to *_server.py must be refused, got 200 " + f"(body={resp.body!r})" + ) + assert b"SECRET" not in resp.body + assert resp.code in (403, 404), ( + f"expected 403/404 for traversal, got {resp.code}" + ) + + def test_traversal_to_auth_rules_blocked(self): + """``../../auth_rules.json`` must NOT return 200.""" + resp = self.fetch( + "/apps/DemoApp/static/../../auth_rules.json", + follow_redirects=False, + ) + assert resp.code != 200, ( + "traversal to auth_rules.json must be refused, got 200 " + f"(body={resp.body!r})" + ) + assert b"must not leak" not in resp.body + + def test_percent_encoded_traversal_blocked(self): + """The percent-encoded ``%2e%2e`` variant must NOT return 200.""" + resp = self.fetch( + "/apps/DemoApp/static/%2e%2e/DemoApp_server.py", + follow_redirects=False, + ) + assert resp.code != 200, ( + "percent-encoded traversal must be refused, got 200 " + f"(body={resp.body!r})" + ) + assert b"SECRET" not in resp.body + + def test_app_without_static_dir_does_not_crash(self): + """An app with no static/ dir 404s rather than erroring.""" + resp = self.fetch( + "/apps/NoSuchApp/static/whatever.js", follow_redirects=False + ) + assert resp.code == 404, ( + f"missing static dir should 404, got {resp.code}" + ) From 7038885d5affe77309991e2538ff4d91719f4e5b Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Fri, 3 Jul 2026 13:50:50 +0200 Subject: [PATCH 23/28] fix(security): handle HEAD in AppStaticFileHandler (regression from 6d94cdb) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019iFv2TeDxX86EDqgaQyqZG (cherry picked from commit 830839c130f0bf50fd2c84be8417774e52d33279) --- pyplet/server/_server.py | 9 ++++++ tests/app_static_traversal_test.py | 44 ++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/pyplet/server/_server.py b/pyplet/server/_server.py index 16a8469..5e78f93 100644 --- a/pyplet/server/_server.py +++ b/pyplet/server/_server.py @@ -162,6 +162,15 @@ async def get( # noqa: A003 - Tornado handler hook self.root = os.path.join(self._apps_root, project, "static") await super().get(path, include_body=include_body) + async def head( # noqa: A003 - Tornado handler hook + self, project: str, path: str + ) -> None: + # Tornado dispatches every method as method(*path_args), so HEAD must + # accept both capture groups too (project, tail). Delegate to the + # body-less GET path (which sets the per-request ``self.root``), + # mirroring StaticFileHandler.head -> get(path, include_body=False). + await self.get(project, path, include_body=False) + # --------------------------------------------------------------------------- # Application handlers diff --git a/tests/app_static_traversal_test.py b/tests/app_static_traversal_test.py index a0c8d38..ee0feb2 100644 --- a/tests/app_static_traversal_test.py +++ b/tests/app_static_traversal_test.py @@ -142,6 +142,50 @@ def test_percent_encoded_traversal_blocked(self): ) assert b"SECRET" not in resp.body + def test_head_legit_static_file_is_served(self): + """HEAD on a real asset: 200, empty body, matching Content-Length. + + Tornado dispatches every method as ``method(*path_args)``; the + two-group route means HEAD is called with (project, tail). Without a + matching ``head`` override the inherited + ``StaticFileHandler.head(self, path)`` got two positional args and + raised TypeError -> HTTP 500. This pins the fix: HEAD works and its + Content-Length matches the GET body length. + """ + get_resp = self.fetch("/apps/DemoApp/static/d3.v7.min.js") + assert get_resp.code == 200 + head_resp = self.fetch( + "/apps/DemoApp/static/d3.v7.min.js", method="HEAD" + ) + assert head_resp.code == 200, ( + "HEAD on a legit static file should serve 200, got " + f"{head_resp.code} (body={head_resp.body!r})" + ) + assert head_resp.body == b"", ( + f"HEAD response body must be empty, got {head_resp.body!r}" + ) + assert head_resp.headers.get("Content-Length") == str( + len(get_resp.body) + ), ( + "HEAD Content-Length must match GET body length; got " + f"{head_resp.headers.get('Content-Length')!r} vs " + f"{len(get_resp.body)}" + ) + + def test_head_traversal_to_sibling_server_source_blocked(self): + """HEAD ``..`` to the sibling ``*_server.py`` must NOT return 200.""" + resp = self.fetch( + "/apps/DemoApp/static/../DemoApp_server.py", + method="HEAD", + follow_redirects=False, + ) + assert resp.code != 200, ( + "HEAD traversal to *_server.py must be refused, got 200" + ) + assert resp.code in (403, 404), ( + f"expected 403/404 for HEAD traversal, got {resp.code}" + ) + def test_app_without_static_dir_does_not_crash(self): """An app with no static/ dir 404s rather than erroring.""" resp = self.fetch( From 59c066654407de469a149a52c4ba4514fdd2c975 Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Wed, 26 Aug 2026 19:35:44 +0200 Subject: [PATCH 24/28] fix(security): refuse symlink escapes from app static/ dirs Tornado's containment test in `validate_absolute_path` is purely lexical (`abspath` prefix compare, never `realpath`), so confining the route to `apps/<project>/static/` still served whatever a symlink planted in that directory pointed at -- including files outside `apps/` entirely, an unauthenticated arbitrary-file read strictly worse than the traversal the previous commit fixed. Found by adversarial review of the MR. Override `validate_absolute_path` to re-check the symlink-RESOLVED paths with `os.path.commonpath`. Symlinks that stay inside `static/` keep working; only escapes are refused. Tests: symlink to a file outside the tree, read through a symlinked directory, and the HEAD variant of each -- all refused; an internal symlink still returns 200. --- pyplet/server/_server.py | 25 ++++++ tests/app_static_traversal_test.py | 126 ++++++++++++++++++++++++++--- 2 files changed, 140 insertions(+), 11 deletions(-) diff --git a/pyplet/server/_server.py b/pyplet/server/_server.py index 5e78f93..6422473 100644 --- a/pyplet/server/_server.py +++ b/pyplet/server/_server.py @@ -146,6 +146,9 @@ class AppStaticFileHandler(tornado.web.StaticFileHandler): url-decodes before matching) can no longer resolve to anything outside the requested app's own ``static/`` dir (attempts get 403/404). Apps with no ``static/`` dir simply 404 rather than crashing the server. + + Tornado's containment test is purely lexical, so ``validate_absolute_path`` + is overridden to re-check the symlink-resolved paths too — see there. """ def initialize(self, apps_root: str) -> None: @@ -171,6 +174,28 @@ async def head( # noqa: A003 - Tornado handler hook # mirroring StaticFileHandler.head -> get(path, include_body=False). await self.get(project, path, include_body=False) + def validate_absolute_path( + self, root: str, absolute_path: str + ) -> Optional[str]: + # Tornado's own containment check is LEXICAL: it compares + # ``os.path.abspath`` prefixes and never resolves symlinks. So a + # symlink planted inside an app's ``static/`` dir is served whatever it + # points at, including files entirely outside the ``apps/`` tree — an + # unauthenticated arbitrary-file read, strictly worse than the + # traversal the per-request root above closes. Re-run the containment + # test on the SYMLINK-RESOLVED paths. Symlinks that stay inside the + # app's own ``static/`` dir keep working (they resolve inside root). + validated = super().validate_absolute_path(root, absolute_path) + if validated is None: + return None + real_root = os.path.realpath(root) + real_path = os.path.realpath(validated) + if os.path.commonpath((real_root, real_path)) != real_root: + raise tornado.web.HTTPError( + 403, "%s is not in the app's static directory", self.path + ) + return validated + # --------------------------------------------------------------------------- # Application handlers diff --git a/tests/app_static_traversal_test.py b/tests/app_static_traversal_test.py index ee0feb2..9932a6a 100644 --- a/tests/app_static_traversal_test.py +++ b/tests/app_static_traversal_test.py @@ -18,12 +18,27 @@ (b) ``..`` traversal to a sibling ``*_server.py`` is refused (NOT 200); (c) ``..`` traversal up to ``auth_rules.json`` is refused (NOT 200); (d) the percent-encoded ``%2e%2e`` variant is also refused (NOT 200); - (e) the route regex captures project + tail as two groups (structural guard). + (e) an absolute tail (``static//etc/hosts``) is refused (NOT 200); + (f) the route regex captures project + tail as two groups (structural guard), + read from ``_app_spec`` itself so a revert to the one-group form fails. + +Rooting per-app is not enough on its own: Tornado's containment test is purely +LEXICAL (``abspath`` prefix compare, never ``realpath``), so a symlink planted +in an app's ``static/`` dir was served whatever it pointed at, including files +outside ``apps/`` entirely — an unauthenticated arbitrary-file read. The +handler therefore overrides ``validate_absolute_path`` to re-check the +symlink-resolved paths. This also pins: + + (g) a symlink out of ``static/`` is refused, by GET and by HEAD (NOT 200); + (h) a read *through* a symlinked directory is refused (NOT 200); + (i) a symlink resolving INSIDE ``static/`` still serves (200) — the guard + refuses escapes only, internal symlinks are not collateral damage. Self-contained: a temp apps/ tree is built with one app that has a ``static/`` file, plus a sibling ``*_server.py`` and an ``auth_rules.json`` as traversal -targets. Uses Tornado's ``AsyncHTTPTestCase`` harness (shipped with the -existing ``tornado`` dep — no new dependency), mirroring ``healthz_test.py``. +targets, and symlinks into a second temp dir outside the tree as symlink bait. +Uses Tornado's ``AsyncHTTPTestCase`` harness (shipped with the existing +``tornado`` dep — no new dependency), mirroring ``healthz_test.py``. """ import os @@ -32,7 +47,7 @@ from tornado.testing import AsyncHTTPTestCase from tornado.web import Application -from pyplet.server._server import AppStaticFileHandler, _app_spec +from pyplet.server._server import _app_spec def _handler_patterns() -> list[str]: @@ -40,6 +55,13 @@ def _handler_patterns() -> list[str]: return [entry[0] for entry in _app_spec["handlers"]] +def _static_route_entry() -> tuple: + """The single ``_app_spec`` entry serving ``/apps/<project>/static/``.""" + entries = [e for e in _app_spec["handlers"] if "/static/" in e[0]] + assert len(entries) == 1, f"expected one app-static route, got {entries!r}" + return entries[0] + + def test_app_static_route_captures_project_and_tail_separately(): """The static route must capture project + tail as two groups. @@ -78,21 +100,35 @@ def setUp(self): with open(os.path.join(apps_root, "auth_rules.json"), "w") as f: f.write('{"acl": "must not leak"}\n') + # A symlink bait: a file OUTSIDE the apps/ tree entirely, reachable + # only if the containment test is lexical (see the symlink tests). + self._outside = tempfile.TemporaryDirectory() + outside_secret = os.path.join(self._outside.name, "id_rsa") + with open(outside_secret, "w") as f: + f.write("OUTSIDE-PRIVATE-KEY must not leak\n") + os.symlink(outside_secret, os.path.join(app_static, "link_file")) + os.symlink(self._outside.name, os.path.join(app_static, "link_dir")) + # A symlink that stays INSIDE static/ must keep working. + os.symlink( + os.path.join(app_static, "d3.v7.min.js"), + os.path.join(app_static, "alias.js"), + ) + super().setUp() def tearDown(self): super().tearDown() self._tmp.cleanup() + self._outside.cleanup() def get_app(self) -> Application: + # Take the PRODUCTION route entry (regex, handler class, kwarg name) + # from `_app_spec` and only redirect its root at the temp tree. A + # hand-copied entry here would keep passing if production swapped the + # handler or the regex back to the vulnerable one-group form. + pattern, handler, kwargs = _static_route_entry() return Application( - [ - ( - r"/apps/([a-zA-Z_][a-zA-Z0-9_]*)/static/(.*)", - AppStaticFileHandler, - {"apps_root": self._apps_root}, - ), - ] + [(pattern, handler, {**kwargs, "apps_root": self._apps_root})] ) def test_legit_static_file_is_served(self): @@ -142,6 +178,74 @@ def test_percent_encoded_traversal_blocked(self): ) assert b"SECRET" not in resp.body + def test_absolute_path_escape_blocked(self): + """An absolute tail (``static//etc/hosts``) must NOT return 200. + + The other traversal tests all use ``..``/``%2e%2e``. A refactor that + strips the leading slash off the tail before joining would regress + this silently, so it gets its own assertion. + """ + resp = self.fetch( + "/apps/DemoApp/static//etc/hosts", follow_redirects=False + ) + assert resp.code != 200, ( + "absolute-path escape must be refused, got 200 " + f"(body={resp.body!r})" + ) + + def test_symlink_out_of_static_dir_blocked(self): + """A symlink to a file outside ``apps/`` must NOT be served. + + Tornado's containment check is purely LEXICAL (``abspath`` prefix + compare, never ``realpath``), so before the + ``validate_absolute_path`` override a symlink planted in an app's + ``static/`` dir was served whatever it pointed at — an + unauthenticated arbitrary-file read, worse than the ``..`` traversal + the per-request root closes. + """ + resp = self.fetch( + "/apps/DemoApp/static/link_file", follow_redirects=False + ) + assert resp.code != 200, ( + "symlink out of static/ must be refused, got 200 " + f"(body={resp.body!r})" + ) + assert b"OUTSIDE-PRIVATE-KEY" not in resp.body + + def test_symlinked_directory_out_of_static_dir_blocked(self): + """Reaching through a symlinked *directory* must NOT be served.""" + resp = self.fetch( + "/apps/DemoApp/static/link_dir/id_rsa", follow_redirects=False + ) + assert resp.code != 200, ( + "read through a symlinked dir must be refused, got 200 " + f"(body={resp.body!r})" + ) + assert b"OUTSIDE-PRIVATE-KEY" not in resp.body + + def test_head_symlink_out_of_static_dir_blocked(self): + """HEAD goes through the same guard as GET.""" + resp = self.fetch( + "/apps/DemoApp/static/link_file", + method="HEAD", + follow_redirects=False, + ) + assert resp.code != 200, ( + f"HEAD on an escaping symlink must be refused, got {resp.code}" + ) + + def test_symlink_inside_static_dir_still_served(self): + """A symlink that resolves INSIDE static/ keeps working. + + The guard refuses escapes only; internal symlinks (a common way to + pin an asset version) must not become collateral damage. + """ + resp = self.fetch("/apps/DemoApp/static/alias.js") + assert resp.code == 200, ( + f"symlink inside static/ should serve 200, got {resp.code}" + ) + assert b"legit static asset" in resp.body + def test_head_legit_static_file_is_served(self): """HEAD on a real asset: 200, empty body, matching Content-Length. From 5d39681e54d2d88fc70340d905e1199dbc621d47 Mon Sep 17 00:00:00 2001 From: Arthur LORIN <Arthur.LORIN@student.umons.ac.be> Date: Wed, 26 Aug 2026 20:32:46 +0200 Subject: [PATCH 25/28] test(ws): pin permessage-deflate on the shared ServerWebSocket Cherry-picked from the abandoned qs-port branch (f083dda): the feature itself already landed on main, only its regression test was missing. Two properties: get_compression_options returns a dict (level 6), and a real handshake answers 101 with Sec-WebSocket-Extensions naming permessage-deflate. --- tests/ws_compression_test.py | 84 ++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 tests/ws_compression_test.py diff --git a/tests/ws_compression_test.py b/tests/ws_compression_test.py new file mode 100644 index 0000000..f90e62b --- /dev/null +++ b/tests/ws_compression_test.py @@ -0,0 +1,84 @@ +"""pyplet-CORE WebSocket compression tests (qs-port Phase P0). + +WHAT THIS PROVES +================ +Phase P0 of the QualiSpectra→pyplet port enables Tornado's standard +``permessage-deflate`` extension on the single shared ``ServerWebSocket`` that +fronts EVERY pyplet app, so the chatty JSON/text frames the client exchanges +are compressed on the wire. Two properties are pinned here: + + (a) **``get_compression_options`` opts into compression** — the method + returns a (non-``None``) dict; returning a dict is exactly how a Tornado + ``WebSocketHandler`` enables ``permessage-deflate``. We also assert the + ``compression_level`` we set (6, zlib's default) survives. + (b) **a real handshake negotiates ``permessage-deflate``** — a client that + offers the extension (``compression_options={}``) receives a 101 whose + ``Sec-WebSocket-Extensions`` response header names + ``permessage-deflate``. Tornado calls ``get_compression_options`` + during the upgrade (before + ``open``), so a thin no-auth subclass that only neutralizes the auth gate + still exercises the real, inherited method end-to-end. + +The handshake assertion uses Tornado's ``AsyncHTTPTestCase`` harness (shipped +with the existing ``tornado`` dep — no new dependency), mirroring +``tests/healthz_test.py``; the unit assertion builds a bare handler via +``__new__`` like ``tests/prod_hardening_test.py``. +""" + +from tornado.testing import AsyncHTTPTestCase, gen_test +from tornado.web import Application +from tornado.websocket import websocket_connect + +from pyplet.server._server import ServerWebSocket + + +def test_get_compression_options_enables_compression(): + """``get_compression_options`` returns a compression-enabling dict. + + A Tornado ``WebSocketHandler`` opts into ``permessage-deflate`` by + returning a dict (``None`` disables it); we also pin the + ``compression_level`` we configured so a silent drop to defaults is caught. + """ + ws = ServerWebSocket.__new__(ServerWebSocket) + options = ws.get_compression_options() + assert isinstance(options, dict), ( + "get_compression_options must return a dict to enable " + f"permessage-deflate, got {options!r}" + ) + assert options.get("compression_level") == 6 + + +class _NoAuthWS(ServerWebSocket): + """``ServerWebSocket`` with the auth gate + origin check neutralized. + + ``get_compression_options`` is inherited unchanged, so the handshake still + negotiates via the real method; only ``open``/``check_origin`` are stubbed + so the (routed, argument-less) upgrade completes without a session. + """ + + def check_origin(self, origin): + return True + + async def open(self): # noqa: A003 - Tornado handler hook + pass + + +class WSCompressionHandshakeTest(AsyncHTTPTestCase): + """Real-handshake assertion: ``permessage-deflate`` is negotiated.""" + + def get_app(self) -> Application: + return Application([(r"/ws", _NoAuthWS)]) + + @gen_test + async def test_handshake_negotiates_permessage_deflate(self): + """A client offering the extension gets it back in the 101 response.""" + url = f"ws://127.0.0.1:{self.get_http_port()}/ws" + conn = await websocket_connect(url, compression_options={}) + try: + extensions = conn.headers.get("Sec-WebSocket-Extensions", "") + assert "permessage-deflate" in extensions, ( + "server did not negotiate permessage-deflate; " + f"Sec-WebSocket-Extensions={extensions!r}" + ) + finally: + conn.close() From 1c93a7fe95e2244f42783eca488fddbecccf61e9 Mon Sep 17 00:00:00 2001 From: Arthur LORIN <arthur.lorin@cetic.be> Date: Wed, 26 Aug 2026 21:41:09 +0200 Subject: [PATCH 26/28] docs(readme): document the GitLab->GitHub one-way flow --- README.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/README.md b/README.md index a9bb079..1a1acd1 100644 --- a/README.md +++ b/README.md @@ -428,6 +428,21 @@ Since client code runs in PyScript (WebAssembly): ## Contributing +### Where development happens + +Pyplet lives in two places, and they are not interchangeable: + +- **GitLab `seglab/pyplet`** (CETIC forge, `git.cetic.be`) is the + **canonical** repository. Development lands there, on the default + branch `main`, through merge requests. +- **GitHub [`cetic/Pyplet`](https://github.com/cetic/Pyplet/)** is the + **publication mirror**. It is deliberately behind: nothing is developed + there, and it is refreshed from GitLab `main` by a maintainer when a + state is worth publishing. + +The flow is one-way: **GitLab `main` → GitHub**. A change pushed straight +to GitHub would be overwritten by the next publication. + Contributions are welcome! When contributing: 1. Maintain clean separation between client and server code From 34bf36346097c3ff2f03339f867aab6da2c76147 Mon Sep 17 00:00:00 2001 From: "Vincent STRAGIER (Dell Pro Max)" <vincent.stragier@cetic.be> Date: Fri, 28 Aug 2026 15:13:49 +0200 Subject: [PATCH 27/28] fix(cli): stop freezing env-sourced config params on every `start` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `parser_start`'s per-param `--<name>` flags defaulted to `os.environ.get(env_var, SUPPRESS)`, so an omitted flag was indistinguishable from an explicit one whenever its env var happened to be set. `main()` then unconditionally `setattr(config, name, value)`s every populated arg, permanently freezing that value into `config.__dict__` — which shadows the env var for the rest of the process, even across later monkeypatched changes in tests. This broke CI: `PYPLET_DEBUG=1` is set job-wide, so any cli_test.py test invoking `main(["start", ...])` froze `config.debug = "1"`, causing prod_hardening_test.py's later `monkeypatch.setenv("PYPLET_DEBUG", "0")` to be silently ignored. Default to `argparse.SUPPRESS` unconditionally instead — `config`'s own Param descriptor already reads the env var live on every access when not explicitly overridden, so seeding argparse's default from it here was redundant as well as harmful. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --- pyplet/server/cli.py | 11 ++++++++++- tests/cli_test.py | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/pyplet/server/cli.py b/pyplet/server/cli.py index 43fbd80..2e1ddd5 100644 --- a/pyplet/server/cli.py +++ b/pyplet/server/cli.py @@ -131,7 +131,16 @@ def main() -> None: f"--{name.replace('_', '-')}", required=False, help=f"{param_obj.description} [Env: {param_obj.env_var}]", - default=os.environ.get(param_obj.env_var, argparse.SUPPRESS), + # SUPPRESS unconditionally (not os.environ.get(..., SUPPRESS)): + # `config`'s own Param descriptor already falls back to the + # env var on every access when not explicitly overridden, so + # seeding the argparse default from it here is redundant — + # and harmful, since it makes an *omitted* flag indistinguishable + # from an *explicit* one. Both would get `setattr(config, ...)`'d + # below, permanently freezing that env var's current value into + # `config.__dict__`, which then shadows the env var forever + # (even across later, unrelated test/process env var changes). + default=argparse.SUPPRESS, type=param_obj.type_cast, ) diff --git a/tests/cli_test.py b/tests/cli_test.py index b42cb3c..192bfd1 100644 --- a/tests/cli_test.py +++ b/tests/cli_test.py @@ -802,6 +802,45 @@ def test_start_checks_directory_for_custom_apps_value( finally: config.apps = original_apps + @patch("pyplet.server.cli.start_server") + @patch("pyplet.server.cli.Path.exists", return_value=True) + def test_start_does_not_freeze_omitted_env_sourced_params( + self, mock_exists, mock_start_server, monkeypatch + ): + """Regression test: an omitted `--<param>` flag must never get + permanently baked into `config.__dict__` just because its env + var happens to be set (e.g. PYPLET_DEBUG=1 set job-wide in CI). + `argparse`'s default used to be `os.environ.get(env_var, + SUPPRESS)`, making an omitted flag indistinguishable from an + explicit one once its env var was set — `setattr(config, name, + value)` then froze it forever, permanently shadowing that env + var for the rest of the process (breaking, e.g., a later + `monkeypatch.setenv(...)` on the same var in an unrelated test, + such as prod_hardening_test.py toggling PYPLET_DEBUG).""" + from pyplet.server.cli import main + from pyplet.server.config import config + + original_port = config.port + config.__dict__.pop("debug", None) # start from a clean slate + monkeypatch.setenv("PYPLET_DEBUG", "1") + + try: + # --debug is never passed on the command line; only --port is. + with patch("sys.argv", ["pyplet", "start", "--port", "9999"]): + main() + + assert config.port == 9999 + # config.debug must still be reading the env var live, not a + # frozen instance value. + assert "debug" not in config.__dict__ + assert config.debug == "1" + + monkeypatch.setenv("PYPLET_DEBUG", "0") + assert config.debug == "0" + finally: + config.port = original_port + config.__dict__.pop("debug", None) + @pytest.mark.unit class TestCLIIntegration: From a123c45756d2680ff8117f55d397795c5742b048 Mon Sep 17 00:00:00 2001 From: "Vincent STRAGIER (Dell Pro Max)" <vincent.stragier@cetic.be> Date: Fri, 28 Aug 2026 16:50:39 +0200 Subject: [PATCH 28/28] fix(config): stop config.__dict__ test restores from freezing params MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Param.__set__` unconditionally writes an instance override, even when the value being written matches what was there before. So the common test pattern `original = config.x; ...; config.x = original` doesn't actually restore the *original resolution behavior* (dynamic: instance dict -> env var -> default) — it just re-freezes `config.x` to whatever it happened to read at test start, permanently shadowing that param's env var for the rest of the process. This silently broke the e2e `server` fixture (tests/conftest.py): cli_test.py's TestCLIConfigOverrides tests "restore" config.port / config.address / config.apps this way, so after they ran, config.port stayed frozen — the server fixture's forked child sets PYPLET_PORT before calling astart(), expecting config.port to read it live, but got the frozen value instead and bound the wrong port (observed as "Pyplet test server not reachable" for all 12 template_app_test.py tests, with the log showing the server actually started on :8080 instead of the assigned free port). Fix: - Add Param.__delete__ to config.py, so an instance override can actually be cleared (not just reassigned to an old value). - Replace the three affected cli_test.py "restore" spots (plus package_test.py's restore_config_apps fixture) with a full config.__dict__ snapshot/restore, which correctly undoes an override introduced mid-test instead of re-freezing it. - Add a regression test characterizing the exact failure mode. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --- pyplet/server/config.py | 9 +++ tests/cli_test.py | 147 +++++++++++++++++++++++++++------------- tests/package_test.py | 12 +++- 3 files changed, 118 insertions(+), 50 deletions(-) diff --git a/pyplet/server/config.py b/pyplet/server/config.py index 21a769c..edb98f7 100644 --- a/pyplet/server/config.py +++ b/pyplet/server/config.py @@ -58,6 +58,15 @@ def __set__(self, instance, value): else: instance.__dict__[self.name] = None + def __delete__(self, instance): + # Clear any instance override (CLI/explicit setattr), restoring + # normal env-var/default resolution. `__set__` alone can't do + # this: setting back a previously-read value still leaves a + # frozen instance override behind, which permanently shadows + # that env var for the rest of the process (see `del config.x` + # usage in tests that must NOT leak state between runs). + instance.__dict__.pop(self.name, None) + class PypletConfig: # ── Core Server ────────────────────────────────────────────────────────── diff --git a/tests/cli_test.py b/tests/cli_test.py index 192bfd1..890b797 100644 --- a/tests/cli_test.py +++ b/tests/cli_test.py @@ -738,45 +738,58 @@ def test_script_main_adds_cwd_to_path(self): sys.path = original_path +@pytest.fixture +def preserve_config_dict(): + """Snapshot/restore `config.__dict__` verbatim — not just a captured + resolved value — around a test that overrides params via direct + assignment (`config.x = value`). + + Restoring via `config.port = original_port` still calls `Param.__set__`, + which unconditionally freezes an instance override into + `config.__dict__`, even when the value being written happens to equal + what was there before. That permanently shadows the param's env var + for the rest of the process (`Param.__get__` checks `instance.__dict__` + before ever consulting `os.environ`) — e.g. it silently broke the e2e + `server` fixture's `PYPLET_PORT` override in a *later*, unrelated test + session. Clearing and restoring the whole dict (rather than + reassigning individual attributes) actually undoes any override that + didn't exist before the test, instead of re-freezing it. + """ + from pyplet.server.config import config + + original = dict(config.__dict__) + yield config + config.__dict__.clear() + config.__dict__.update(original) + + class TestCLIConfigOverrides: @patch("pyplet.server.cli.start_server") @patch("pyplet.server.cli.Path.exists", return_value=True) def test_start_command_sets_config_attributes( - self, mock_exists, mock_start_server + self, mock_exists, mock_start_server, preserve_config_dict ): """Test that CLI arguments correctly override config values.""" from pyplet.server.cli import main - from pyplet.server.config import config - # 1. Store original config values to prevent test leakage - # (Assuming your config has 'port' and 'host' as valid params) - original_port = config.port + config = preserve_config_dict original_host = config.address - # 2. Simulate the CLI command: `pyplet start --port 9999` + # Simulate the CLI command: `pyplet start --port 9999` with patch("sys.argv", ["pyplet", "start", "--port", "9999"]): main() - try: - # 3. Verify the setattr logic correctly - # updated the provided argument - assert config.port == 9999 - - # 4. Verify the `argparse.SUPPRESS` and - # `if value is not ...:` logic - # This ensures omitted arguments don't - # accidentally overwrite defaults - # with None or empty strings. - assert config.address == original_host + # Verify the setattr logic correctly updated the provided argument + assert config.port == 9999 - finally: - # 5. Clean up the singleton state so subsequent tests don't fail! - config.port = original_port - config.address = original_host + # Verify the `argparse.SUPPRESS` and `if value is not ...:` logic. + # This ensures omitted arguments don't accidentally overwrite + # defaults with None or empty strings. + assert config.address == original_host @patch("pyplet.server.cli.start_server") def test_start_checks_directory_for_custom_apps_value( - self, mock_start_server, tmp_path, monkeypatch + self, mock_start_server, tmp_path, monkeypatch, preserve_config_dict ): """Regression test: `--apps` must be applied to `config` *before* the projects-directory existence check. Previously the check ran @@ -784,28 +797,27 @@ def test_start_checks_directory_for_custom_apps_value( a directory other than "./apps" could fail even though the given directory exists (here, only "custom_apps" exists, not "apps").""" from pyplet.server.cli import main - from pyplet.server.config import config + + config = preserve_config_dict monkeypatch.delenv("PYPLET_APPS", raising=False) monkeypatch.chdir(tmp_path) (tmp_path / "custom_apps").mkdir() - original_apps = config.apps - try: - with patch( - "sys.argv", ["pyplet", "start", "--apps", "custom_apps"] - ): - main() + with patch("sys.argv", ["pyplet", "start", "--apps", "custom_apps"]): + main() - assert config.apps == "custom_apps" - mock_start_server.assert_called_once() - finally: - config.apps = original_apps + assert config.apps == "custom_apps" + mock_start_server.assert_called_once() @patch("pyplet.server.cli.start_server") @patch("pyplet.server.cli.Path.exists", return_value=True) def test_start_does_not_freeze_omitted_env_sourced_params( - self, mock_exists, mock_start_server, monkeypatch + self, + mock_exists, + mock_start_server, + monkeypatch, + preserve_config_dict, ): """Regression test: an omitted `--<param>` flag must never get permanently baked into `config.__dict__` just because its env @@ -818,28 +830,67 @@ def test_start_does_not_freeze_omitted_env_sourced_params( `monkeypatch.setenv(...)` on the same var in an unrelated test, such as prod_hardening_test.py toggling PYPLET_DEBUG).""" from pyplet.server.cli import main - from pyplet.server.config import config - original_port = config.port + config = preserve_config_dict config.__dict__.pop("debug", None) # start from a clean slate monkeypatch.setenv("PYPLET_DEBUG", "1") + # --debug is never passed on the command line; only --port is. + with patch("sys.argv", ["pyplet", "start", "--port", "9999"]): + main() + + assert config.port == 9999 + # config.debug must still be reading the env var live, not a + # frozen instance value. + assert "debug" not in config.__dict__ + assert config.debug == "1" + + monkeypatch.setenv("PYPLET_DEBUG", "0") + assert config.debug == "0" + + def test_config_dict_restore_undoes_override_left_by_naive_reassign( + self, + ): + """Regression test for the restore mechanism itself. + + This is exactly what broke the e2e `server` fixture + (tests/conftest.py): its forked child sets `PYPLET_PORT` in + `os.environ` before calling `astart()`, expecting `config.port` + to read it live. But an *earlier*, unrelated cli_test.py test + that did `original = config.port; ...; config.port = original` + to "clean up" left `config.port` permanently frozen in + `config.__dict__` — `Param.__set__` always writes the instance + override regardless of whether the value matches what was + there before. The server then bound the stale/default port + instead of the one the test fixture assigned, and the e2e suite + failed with "server not reachable". + + The fix (snapshotting/restoring the whole `config.__dict__`, + not reassigning a captured value) must actually remove an + override introduced mid-test, not just overwrite it with an + equal-looking value. + """ + from pyplet.server.config import config + + assert "port" not in config.__dict__ + snapshot = dict(config.__dict__) + try: - # --debug is never passed on the command line; only --port is. - with patch("sys.argv", ["pyplet", "start", "--port", "9999"]): - main() + # The naive pattern this regression guards against: + original_port = config.port + config.port = 12345 + config.port = original_port + assert "port" in config.__dict__ # naive reassign still froze it - assert config.port == 9999 - # config.debug must still be reading the env var live, not a - # frozen instance value. - assert "debug" not in config.__dict__ - assert config.debug == "1" + # The actual fix: clear + restore from the pre-test snapshot, + # instead of reassigning a captured resolved value. + config.__dict__.clear() + config.__dict__.update(snapshot) - monkeypatch.setenv("PYPLET_DEBUG", "0") - assert config.debug == "0" + assert "port" not in config.__dict__ finally: - config.port = original_port - config.__dict__.pop("debug", None) + config.__dict__.clear() + config.__dict__.update(snapshot) @pytest.mark.unit diff --git a/tests/package_test.py b/tests/package_test.py index f812bb2..9711f1b 100644 --- a/tests/package_test.py +++ b/tests/package_test.py @@ -34,9 +34,17 @@ class MyProjServer(ServerApplication): @pytest.fixture def restore_config_apps(): - original = config.apps + # Snapshot/restore the whole instance dict, not just `config.apps`'s + # resolved value: reassigning `config.apps = original` still calls + # `Param.__set__`, which unconditionally freezes an instance override + # into `config.__dict__` — permanently shadowing `PYPLET_APPS` for the + # rest of the process even when the restored value matches the + # pre-test one. Clearing and restoring the dict instead correctly + # undoes an override that didn't exist before the test. + original = dict(config.__dict__) yield - config.apps = original + config.__dict__.clear() + config.__dict__.update(original) class _FakeHandler: