Core improvements - #6
Merged
Merged
Conversation
Add `config.site_name` (`PYPLET_SITE_NAME`, default `"Pyplet"`) and read it at call time for every user-facing product-name literal: - `templates.py`: `<title>`, 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.
`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.
…aram 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)
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)
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>
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)
…cret 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)
`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.
`_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)
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)
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)
…ck_origin 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)
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.
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.
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.
``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).
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>
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.
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.
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.
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.
oauth: replace Google/Drive hardcoding with a provider registry See merge request seglab/pyplet!8
feat(config): make the displayed product name configurable See merge request seglab/pyplet!3
feat(auth): fail-closed startup, deny-by-default ACL, Secure cookie, JWKS verification See merge request seglab/pyplet!4
feat(server): production hardening — /healthz, WS frame cap, debug off, xheaders, check_origin See merge request seglab/pyplet!5
feat(server): routes() hook, WS-resolved login, permessage-deflate See merge request seglab/pyplet!6
fix(client): boot splash and micropip self-heal during the Pyodide bootstrap See merge request seglab/pyplet!7
…traversal) 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 27495cb)
…d94cdb) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019iFv2TeDxX86EDqgaQyqZG (cherry picked from commit 830839c)
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.
fix(security): confine app static route to per-app static/ dir (path traversal) See merge request seglab/pyplet!9
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.
test(ws): pin permessage-deflate on the shared ServerWebSocket See merge request seglab/pyplet!10
docs(readme): document the GitLab->GitHub one-way flow See merge request seglab/pyplet!12
`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>
`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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add Arthur's core improvements.