From 0cceefc890ad268a700daa0e485906e08a4784d2 Mon Sep 17 00:00:00 2001 From: magdev Date: Sun, 3 May 2026 16:31:54 +0200 Subject: [PATCH] v0.1.3: audit-driven non-breaking fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three bugs surfaced by the post-v0.1.2 architecture audit: - bridge.qml_path is now actually configurable. BridgeBundle::configure defines the qml_path scalar node (default ../qml/); loadExtension exposes it as the bridge.qml_path container parameter; services.yaml binds it into BridgeResourceMaker + BridgeWindowMaker. Apps override with `config/packages/bridge.yaml`. The existing maker docstrings claimed this worked already — they lied; now they don't. - SessionAuthenticator implements AuthenticationEntryPointInterface and routes the no-token entry-point path through the same problem+json helper as onAuthenticationFailure, so QML's RestClient sees one error shape regardless of which firewall path was taken. Test added. - CorrelationKeyListener::onTerminate guards on isMainRequest() now, matching onRequest's existing guard. No user-visible impact in worker mode (no sub-requests emitted), but the asymmetry was a defensive bug that would corrupt optimistic-update reconciliation. PLAN.md §13 gains a v0.1.3 section + folds the audit's API-surface items (PublisherInterface / ModelPublisherInterface / BridgeOp enum / maker DRY / DTO-shaped scaffold) into v0.2.0. CHANGELOG.md mirrors. PHPStan + cs-fixer + PHPUnit (17/17) + maker snapshot tests all green. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 13 +++++++++- PLAN.md | 24 +++++++++++++++++-- framework/php/config/services.yaml | 8 +++++++ framework/php/src/BridgeBundle.php | 13 +++++++--- .../CorrelationKeyListener.php | 6 +++++ .../php/src/Maker/BridgeResourceMaker.php | 4 ++-- framework/php/src/Maker/BridgeWindowMaker.php | 3 ++- framework/php/src/SessionAuthenticator.php | 22 +++++++++++++++-- .../php/tests/SessionAuthenticatorTest.php | 16 +++++++++++++ 9 files changed, 98 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 69e4cfd..0a285cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,16 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) - (none yet — next changes land here) +## [0.1.3] — TBD + +Non-breaking bugfixes from the post-v0.1.2 architecture audit. No behaviour change for existing apps; no public API additions or signature changes. + +### Fixed + +- **`bridge.qml_path` is now actually configurable.** The `BridgeResourceMaker` and `BridgeWindowMaker` docstrings claimed the QML scaffold path was settable via the bundle's `qml_path` option, but the bundle's `configure()` was empty and the constructor default (`'../qml/'`) was the only knob. `BridgeBundle::configure` now defines a `qml_path` scalar node; `loadExtension` exposes it as the `bridge.qml_path` container parameter; `services.yaml` binds it into both makers. Apps can override with `config/packages/bridge.yaml`: `bridge: { qml_path: ../qml/ }`. Default unchanged. +- **`SessionAuthenticator`: problem+json on the entry-point path.** `onAuthenticationFailure` already returned RFC 7807 `application/problem+json` for *bad-token* requests, but Symfony's default `AuthenticationEntryPointInterface::start` fired for *no-token* requests, returning a Form-flavoured 302/401 with the wrong shape for QML's `RestClient` error mapping. The authenticator now implements `AuthenticationEntryPointInterface` and routes both paths through a shared `problemJson()` helper so QML sees one error shape regardless of which firewall path was taken. New test covers the entry-point response. +- **`CorrelationKeyListener::onTerminate` sub-request guard.** `onRequest` already guarded with `isMainRequest()`, but `onTerminate` cleared unconditionally — a sub-request finishing mid-controller would wipe the main request's correlation key, causing the matching Mercure echo to lose its `correlationKey` field and the optimistic UI to never reconcile. FrankenPHP worker mode does not currently emit sub-requests so the user-visible impact is nil, but the asymmetry was a defensive bug. + ## [0.1.2] — TBD ### Fixed @@ -68,7 +78,8 @@ First public preview. Phases 0 through 4a in PLAN.md are complete plus the Phase - The bundle ships without `composer.lock` (it's a library); the skeleton and the todo example carry their own. - Licensed under **LGPL-3.0-or-later** (`LICENSE` + `LICENSE.GPL` at the repo root). Chosen to align with Qt 6's LGPLv3 licensing — see PLAN.md §12 for the relinkability obligations the AppImage build already honours. -[Unreleased]: https://src.bundespruefstelle.ch/magdev/php-qml/compare/v0.1.2...HEAD +[Unreleased]: https://src.bundespruefstelle.ch/magdev/php-qml/compare/v0.1.3...HEAD +[0.1.3]: https://src.bundespruefstelle.ch/magdev/php-qml/releases/tag/v0.1.3 [0.1.2]: https://src.bundespruefstelle.ch/magdev/php-qml/releases/tag/v0.1.2 [0.1.1]: https://src.bundespruefstelle.ch/magdev/php-qml/releases/tag/v0.1.1 [0.1.0]: https://src.bundespruefstelle.ch/magdev/php-qml/releases/tag/v0.1.0 diff --git a/PLAN.md b/PLAN.md index 2600e5b..2a2a37b 100644 --- a/PLAN.md +++ b/PLAN.md @@ -1,6 +1,6 @@ # php-qml — Plan for a Symfony/FrankenPHP + Qt/QML Desktop Framework -> **Status (2026-05):** v0.1.0 + v0.1.1 shipped 2026-05-03 (LGPL-3.0-or-later). v0.1.2 in progress on `dev`. Planning is version-based — see §13. +> **Status (2026-05):** v0.1.0 + v0.1.1 shipped 2026-05-03 (LGPL-3.0-or-later). v0.1.2 + v0.1.3 in progress on `dev`. Planning is version-based — see §13. > > **Where else to look:** > @@ -545,9 +545,29 @@ Closed the four shakedown follow-ups identified during v0.1.0 shipping: - **Bundled supervisor: clean child shutdown.** The destructor's `teardownChild()` only ran during stack unwinding *after* `app.exec()` returned, by which point Qt's event loop was already mid-shutdown — so `QProcess::waitForFinished` couldn't reliably reap the child and Qt warned `QProcess: Destroyed while process is still running`, leaving an orphan frankenphp + its workers behind. Fix: connect `QCoreApplication::aboutToQuit` to `teardownChild` in the constructor, so the child is SIGTERM'd while the event loop is still active. Bundled-supervisor integration test gained a clean-shutdown assertion (no Qt warning, no orphan frankenphp under the host's PGID after SIGTERM). +### v0.1.3 — in progress on `dev` + +Non-breaking bugfixes surfaced by the post-v0.1.2 architecture audit: + +- **`bridge.qml_path` is now actually configurable.** `BridgeResourceMaker` and `BridgeWindowMaker` carried docstrings claiming the QML output dir was settable via the bundle's `qml_path` option, but the bundle never wired one — the constructor default was the only knob. `BridgeBundle::configure` now defines a `qml_path` node (default `../qml/`); `loadExtension` exposes it as the `bridge.qml_path` container parameter; `services.yaml` binds it into both makers. Apps configure with `config/packages/bridge.yaml`: `bridge: { qml_path: ../qml/ }`. Default is unchanged so existing skeleton/example apps need no edit. +- **`SessionAuthenticator` problem+json on entry-point path.** `onAuthenticationFailure` already returned RFC 7807 `application/problem+json` for *bad-token* requests, but Symfony's default entry point fired for *no-token* requests — yielding a Form-flavoured 302/401 instead. Implemented `AuthenticationEntryPointInterface::start`, factored the response into a `problemJson()` helper, so QML's RestClient sees one shape regardless of which path the firewall takes. Added test coverage. +- **`CorrelationKeyListener::onTerminate` sub-request guard.** `onRequest` already had `isMainRequest()`, but `onTerminate` cleared unconditionally — so a sub-request finishing mid-controller would wipe the main request's correlation key, causing the matching Mercure echo to lose its `correlationKey` field and the optimistic UI to never reconcile. Defensive: real-world impact is low (FrankenPHP worker mode does not currently emit sub-requests), but cheap to fix and the obvious correctness bug. + ### v0.2.0 — next minor -Pulls in the originally-Phase-3/5-deferred items that don't need new operational dependencies, plus the smaller §12 risks. Cross-platform packaging is parked at v0.9.0 — the operational lift (self-hosted runners + platform certs) is too big to fold into the next minor and Linux remains the primary target until the framework's surface stabilises. +Pulls in the originally-Phase-3/5-deferred items that don't need new operational dependencies, plus the smaller §12 risks **and** the public-API / DX items surfaced by the post-v0.1.2 audit. Cross-platform packaging is parked at v0.9.0 — the operational lift (self-hosted runners + platform certs) is too big to fold into the next minor and Linux remains the primary target until the framework's surface stabilises. + +**Public-API surface (audit-driven, breaks pre-1.0 SemVer permitted):** + +- **Ship interfaces for the bridge's three public services.** `Publisher`, `ModelPublisher`, and `CorrelationContext` are typehinted concretely everywhere (the Doctrine listener, the example `PingController`, every user controller that wants to fire a manual envelope) — the matching upstream Symfony idiom is `HubInterface` / `EventDispatcherInterface` / `NormalizerInterface`. Extract `PublisherInterface`, `ModelPublisherInterface`, `CorrelationContextInterface`; have the concrete classes implement them; switch every internal typehint over; document the interfaces as the public contract. Lets app code mock at the seam without a concrete-class spy and lets us iterate the implementations behind the contract. +- **`BridgeOp` enum.** `'upsert'` / `'delete'` are passed as raw strings between `DoctrineBridgeListener` and `ModelPublisher::publishEntityChange`. PHP 8.1 backed enum is the obvious typed replacement; PLAN.md §4's envelope `op` field already enumerates `upsert` | `delete` | `replace` | `event` so the enum encodes a documented contract. Method signature change is API-visible — pre-1.0 SemVer permits it; ship deprecation paths if the audit surfaces external callers. +- **`HealthController` deep-load canary refactor.** Constructor-injects `Publisher` only as a "is the bundle resolvable" probe (added in v0.1.1). Switching the dependency to a tiny `BridgeBundleInfo` value object that the bundle registers documents intent and decouples `/healthz` from the publisher contract — important once `PublisherInterface` lands. + +**Maker DRY + DX (audit-driven):** + +- **Maker shared helpers.** All three makers re-implement the same name-prompt-or-fail closure (`ucfirst(trim(…))` plus throw on empty) and re-spell their own camel-to-snake / camel-to-kebab regexes inline. Extract `Maker\Support\NameInput::askOrFail()` and `Maker\Support\Naming::camelTo($name, '_'|'-')` — single source of truth, three call sites. +- **DTO-shaped controller scaffold (`make:bridge:resource --with-dto`).** Generated CRUD controllers currently accept any JSON shape: `if (isset($data['title'])) …` with silent type coercion, no required-field enforcement, malformed JSON swallowed as `?? []`. Add a `--with-dto` option that emits `CreateDto` + `UpdateDto` DTOs alongside the controller and rewrites the action signatures to `#[MapRequestPayload] CreateTodoDto $dto`. Pulls `symfony/validator` into the skeleton/example dependencies; `#[Assert\NotBlank]` on title fields is the headline default. Symfony's payload-mapping infrastructure produces RFC 7807 problem+json on validation failure for free, fixing the field-mapping repetition between `create()` and `update()` at the same time. Once stable, flip `--with-dto` to default-on. +- **Generated controller `findOr404` boilerplate.** `update()` and `delete()` both inline the find-or-404 problem+json response. Either factor a private helper into the template or migrate to Symfony's `#[MapEntity]` attribute (ships in 7.x). **Makers + reactive types (Phase 3.x deferred):** diff --git a/framework/php/config/services.yaml b/framework/php/config/services.yaml index db06849..91fd1d3 100644 --- a/framework/php/config/services.yaml +++ b/framework/php/config/services.yaml @@ -11,3 +11,11 @@ services: PhpQml\Bridge\SessionAuthenticator: arguments: $expectedToken: '%env(default::BRIDGE_TOKEN)%' + + PhpQml\Bridge\Maker\BridgeResourceMaker: + arguments: + $qmlPath: '%bridge.qml_path%' + + PhpQml\Bridge\Maker\BridgeWindowMaker: + arguments: + $qmlPath: '%bridge.qml_path%' diff --git a/framework/php/src/BridgeBundle.php b/framework/php/src/BridgeBundle.php index 44a887a..08edfc9 100644 --- a/framework/php/src/BridgeBundle.php +++ b/framework/php/src/BridgeBundle.php @@ -12,16 +12,23 @@ use Symfony\Component\HttpKernel\Bundle\AbstractBundle; final class BridgeBundle extends AbstractBundle { /** - * @param array $config + * @param array{qml_path?: string} $config */ public function loadExtension(array $config, ContainerConfigurator $container, ContainerBuilder $builder): void { + $builder->setParameter('bridge.qml_path', $config['qml_path']); $container->import(__DIR__.'/../config/services.yaml'); } public function configure(DefinitionConfigurator $definition): void { - // Bundle config tree gains nodes when bridge:doctor and the - // skeleton's wiring need settable knobs (Phase 1 sub-commits 3 & 6). + $definition->rootNode() + ->children() + ->scalarNode('qml_path') + ->info('Where make:bridge:resource and make:bridge:window write QML scaffolds. Path is resolved relative to the Symfony project dir.') + ->defaultValue('../qml/') + ->cannotBeEmpty() + ->end() + ->end(); } } diff --git a/framework/php/src/EventSubscriber/CorrelationKeyListener.php b/framework/php/src/EventSubscriber/CorrelationKeyListener.php index f1ed839..f728bd7 100644 --- a/framework/php/src/EventSubscriber/CorrelationKeyListener.php +++ b/framework/php/src/EventSubscriber/CorrelationKeyListener.php @@ -43,6 +43,12 @@ final class CorrelationKeyListener implements EventSubscriberInterface public function onTerminate(TerminateEvent $event): void { + // Sub-requests share the kernel's correlation context with the main + // request — clearing on a sub-request's TerminateEvent would wipe the + // key while the main controller is still running. + if (!$event->isMainRequest()) { + return; + } $this->context->clear(); } } diff --git a/framework/php/src/Maker/BridgeResourceMaker.php b/framework/php/src/Maker/BridgeResourceMaker.php index f41a750..5bafc45 100644 --- a/framework/php/src/Maker/BridgeResourceMaker.php +++ b/framework/php/src/Maker/BridgeResourceMaker.php @@ -33,8 +33,8 @@ use Symfony\Component\Uid\Uuid; * * The Doctrine subscriber installed by the bundle picks the entity up * automatically — no per-resource listener is generated. The QML snippet - * goes to `qml_path` (default: `../qml/`, configurable via the bundle's - * `qml_path` option in services.yaml). + * goes to `qml_path` (default: `../qml/`, set via `config/packages/bridge.yaml`: + * `bridge: { qml_path: ../qml/ }`). * * See PLAN.md §8 (*Custom makers*). */ diff --git a/framework/php/src/Maker/BridgeWindowMaker.php b/framework/php/src/Maker/BridgeWindowMaker.php index 571c407..0c562e0 100644 --- a/framework/php/src/Maker/BridgeWindowMaker.php +++ b/framework/php/src/Maker/BridgeWindowMaker.php @@ -21,7 +21,8 @@ use Symfony\Component\Console\Input\InputInterface; * the first window and as many extra instances as it wants for the * multi-window test from PLAN.md §9 / §13 Phase 3. * - * Generated file goes to `qml_path` (default: `../qml/`). + * Generated file goes to `qml_path` (default: `../qml/`, set via + * `config/packages/bridge.yaml`: `bridge: { qml_path: ../qml/ }`). */ final class BridgeWindowMaker extends AbstractMaker { diff --git a/framework/php/src/SessionAuthenticator.php b/framework/php/src/SessionAuthenticator.php index 4f1b5d4..c08371e 100644 --- a/framework/php/src/SessionAuthenticator.php +++ b/framework/php/src/SessionAuthenticator.php @@ -13,6 +13,7 @@ use Symfony\Component\Security\Http\Authenticator\AbstractAuthenticator; use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; use Symfony\Component\Security\Http\Authenticator\Passport\Passport; use Symfony\Component\Security\Http\Authenticator\Passport\SelfValidatingPassport; +use Symfony\Component\Security\Http\EntryPoint\AuthenticationEntryPointInterface; /** * Validates the per-session bearer token shared between the Qt host @@ -22,7 +23,7 @@ use Symfony\Component\Security\Http\Authenticator\Passport\SelfValidatingPasspor * Qt host generates it per session and passes it to FrankenPHP via env. * See PLAN.md §3 (*Run modes*, *Edge cases — Per-session secret rotation*). */ -final class SessionAuthenticator extends AbstractAuthenticator +final class SessionAuthenticator extends AbstractAuthenticator implements AuthenticationEntryPointInterface { public function __construct( #[\SensitiveParameter] @@ -57,13 +58,30 @@ final class SessionAuthenticator extends AbstractAuthenticator } public function onAuthenticationFailure(Request $request, AuthenticationException $exception): Response + { + return $this->problemJson($exception->getMessage()); + } + + /** + * Entry point invoked when access is denied without a triggered authenticator + * (e.g. an anonymous request to a protected route). Without this, Symfony + * returns its default `WWW-Authenticate: Form` 302/401, which clients + * speaking JSON would never expect — same shape as onAuthenticationFailure + * keeps QML's RestClient error mapping consistent across both paths. + */ + public function start(Request $request, ?AuthenticationException $authException = null): Response + { + return $this->problemJson($authException?->getMessage() ?? 'Bearer token required.'); + } + + private function problemJson(string $detail): JsonResponse { return new JsonResponse( [ 'type' => 'about:blank', 'title' => 'Unauthorized', 'status' => Response::HTTP_UNAUTHORIZED, - 'detail' => $exception->getMessage(), + 'detail' => $detail, ], Response::HTTP_UNAUTHORIZED, ['Content-Type' => 'application/problem+json'], diff --git a/framework/php/tests/SessionAuthenticatorTest.php b/framework/php/tests/SessionAuthenticatorTest.php index 76e703f..54742a1 100644 --- a/framework/php/tests/SessionAuthenticatorTest.php +++ b/framework/php/tests/SessionAuthenticatorTest.php @@ -82,4 +82,20 @@ final class SessionAuthenticatorTest extends TestCase self::assertSame(401, $body['status']); self::assertSame('Unauthorized', $body['title']); } + + public function testStartReturnsProblemJsonForAnonymousAccess(): void + { + // Entry-point path: no Authorization header → supports() returns false → + // Symfony invokes start() with no exception. Without our start(), the + // default would be a Form-flavoured 302/401 — wrong shape for QML. + $auth = new SessionAuthenticator('s3cret'); + $response = $auth->start(new Request()); + + self::assertSame(Response::HTTP_UNAUTHORIZED, $response->getStatusCode()); + self::assertSame('application/problem+json', $response->headers->get('Content-Type')); + $body = json_decode((string) $response->getContent(), true); + self::assertSame(401, $body['status']); + self::assertSame('Unauthorized', $body['title']); + self::assertSame('Bearer token required.', $body['detail']); + } }