diff --git a/CHANGELOG.md b/CHANGELOG.md index 054ea778..ffbdc3b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,80 @@ All notable changes to LaraFly are documented here. This project uses CalVer (`Y ## [Unreleased] +## [26.09.10] - 2026-09-29 + +### Fixed + +- **OpenAPI contracts:** derive path-pattern `404` responses and restrict inferred validation `422` responses + to typed request bodies. Included page controllers now preserve their actual JSON, HTML or redirect return + contract. Browser coverage checks every generated operation, tag, response and component, including nested + schemas and keyboard access on phones. +- **OpenAPI readability:** improve local Swagger text contrast for method and version badges, links, actions, + code examples, schema controls and constraints; verify expanded schemas with either system color preference. + Normalize native schema buttons and wrap operation controls to prevent phone overflow in WebKit. +- **Error pages:** shorten paths in symlinked deployments and test harnesses, bound the debug stack before + rendering, and group dependency frames behind a native disclosure. Frame summaries stay on one line on desktop and give calls a second line on phones; + text contrast meets 4.5:1 in both themes. The production facts grid has no empty colored cells and shows + the reference once, with selectable text and an optional clipboard enhancement. +- **Error URL safety:** validate configured home, sign-in, support and problem-type URLs at construction; + reject unsafe schemes, authority-relative paths and interior URL control characters. +- **Problem documents:** substitute invalid UTF-8 and return a degraded document if encoding or a serialization + callback fails. Wildcard, absent and unsupported Accept types receive problem+json while error-page + fallback is enabled. Laravel's own validation, authentication and carried-response exceptions retain + their native handling. +- **Migration:** problem `instance` values now begin with `/`, with unsafe path characters percent-encoded. + Clients comparing the old relative path must account for the leading slash. + +- **Admin listings:** replace competing column rules with typed columns, fixed table layout and explicit + colgroups. Route paths no longer collapse into stacks of characters beside unused space. Rigid column + widths include cell padding and allow for Linux header-font metrics, and a bounded table scrollport makes + sticky headers work. +- **Stable paging:** append an ascending identity tiebreak so tied rows cannot move between pages. Clamp + stale out-of-range pages to the last page; a SQL-backed data listing may need one additional query. + +### Added + +- **Route detail:** permalinked, server-rendered route contracts with ordered caller/injected bindings, + resolver claims, binding-specific failures, bounded DTO trees, sibling comparison and duplicate warnings. + Gated wiring/configuration/API/traffic links, default responses, exception handlers, registered route + metadata and collapsed advice provenance make the compiled contract inspectable without running it. + `firefly.admin.routes.detail` and `firefly.admin.routes.advice` control the new surface. +- **Bean explorer:** server-rendered landing/search/focus/module states, native keyboard links, bounded hop + columns, exact overflow links, complete paginated catalogue and relations, module coupling metrics, + conditions and shortest entry-point chains. Iterative SCC analysis handles deep graphs and self-cycles. +- **Bean graph truthfulness:** stable competing factory identities across configurations, explicit unresolved + ambiguity instead of an arbitrary target, unknown factory scope and exclusion of unbound config DTOs. +- **Explorer settings:** focus depth/row/node/path/page budgets, starter/module budgets and catalogue page size. + The legacy `firefly.admin.graph.max-nodes` is still parsed but no longer controls drawing; **0 no longer + forces a list**. Use the catalogue or relation tables for tabular exploration. + +- **Error navigation:** configured sign-in on 401, retry on GET/HEAD 5xx, and home/support links where + configured. Production ledes retain safe authored details and 405 pages name the allowed methods. +- **Error configuration:** documented `max-frames`, `home`, `sign-in`, `support`, `actions`, `copy-button`, + `authored-detail`, `problem-fallback` and `problem.type-uri` settings in the reference and module guide. +- **RFC 9457 type:** `about:blank` by default, a code-derived URI when an HTTP(S) base is configured, or an + omitted member when the setting is empty. Omitting `type` does not revert the corrected `instance` path. + +- **Shared listing controls:** server-side paging, sorting and searching with validated, bookmarkable URL + state across routes, beans, conditions, scheduled tasks, configuration, runtime and data listings. Paired + listings preserve each other's state, and row-count controls have submit buttons for use without JavaScript. +- **Table settings:** `firefly.admin.table.page-size`, `page-sizes`, `max-page-size`, `max-height`, `density` + and `remember-scroll`. The offered size set is closed; the data browser applies its own bounds in series. + Auto-refresh keeps URL state; optional per-URL scroll restoration applies on reload and back/forward. + +### Changed + +- **`packages/admin` — the data browser's rows-per-page control offers the dashboard's set, and a `?size=` + outside it is refused rather than lowered.** `/firefly/data` used to draw its own ` - + {{-- + WHERE THE READER WAS. This form is a POST, so none of the listing's + state survives it on its own — and AdminAction::setLoggerLevel() + redirects, which throws away the request's query string too. One + hidden field carrying `$query->link()` (the search, the ordering, + the size and the page, exactly as the pager writes them) is what + returns someone who filtered for `queue` on page 3 to page 3 of + `queue` instead of to the top of the unfiltered table. The action + validates it against this page's own URL before redirecting to it. + --}} + + @@ -48,6 +53,7 @@ + @include('firefly-admin::_pager', ['slice' => $slice, 'query' => $query]) @endif diff --git a/packages/admin/resources/views/mapping.blade.php b/packages/admin/resources/views/mapping.blade.php new file mode 100644 index 00000000..4447a6f6 --- /dev/null +++ b/packages/admin/resources/views/mapping.blade.php @@ -0,0 +1,142 @@ +@extends('firefly-admin::layout') +@section('title', $detail->route->httpMethod.' '.$detail->route->path) +@section('body') + @php + use Firefly\Admin\Format; + use Firefly\Admin\Route\RouteBodyNode; + use Firefly\Admin\Route\RouteInspector; + $route = $detail->route; + $back = $query->link(); + @endphp +
+ ← Back to routes +

{{ $route->httpMethod }} {{ $route->path }}

+

{{ $route->controllerClass }}::{{ $route->methodName }}() · {{ $route->name ?? 'Unnamed route' }}

+ + @if ($detail->siblings !== []) +
+ Same controller · {{ count($detail->siblings) }} other registrations + +
+ @endif +
+
+
Default success status
{{ $route->status }}
+
HTML stereotype
{{ $route->html ? 'Declared' : 'No / legacy' }}
+
Caller arguments
{{ count($detail->caller) }}
+
Injected arguments
{{ count($detail->injected) }}
+
Route source
{{ $bootMode }}
+
+ @if ($manifestTime !== false)

Route artifact modified {{ Format::since($manifestTime, $now) }}. Rebuild with php artisan firefly:cache after changing declarations.

@endif + @if (count($detail->registrations) > 1) +
+

Duplicate registrations

+

The last registration wins for this verb and path. Earlier handlers are shadowed.

+
    @foreach ($detail->registrations as $registration)
  1. {{ $loop->last ? 'Effective' : 'Shadowed' }} · {{ $registration->controllerClass }}::{{ $registration->methodName }}()
    2. @endforeach
+
+ @endif +
+

Request · what the caller sends

Signature positions are preserved. Defaults are compiled attribute values, not necessarily PHP signature defaults. Null does not establish nullability; an unrecorded type may be union, intersection or untyped.

+ @if ($detail->caller === [])

No caller-supplied arguments in the compiled contract.

+ @else +
+ + @include('firefly-admin::_table-head', ['view' => $bindingView, 'query' => null]) + @foreach ($detail->caller as $binding) + + + + + + + + + + + @endforeach +
{{ $binding->position }}${{ $binding->plan['name'] }}{{ $binding->plan['kind'] }}{{ $binding->plan['key'] }}{{ Format::leafOf($binding->plan['type'] ?? 'not recorded') }}{{ Format::stemOf($binding->plan['type'] ?? '') }}{{ $binding->plan['required'] ? 'yes' : 'no' }}{{ $binding->defaultLabel() }}{{ $binding->plan['valid'] ? 'yes' : 'no' }}
+
+ @foreach ($detail->caller as $binding) + @if ($binding->notFoundMessage !== null) +

${{ $binding->plan['name'] }} pattern ^(?:{{ $binding->plan['pattern'] }})$ (case-insensitive). A mismatch returns 404 {{ $binding->plan['notFoundCode'] ?? 'RESOURCE_NOT_FOUND' }}: {{ $binding->notFoundMessage }}

+ @endif + @endforeach + @endif +
+
+

Before the controller · possible binding failures

+

Framework defaults derived from this contract. Exception handlers may replace the response or status. Resolver-specific failures are not inferred.

+ @if ($detail->failures === [])

No default binding failures are implied by these arguments.

+ @else@endif +
+ {{-- Services are collaborators, never request parameters. A resolver claim overrides the compiled kind. --}} +
+

Handler arguments · what the container supplies

+ @if ($detail->injected === [])

No injected arguments in this signature.

+ @else
    @foreach ($detail->injected as $binding) +
  1. ${{ $binding->plan['name'] }} · {{ $binding->plan['type'] ?? 'not recorded (union, intersection or untyped)' }} + @if ($binding->resolver !== null)Resolver claimed

    Supplied by {{ $binding->resolver }}; compiled kind {{ $binding->plan['kind'] }} is overridden.

    + @elseContainer service@endif + @if (array_key_exists('nullable', $binding->plan)) · {{ $binding->plan['nullable'] ? 'may be null' : 'non-null' }}@endif +
    2. + @endforeach
@endif +
+ @foreach ($detail->caller as $binding) + @if ($binding->plan['kind'] === 'body') +
+

Request body · ${{ $binding->plan['name'] }}

+

{{ $binding->plan['valid'] && $binding->plan['type'] !== null ? 'Validated before hydration; validation can answer 422.' : 'No body validation is declared.' }} Validation rules are not part of the route manifest.

+ @php $bodyNodes = RouteBodyNode::forBinding($binding); @endphp + @if ($bodyNodes !== [])@include('firefly-admin::_route-body', ['nodes' => $bodyNodes, 'depth' => 0]) + @else

{{ $binding->plan['type'] === null || ! class_exists($binding->plan['type']) ? 'The decoded array is passed through.' : 'No constructor properties are recorded in this manifest.' }}

@endif +
+ @endif + @endforeach +
+

Response

+

Default success status {{ $route->status }}. A returned response can override it.

+

{{ $route->html ? 'The manifest records the #[Controller] HTML stereotype. This is declaration metadata, not a media-type guarantee.' : 'No HTML stereotype is recorded. Older manifests may omit this metadata.' }}

+

The returned value and Accept determine the response. Views and HTML-capable values can render as HTML; data is negotiated; returned responses retain their own headers.

+ @if ($route->html)

HTML controllers are excluded from the OpenAPI document unless firefly.openapi.include-html is enabled.

@endif +

Exception handlers

+

Controller-local handlers take precedence over global handlers; within that scope, the most-derived matching exception class wins.

+ @if ($handlers === [])

No matching local or global exception handlers are registered.

+ @else@endif +
+ @if ($settings->routeAdvice) +
Advice · compiled contract and runtime bindings +

Source: {{ $adviceSource }}. Listed outermost first. LIVE means the interceptor is bound; it does not prove a request executed it. UNBOUND advice can fail proxy creation unless explicitly permitted to be INERT.

+ @if ($advice === [])

No method advice is recorded in the available plan.

+ @else
    @foreach ($advice as $item)
  1. {{ $item['id'] }} · order {{ $item['order'] }} · {{ $item['state'] }}

    {{ $item['interceptor'] }}

    Declared settings and meter names
    {{ $item['contract'] }}
    2. @endforeach
@endif +

Meter names are declarations; metric totals belong to the application, not this route.

+
+ @endif + @if ($metadata !== null) +
As Laravel registered it +

URI {{ $metadata['uri'] }} · name {{ $metadata['name'] ?? 'unnamed' }} · domain {{ $metadata['domain'] ?? 'any host' }}

+

Declared route middleware: {{ implode(', ', $metadata['middleware']) ?: 'none' }}. Global middleware is not included.

+ @foreach ($metadata['patterns'] as $name => $pattern)

{{ $name }} constraint: {{ $pattern }}

@endforeach +
+ @endif +
Dispatch order and inspection limits +
  1. Resolve the controller.
  2. Bind arguments in signature order: +
      @foreach ($detail->arguments() as $binding)
    1. ${{ $binding->plan['name'] }} · {{ $binding->resolver !== null ? 'registered resolver' : ($binding->plan['kind'] === 'service' ? 'container service' : $binding->plan['kind'].' binding') }}
      2. @endforeach
    + See binding failures above.
  3. Check controller authorization after arguments are available.
  4. Invoke the handler and apply after-invocation checks.
  5. Build the response with default status {{ $route->status }}.
+ {{-- Proxy security absence is not an authorization verdict; URL rules alone cannot establish public access. --}} +

Security authorization is not inferred from this page. Controller and method checks, URL rules and the concrete request may all affect access.

+

No handler, argument resolver or synthetic request is executed by this inspection. HTTP traffic is a separate application-wide view.

+
+@endsection +@push('scripts') + +@endpush diff --git a/packages/admin/resources/views/mappings.blade.php b/packages/admin/resources/views/mappings.blade.php index ed4d27bb..57f19b37 100644 --- a/packages/admin/resources/views/mappings.blade.php +++ b/packages/admin/resources/views/mappings.blade.php @@ -11,31 +11,40 @@
@include('firefly-admin::_panel-head', [ - 'title' => 'Mappings', 'count' => count($mappings), - 'filter' => 'map-body', 'placeholder' => 'Filter by path or handler…', + 'title' => 'Mappings', 'count' => $slice->total, 'query' => $query, + 'placeholder' => 'Search by path or handler…', ]) - @if ($mappings === []) - @include('firefly-admin::_empty', [ - 'title' => 'No routes mapped', - 'body' => 'Create one with php artisan make:firefly-controller, then re-run firefly:cache if this application boots compiled.', - ]) + @if ($slice->isEmpty()) + @include('firefly-admin::_empty', $query->isFiltered() + ? ['title' => 'Nothing matches', 'body' => 'No route\'s path, handler or name contains that. Show them all.'] + : ['title' => 'No routes mapped', 'body' => 'Create one with php artisan make:firefly-controller, then re-run firefly:cache if this application boots compiled.']) @else
- - - - @foreach ($mappings as $route) - @php $handler = is_string($route['handler'] ?? null) ? $route['handler'] : ''; @endphp +
MethodPathHandlerName
+ @include('firefly-admin::_table-head', ['view' => $view, 'query' => $query]) + + @foreach ($slice->rows as $route) - - - - + + + + @endforeach
{{ $route['httpMethod'] ?? '' }}{{ $route['path'] ?? '' }}{{ Format::shortClass($handler) }}{{ rtrim(Format::namespaceOf($handler), '\\') }}{{ $route['name'] ?: '—' }}{{ $route['httpMethod'] }} + @if ($settings->routeDetail) + Inspect {{ $route['httpMethod'] }} {{ $route['path'] }} + @else + {{ $route['path'] }} + @endif + @if ($route['shadowed'])Shadowed@endif + + {{ Format::leafOf($route['handler']) }} + {{ Format::stemOf($route['handler']) }} + {{ $route['name'] ?: '—' }}
+ @include('firefly-admin::_pager', ['slice' => $slice, 'query' => $query]) @endif
@endsection diff --git a/packages/admin/resources/views/metrics.blade.php b/packages/admin/resources/views/metrics.blade.php index 7703e96e..19cb7abb 100644 --- a/packages/admin/resources/views/metrics.blade.php +++ b/packages/admin/resources/views/metrics.blade.php @@ -1,10 +1,7 @@ @extends('firefly-admin::layout') @section('title', 'Metrics') @section('body') - @php - $peak = 0.0; - foreach ($metrics as $metric) { foreach ($metric['rows'] as $row) { $peak = max($peak, abs($row['value'])); } } - @endphp + @php use Firefly\Admin\Format; @endphp

Metrics

@@ -14,34 +11,46 @@
@include('firefly-admin::_panel-head', [ - 'title' => 'Meters', 'count' => count($metrics), - 'filter' => 'metrics-body', 'placeholder' => 'Filter meters…', + 'title' => 'Meters', 'count' => $slice->total, 'query' => $query, 'placeholder' => 'Search meters…', ]) - @if ($metrics === []) - @include('firefly-admin::_empty', [ - 'title' => 'Nothing recorded yet', - 'body' => 'The default registry keeps meters in process memory, so under PHP-FPM a page only ever sees its own request. Set firefly.observability.metrics.store to a cache store to accumulate across workers.', - ]) + @if ($slice->isEmpty()) + @include('firefly-admin::_empty', $query->isFiltered() + ? ['title' => 'Nothing matches', 'body' => 'No meter name or statistic contains that. Show them all.'] + : ['title' => 'Nothing recorded yet', 'body' => 'The default registry keeps meters in process memory, so under PHP-FPM a page only ever sees its own request. Set firefly.observability.metrics.store to a cache store to accumulate across workers.']) @else
- - - - @foreach ($metrics as $metric) +
MeterStatisticValueRelative
+ @include('firefly-admin::_table-head', ['view' => $view, 'query' => $query]) + + @foreach ($slice->rows as $metric) + {{-- ONE ROW PER MEASUREMENT, ONE SLICE PER METER. The listing pages meters, so a + meter's statistics are drawn together however many there are, and only the + first of them names the meter — the blank cells under it are what says "these + readings are of one thing". --}} @forelse ($metric['rows'] as $row) - - - - + + + @empty - + @endforelse @@ -49,6 +58,7 @@
{{ $loop->first ? $metric['name'] : '' }}{{ $row['statistic'] }}{{ $row['display'] }} - {{-- One shared scale across every meter: the bar answers "which of these is - large", which is the only comparison a mixed-unit list supports. --}} + + @if ($loop->first) + {{ Format::leafOf($metric['name'], '.') }} + {{ Format::stemOf($metric['name'], '.') }} + @endif + {{ $row['statistic'] }}{{ $row['display'] }} + {{-- One shared scale across every meter, and across every PAGE of them: + the bar answers "which of these is large", which is the only + comparison a mixed-unit list supports, and an answer that changed + when the reader turned the page would not be one. --}}
{{ $metric['name'] }} + {{ Format::leafOf($metric['name'], '.') }} + {{ Format::stemOf($metric['name'], '.') }} + no measurements
+ @include('firefly-admin::_pager', ['slice' => $slice, 'query' => $query]) @endif
@endsection diff --git a/packages/admin/resources/views/oauth2.blade.php b/packages/admin/resources/views/oauth2.blade.php index ab6b9909..9a70f3f9 100644 --- a/packages/admin/resources/views/oauth2.blade.php +++ b/packages/admin/resources/views/oauth2.blade.php @@ -3,9 +3,8 @@ @section('body') @php // Whether the Active counts came from a per-process store (the endpoint's own - // `authorizations.processLocal`). Defaulted rather than assumed, so a payload that does not say is - // rendered without a caveat instead of with one nothing substantiates. - $processLocal = ($processLocalAuthorizations ?? false) === true; + // `authorizations.processLocal`), decided in AdminAction and carried here for the note below. + $processLocal = $processLocalAuthorizations; @endphp
@@ -17,81 +16,65 @@
@include('firefly-admin::_panel-head', [ - 'title' => 'Clients', 'count' => count($clients), - 'filter' => 'oauth2-body', 'placeholder' => 'Filter by client id, grant or scope…', + 'title' => 'Clients', 'count' => $slice->total, 'query' => $query, + 'placeholder' => 'Search by client id, grant or scope…', ]) - @if ($clients === []) - @include('firefly-admin::_empty', [ - 'title' => 'No clients registered', - 'body' => 'Add a block under firefly.security.oauth2.server.clients, or switch clients.driver to eloquent and register clients dynamically.', - ]) + @if ($slice->isEmpty()) + @include('firefly-admin::_empty', $query->isFiltered() + ? ['title' => 'Nothing matches', 'body' => 'No client id, name, grant or scope contains that. Show them all.'] + : ['title' => 'No clients registered', 'body' => 'Add a block under firefly.security.oauth2.server.clients, or switch clients.driver to eloquent and register clients dynamically.']) @else
- - +
ClientAuthenticationGrantsScopesRedirect URIsTokensActive
+ @include('firefly-admin::_table-head', ['view' => $view, 'query' => $query]) - @foreach ($clients as $client) - @php - // Assembled here rather than with inline @if fragments: Blade only compiles a - // directive that is NOT glued to a word character, so `…}}s@if(…) · PKCE@endif` - // silently leaves an unclosed `if` in the compiled view. - $issuance = [(string) ($client['accessTokenFormat'] ?? ''), ($client['accessTokenTtl'] ?? 0).'s']; - // `requiresProofKey` (what the endpoints enforce), never `requireProofKey` (the - // switch the client registered): `require_pkce` and - // `require_proof_key_for_public_clients` both default to on, so the registered - // switch reads "no PKCE" for a client whose every authorization request is in fact - // refused without a code_challenge — the first thing this page is opened to explain. - // - // Strictly `=== true`, because both keys are `null` for a client with no - // authorization_code grant: PKCE and consent are that path's rules, so a machine - // client sends no code_challenge and reaches no consent screen, and the endpoint - // says "does not apply" rather than reporting a default nothing enforces. A - // truthy test would print both labels beside it and send the operator checking - // two requirements that are not there. - if (($client['requiresProofKey'] ?? null) === true) { - $issuance[] = 'PKCE'; - } - if (($client['requireAuthorizationConsent'] ?? null) === true) { - $issuance[] = 'consent'; - } - $active = is_numeric($client['activeAuthorizations'] ?? null) ? (int) $client['activeAuthorizations'] : 0; - // A `0` counted in a per-process store is the one cell that reads as a fact and is - // not one: the workers beside this one may be holding a hundred live - // authorizations for this client, and an operator who reads "none" goes looking - // for a token endpoint that is refusing nobody. Shown as `—` — nothing counted - // here — while a non-zero count is kept, because that one is a floor the store - // can vouch for. The note under the table names the key that makes it server-wide. - $activeCell = $processLocal && $active === 0 ? '—' : (string) $active; - @endphp + @foreach ($slice->rows as $client) - - - - - - - + {{-- The client name is `class="ns"` WITHOUT `stem`: it is a human name, not a + qualified prefix, so it elides from the right like any prose rather than + from the left like a namespace. --}} + + + + + + + {{-- The `—` for a count no per-process store can vouch for is DRAWN HERE, over + the empty string AdminAction leaves in the row, exactly like the absent + cells above it. The row keeps a number so the column can be ordered as + one; see AdminAction::clientRows() for the page of em-dashes an em-dash in + the row would open a descending Active on. --}} + @endforeach
{{ $client['clientId'] ?? '' }}{{ $client['clientName'] ?? '' }}{{ implode(' ', $client['authenticationMethods'] ?? []) }}{{ implode(' ', $client['grantTypes'] ?? []) }}{{ implode(' ', $client['scopes'] ?? []) ?: '—' }}{{ implode(' ', $client['redirectUris'] ?? []) ?: '—' }}{{ implode(' · ', $issuance) }}{{ $activeCell }} + {{ $client['clientId'] }} + {{ $client['clientName'] }} + {{ $client['authentication'] ?: '—' }}{{ $client['grants'] ?: '—' }}{{ $client['scopes'] ?: '—' }}{{ $client['redirects'] ?: '—' }}{{ $client['issuance'] }}{{ $client['active'] !== '' ? $client['active'] : '—' }}
- @if ($processLocal) - {{-- Said where the number is read, not in the release notes. Phrased as the store this - process RESOLVED rather than as the value of the driver key or the name of a shipped - class, because what the endpoint reports is the store's own processLocal() — an - application may bind a per-process service of its own, and then neither the key nor the - class says anything. --}} -

Active counts this worker only. The authorization store this - process resolved keeps its authorizations in the process — a map rebuilt in every PHP - worker — so these are the ones held by the worker that rendered this page, and under - php-fpm or Octane the next request lands on a different one. A client with live tokens - elsewhere therefore shows — here. Set - firefly.security.oauth2.server.authorizations.driver to eloquent - (and run the oauth2_authorizations migration), or bind a durable - OAuth2AuthorizationService of your own, for counts that describe the - deployment.

- @endif + @include('firefly-admin::_pager', ['slice' => $slice, 'query' => $query]) + @endif + @if ($processLocal) + {{-- Said where the number is read, not in the release notes. Phrased as the store this + process RESOLVED rather than as the value of the driver key or the name of a shipped + class, because what the endpoint reports is the store's own processLocal() — an + application may bind a per-process service of its own, and then neither the key nor the + class says anything. + + OUTSIDE the @if on the listing, because a narrowed search that matches nothing does not + make the caveat untrue: the page is still counting one worker's authorizations, and a + reader who has just typed a term is exactly the reader about to conclude something from + an Active column they cannot see. --}} +

Active counts this worker only. The authorization store this + process resolved keeps its authorizations in the process — a map rebuilt in every PHP + worker — so these are the ones held by the worker that rendered this page, and under + php-fpm or Octane the next request lands on a different one. A client with live tokens + elsewhere therefore shows — here. Set + firefly.security.oauth2.server.authorizations.driver to eloquent + (and run the oauth2_authorizations migration), or bind a durable + OAuth2AuthorizationService of your own, for counts that describe the + deployment.

@endif
@endsection diff --git a/packages/admin/resources/views/overview.blade.php b/packages/admin/resources/views/overview.blade.php index f87f7b26..2c7c2cf3 100644 --- a/packages/admin/resources/views/overview.blade.php +++ b/packages/admin/resources/views/overview.blade.php @@ -44,7 +44,7 @@ 'body' => 'Implement Firefly\Actuator\Health\HealthIndicator and register it as a bean to see it here.', ]) @else -
+
@foreach ($indicators as $indicator) @@ -77,7 +77,7 @@ 'body' => 'No InfoContributor has contributed anything. Set firefly.management.info.app, or register your own contributor.', ]) @else -
+
@foreach ($info as $key => $value) @@ -97,7 +97,7 @@ @if ($exchanges !== [])
@include('firefly-admin::_panel-head', ['title' => 'Recent requests', 'count' => count($exchanges)]) -
+
@foreach ($exchanges as $exchange) @@ -122,7 +122,7 @@ @if ($metrics !== [])
@include('firefly-admin::_panel-head', ['title' => 'Metrics', 'count' => count($metrics)]) -
+
@foreach (array_slice($metrics, 0, 8) as $metric) diff --git a/packages/admin/resources/views/scheduled.blade.php b/packages/admin/resources/views/scheduled.blade.php index 593a552c..a6303d61 100644 --- a/packages/admin/resources/views/scheduled.blade.php +++ b/packages/admin/resources/views/scheduled.blade.php @@ -10,30 +10,35 @@
- @include('firefly-admin::_panel-head', ['title' => 'Tasks', 'count' => count($tasks)]) - @if ($tasks === []) - @include('firefly-admin::_empty', [ - 'title' => 'Nothing scheduled', - 'body' => 'Add #[Scheduled] to a bean method, then run the scheduler with php artisan schedule:work.', - ]) + @include('firefly-admin::_panel-head', [ + 'title' => 'Tasks', 'count' => $slice->total, 'query' => $query, + 'placeholder' => 'Search by runnable, cron or zone…', + ]) + @if ($slice->isEmpty()) + @include('firefly-admin::_empty', $query->isFiltered() + ? ['title' => 'Nothing matches', 'body' => 'No task\'s runnable, cron expression or zone contains that. Show them all.'] + : ['title' => 'Nothing scheduled', 'body' => 'Add #[Scheduled] to a bean method, then run the scheduler with php artisan schedule:work.']) @else
-
- +
RunnableCronFixed rateFixed delayZone
+ @include('firefly-admin::_table-head', ['view' => $view, 'query' => $query]) - @foreach ($tasks as $task) - @php $runnable = is_string($task['runnable'] ?? null) ? $task['runnable'] : ''; @endphp + @foreach ($slice->rows as $task) - - - - - + + + + + @endforeach
{{ Format::shortClass($runnable) }}{{ rtrim(Format::namespaceOf($runnable), '\\') }}{{ $task['cron'] ?: '—' }}{{ $task['fixedRate'] ?: '—' }}{{ $task['fixedDelay'] ?: '—' }}{{ $task['zone'] ?: '—' }} + {{ Format::leafOf($task['runnable']) }} + {{ Format::stemOf($task['runnable']) }} + {{ $task['cron'] ?: '—' }}{{ $task['fixedRate'] ?: '—' }}{{ $task['fixedDelay'] ?: '—' }}{{ $task['zone'] ?: '—' }}
+ @include('firefly-admin::_pager', ['slice' => $slice, 'query' => $query]) @endif
@endsection diff --git a/packages/admin/src/AdminSettings.php b/packages/admin/src/AdminSettings.php index c3b9f1c5..30a6413e 100644 --- a/packages/admin/src/AdminSettings.php +++ b/packages/admin/src/AdminSettings.php @@ -4,6 +4,7 @@ namespace Firefly\Admin; +use Firefly\Admin\Table\TableSettings; use Firefly\Config\Config; /** @@ -33,6 +34,10 @@ public function __construct( public string $theme = 'auto', public int $graphMaxNodes = 220, public array $excludedPages = [], + public TableSettings $table = new TableSettings, + public bool $routeDetail = true, + public bool $routeAdvice = true, + public BeanGraphSettings $graph = new BeanGraphSettings, ) {} public static function fromConfig(Config $config): self @@ -47,11 +52,16 @@ public static function fromConfig(Config $config): self // never finish and the dashboard would hammer the application it is supposed to be observing. refreshSeconds: max(2, $config->int('firefly.admin.refresh-seconds', 10)), theme: self::theme($config->string('firefly.admin.theme', 'auto')), - // Past this, a dependency diagram is a hairball rather than something anyone can read, so the - // graph page lists the relations instead of drawing them. Configurable because "unreadable" - // depends on the screen and the application. + // Deprecated compatibility value; bounded focus views no longer read this ceiling. graphMaxNodes: max(0, $config->int('firefly.admin.graph.max-nodes', 220)), excludedPages: self::csv($config->string('firefly.admin.pages.exclude', '')), + // Every listing's paging, sorting and spacing. On AdminSettings rather than resolved separately + // because a Blade view reaches exactly one settings object, and a second one would mean every + // view that draws a table taking a second parameter through render(). + table: TableSettings::fromConfig($config), + routeDetail: $config->bool('firefly.admin.routes.detail', true), + routeAdvice: $config->bool('firefly.admin.routes.advice', true), + graph: BeanGraphSettings::fromConfig($config), ); } diff --git a/packages/admin/src/BeanGraph.php b/packages/admin/src/BeanGraph.php index ec45faac..43c53611 100644 --- a/packages/admin/src/BeanGraph.php +++ b/packages/admin/src/BeanGraph.php @@ -26,10 +26,8 @@ * the interface in `via` so the reader sees the indirection rather than being quietly shown something they * did not write. * - * Layering is a longest-path assignment over the resolved edges, so a node sits below everything that - * depends on it and arrows read downward. The walk carries its own visited set, so a cycle terminates and - * the edge that closed it is REPORTED — which matters, because the container has no cycle detection and a - * cycle among eager singletons exhausts memory at boot. + * Iterative Tarjan analysis condenses strongly connected components before assigning longest-path levels. + * Cycle membership is a wiring fact; production edges mean it need not be a runtime constructor cycle. */ final class BeanGraph { @@ -65,6 +63,7 @@ public function __construct( public static function build(array $beans, array $configProperties = []): self { $index = new BeanGraphIndex; + $index->countProducers($beans, $configProperties); foreach ($beans as $row) { if (! is_array($row) || ! is_string($row['class'] ?? null)) { @@ -74,7 +73,7 @@ public static function build(array $beans, array $configProperties = []): self } foreach ($configProperties as $class => $row) { - if (is_array($row) && is_string($row['class'] ?? $class)) { + if (is_array($row) && ($row['bound'] ?? true) !== false && is_string($row['class'] ?? $class)) { $index->addConfigProperties(is_string($row['class'] ?? null) ? $row['class'] : (string) $class); } } @@ -84,8 +83,8 @@ public static function build(array $beans, array $configProperties = []): self $degree = []; foreach ($edges as $edge) { - $degree[$edge['from']]['out'] = ($degree[$edge['from']]['out'] ?? 0) + 1; - $degree[$edge['to']]['in'] = ($degree[$edge['to']]['in'] ?? 0) + 1; + $degree[$edge['from']]['out'][$edge['to']] = true; + $degree[$edge['to']]['in'][$edge['from']] = true; } $nodes = []; @@ -93,8 +92,8 @@ public static function build(array $beans, array $configProperties = []): self $nodes[] = [ ...$node, 'level' => $levels[$id] ?? 0, - 'in' => $degree[$id]['in'] ?? 0, - 'out' => $degree[$id]['out'] ?? 0, + 'in' => count($degree[$id]['in'] ?? []), + 'out' => count($degree[$id]['out'] ?? []), ]; } @@ -106,6 +105,7 @@ public static function build(array $beans, array $configProperties = []): self /** * Kept for the older two-argument shape. * + * * @param array $beans */ public static function fromCatalog(array $beans): self @@ -113,7 +113,8 @@ public static function fromCatalog(array $beans): self return self::build($beans); } - /** @return array node count per kind, for the page's summary */ + /** + * @return array node count per kind, for the page's summary */ public function kindCounts(): array { $counts = [self::KIND_COMPONENT => 0, self::KIND_BEAN => 0, self::KIND_CONFIG => 0]; @@ -128,6 +129,7 @@ public function kindCounts(): array * The namespace roots present, most-populated first — the drawing colours by module, and a legend has to * name them. * + * * @return list */ public function modules(): array @@ -156,9 +158,8 @@ public static function moduleOf(string $id): string } /** - * Longest-path layering, so a node always sits below everything that depends on it. Depth is memoised and - * the walk carries a visited set, so a cycle terminates instead of recursing forever — and the edge that - * closed it is reported. + * Longest-path levels on the component DAG, with every internal cyclic edge retained for compatibility. + * * * @param list $ids * @param list $edges @@ -170,54 +171,59 @@ private static function levels(array $ids, array $edges): array foreach ($edges as $edge) { $out[$edge['from']][] = $edge['to']; } - + $components = GraphComponents::of($ids, $out); $depth = []; $cycles = []; - - $walk = static function (string $node, array $path) use (&$walk, &$depth, &$cycles, $out): int { - if (isset($depth[$node])) { - return $depth[$node]; - } - if (isset($path[$node])) { - return 0; - } - - $path[$node] = true; - $deepest = 0; - foreach ($out[$node] ?? [] as $next) { - if (isset($path[$next])) { - $cycles[] = ['from' => $node, 'to' => $next]; - - continue; + // Tarjan emits dependency components before their consumers. + foreach ($components->groups as $index => $members) { + $depth[$index] = 0; + foreach ($members as $member) { + foreach ($out[$member] ?? [] as $target) { + $other = $components->membership[$target]; + if ($other !== $index) { + $depth[$index] = max($depth[$index], $depth[$other] + 1); + } else { + $cycles[] = ['from' => $member, 'to' => $target]; + } } - $deepest = max($deepest, $walk($next, $path) + 1); } - - return $depth[$node] = $deepest; - }; - - foreach ($ids as $id) { - $walk($id, []); } - - // Depth counts how far a node's longest chain of dependencies runs; the drawing wants the opposite, - // with dependents on top. Flip it so level 0 is what nothing depends on. $max = $depth === [] ? 0 : max($depth); $levels = []; - foreach ($depth as $id => $value) { - $levels[$id] = $max - $value; + foreach ($components->membership as $id => $group) { + $levels[$id] = $max - $depth[$group]; + } + + return [$levels, $cycles]; + } + + /** + * @return array{out: array>, in: array>} */ + public function adjacency(): array + { + $out = $in = []; + foreach ($this->edges as $edge) { + $out[$edge['from']][] = $edge['to']; + $in[$edge['to']][] = $edge['from']; } - $seen = []; - $unique = []; - foreach ($cycles as $cycle) { - $key = $cycle['from'].'>'.$cycle['to']; - if (! isset($seen[$key])) { - $seen[$key] = true; - $unique[] = $cycle; + return ['out' => $out, 'in' => $in]; + } + + /** Complete cyclic components, rather than arbitrary closing pairs. + * @return list> */ + public function components(): array + { + $self = []; + foreach ($this->edges as $edge) { + if ($edge['from'] === $edge['to']) { + $self[$edge['from']] = true; } } - return [$levels, $unique]; + return array_values(array_filter( + GraphComponents::of(array_column($this->nodes, 'id'), $this->adjacency()['out'])->groups, + static fn (array $group): bool => count($group) > 1 || isset($self[$group[0]]), + )); } } diff --git a/packages/admin/src/BeanGraphIndex.php b/packages/admin/src/BeanGraphIndex.php index e43f11ec..c1f801cf 100644 --- a/packages/admin/src/BeanGraphIndex.php +++ b/packages/admin/src/BeanGraphIndex.php @@ -25,12 +25,42 @@ final class BeanGraphIndex /** @var array interface or produced type => the node id that satisfies it */ private array $satisfiedBy = []; + /** @var array */ + private array $ambiguous = []; + /** @var list, type: string}> */ private array $pending = []; /** @var array how many factory methods produce each type */ private array $producerCount = []; + /** + * @param array $rows + * @param array $configProperties + */ + public function countProducers(array $rows, array $configProperties = []): void + { + $classes = []; + foreach ($rows as $row) { + if (is_array($row) && is_string($row['class'] ?? null)) { + $classes[$row['class']] = true; + } + } + foreach ($configProperties as $class => $row) { + if (is_array($row) && ($row['bound'] ?? true) !== false && is_string($row['class'] ?? $class)) { + $classes[is_string($row['class'] ?? null) ? $row['class'] : (string) $class] = true; + } + } + foreach ($rows as $row) { + if (! is_array($row)) { + continue; + } + foreach ($this->producers($row['produces'] ?? null) as $produced) { + $this->producerCount[$produced['type']] = ($this->producerCount[$produced['type']] ?? (isset($classes[$produced['type']]) ? 1 : 0)) + 1; + } + } + } + /** * @param array $row */ @@ -59,10 +89,6 @@ public function addComponent(array $row): void 'type' => BeanGraph::EDGE_INJECTS, ]; - foreach ($this->producers($row['produces'] ?? null) as $produced) { - $this->producerCount[$produced['type']] = ($this->producerCount[$produced['type']] ?? 0) + 1; - } - foreach ($this->producers($row['produces'] ?? null) as $produced) { $this->addBean($class, $produced); } @@ -101,14 +127,17 @@ private function addBean(string $declaring, array $produced): void 'namespace' => rtrim(Format::namespaceOf($produced['type']), '\\'), 'kind' => BeanGraph::KIND_BEAN, 'stereotype' => 'bean', - 'scope' => 'Singleton', + 'scope' => '', 'detail' => Format::shortClass($declaring).'::'.$produced['method'].'()', ]); - // The produced type resolves to this node. With competitors, first-writer-wins gives the bare type a - // stable owner while each competitor keeps its own node — the same shape the container itself has, - // where the type key aliases the #[Primary] winner and every candidate stays reachable by name. - $this->satisfy($produced['type'], $id); + // Without primary/qualifier metadata the catalogue cannot identify a contested winner. + if (! $contested) { + $this->satisfy($produced['type'], $id); + } else { + $this->ambiguous[$produced['type']] = true; + unset($this->satisfiedBy[$produced['type']]); + } $this->pending[] = ['from' => $declaring, 'dependencies' => [$id], 'type' => BeanGraph::EDGE_PRODUCES]; $this->pending[] = ['from' => $id, 'dependencies' => $produced['dependencies'], 'type' => BeanGraph::EDGE_INJECTS]; @@ -151,11 +180,7 @@ public function edges(): array continue; } - if ($target === $entry['from']) { - continue; - } - - $key = $entry['from'].'>'.$target.'>'.$entry['type']; + $key = $entry['from']."\0".$target."\0".$entry['type']."\0".$dependency; if (isset($seen[$key])) { continue; } @@ -175,13 +200,26 @@ public function edges(): array private function resolve(string $type): ?string { + if (isset($this->ambiguous[$type])) { + return null; + } + return isset($this->nodes[$type]) ? $type : ($this->satisfiedBy[$type] ?? null); } - /** First writer wins, so the same application always draws the same graph. */ + /** The projection lacks qualifiers and primary metadata: never invent a winner. */ private function satisfy(string $type, string $nodeId): void { - $this->satisfiedBy[$type] ??= $nodeId; + if (isset($this->ambiguous[$type])) { + return; + } + if (isset($this->satisfiedBy[$type]) && $this->satisfiedBy[$type] !== $nodeId) { + unset($this->satisfiedBy[$type]); + $this->ambiguous[$type] = true; + + return; + } + $this->satisfiedBy[$type] = $nodeId; } /** diff --git a/packages/admin/src/BeanGraphSettings.php b/packages/admin/src/BeanGraphSettings.php new file mode 100644 index 00000000..254ff5f5 --- /dev/null +++ b/packages/admin/src/BeanGraphSettings.php @@ -0,0 +1,35 @@ +int('firefly.admin.graph.focus.depth', 2))), + maxRows: min(60, max(4, $config->int('firefly.admin.graph.focus.max-rows', 16))), + maxNodes: min(300, max(8, $config->int('firefly.admin.graph.focus.max-nodes', 72))), + maxPaths: min(10, max(0, $config->int('firefly.admin.graph.focus.max-paths', 3))), + pageSize: min(500, max(10, $config->int('firefly.admin.graph.focus.page-size', 50))), + starters: min(50, max(1, $config->int('firefly.admin.graph.starters', 12))), + moduleMaxNodes: min(200, max(0, $config->int('firefly.admin.graph.modules.max-nodes', 40))), + beansPageSize: min(500, max(10, $config->int('firefly.admin.beans.page-size', 50))), + ); + } +} diff --git a/packages/admin/src/BeanModules.php b/packages/admin/src/BeanModules.php new file mode 100644 index 00000000..1154914c --- /dev/null +++ b/packages/admin/src/BeanModules.php @@ -0,0 +1,132 @@ + $nodes + * @param list $edges + * @param list> $cycles + */ + private function __construct(public array $nodes, public array $edges, public array $cycles) {} + + public static function fromGraph(BeanGraph $graph, bool $produces = false): self + { + $nodes = $aggregated = $used = $out = []; + foreach ($graph->modules() as $module) { + $leaf = Format::shortClass($module); + $base = substr($leaf, 0, 2); + $used[$base] = ($used[$base] ?? 0) + 1; + $nodes[$module] = ['count' => 0, 'sigil' => $base.($used[$base] > 1 ? $used[$base] : ''), 'hue' => (count($nodes) * 137) % 360]; + } + foreach ($graph->nodes as $node) { + $module = BeanGraph::moduleOf($node['id']); + if (isset($nodes[$module])) { + $nodes[$module]['count']++; + } + } + foreach ($graph->edges as $edge) { + if (! $produces && $edge['type'] === BeanGraph::EDGE_PRODUCES) { + continue; + } + $from = BeanGraph::moduleOf($edge['from']); + $to = BeanGraph::moduleOf($edge['to']); + if ($from === $to) { + continue; + } + $key = $from.'>'.$to; + $aggregated[$key] ??= ['from' => $from, 'to' => $to, 'weight' => 0, 'targets' => [], 'interfaces' => [], 'concrete' => 0]; + $aggregated[$key]['weight']++; + $aggregated[$key]['targets'][$edge['to']] = true; + if ($edge['via'] !== null) { + $aggregated[$key]['interfaces'][$edge['via']] = true; + } else { + $aggregated[$key]['concrete']++; + } + $out[$from][] = $to; + } + $edges = []; + foreach ($aggregated as $edge) { + $edges[] = ['from' => $edge['from'], 'to' => $edge['to'], 'weight' => $edge['weight'], 'beans' => count($edge['targets']), 'via' => count($edge['interfaces']), 'concrete' => $edge['concrete']]; + } + + return new self($nodes, $edges, array_values(array_filter(GraphComponents::of(array_keys($nodes), $out)->groups, static fn (array $group): bool => count($group) > 1))); + } + + /** A module's own beans plus one boundary port per foreign module. */ + public static function scope(BeanGraph $graph, string $module): BeanGraph + { + $nodes = []; + foreach ($graph->nodes as $node) { + if (BeanGraph::moduleOf($node['id']) === $module) { + $nodes[$node['id']] = $node; + } + } + $edges = []; + foreach ($graph->edges as $edge) { + $from = BeanGraph::moduleOf($edge['from']); + $to = BeanGraph::moduleOf($edge['to']); + if ($from !== $module && $to !== $module) { + continue; + } + foreach (['from' => $from, 'to' => $to] as $side => $foreign) { + if ($foreign !== $module) { + $id = 'module:'.$foreign; + $edge[$side] = $id; + $nodes[$id] ??= ['id' => $id, 'label' => $foreign, 'namespace' => $foreign, 'kind' => 'module', 'stereotype' => '', 'scope' => '', 'detail' => 'Boundary port', 'level' => 0, 'in' => 0, 'out' => 0]; + } + } + $edges[$edge['from']."\0".$edge['to']."\0".$edge['type']] = $edge; + } + + foreach ($edges as $edge) { + if ($nodes[$edge['from']]['kind'] === 'module') { + $nodes[$edge['from']]['out']++; + } + if ($nodes[$edge['to']]['kind'] === 'module') { + $nodes[$edge['to']]['in']++; + } + } + + return new BeanGraph(array_values($nodes), array_values($edges), [], []); + } + + /** + * @return list */ + public static function exclusive(BeanGraph $graph, string $module): array + { + $adjacency = $graph->adjacency()['out']; + $own = $others = []; + foreach ($graph->nodes as $node) { + if (BeanGraph::moduleOf($node['id']) === $module) { + $own[] = $node['id']; + } else { + $others[] = $node['id']; + } + } + + return array_values(array_diff(self::reachable($own, $adjacency), self::reachable($others, $adjacency))); + } + + /** + * @param list $queue + * @param array> $out + * @return list */ + private static function reachable(array $queue, array $out): array + { + $seen = array_fill_keys($queue, true); + for ($i = 0; isset($queue[$i]); $i++) { + foreach ($out[$queue[$i]] ?? [] as $next) { + if (! isset($seen[$next])) { + $seen[$next] = true; + $queue[] = $next; + } + } + } + + return array_keys($seen); + } +} diff --git a/packages/admin/src/BeanNeighbourhood.php b/packages/admin/src/BeanNeighbourhood.php new file mode 100644 index 00000000..f23644ce --- /dev/null +++ b/packages/admin/src/BeanNeighbourhood.php @@ -0,0 +1,142 @@ + $positions + * @param list}> $overflow + * @param list> $paths + * @param list $columns + */ + private function __construct( + public array $positions, + public array $overflow, + public array $paths, + public array $columns, + public int $width, + public int $height, + ) {} + + public static function around(BeanGraph $graph, string $focus, int $depth, string $direction, BeanGraphSettings $settings): ?self + { + $nodes = array_column($graph->nodes, null, 'id'); + if (! isset($nodes[$focus])) { + return null; + } + $depth = min(4, max(1, $depth)); + $adjacency = $graph->adjacency(); + $columns = range($direction === 'out' ? 0 : -$depth, $direction === 'in' ? 0 : $depth); + $placed = [$focus => 0]; + $byColumn = [0 => [$focus]]; + $frontiers = ['in' => [$focus], 'out' => [$focus]]; + $overflow = []; + for ($hop = 1; $hop <= $depth; $hop++) { + foreach (['in', 'out'] as $side) { + if ($direction !== 'both' && $direction !== $side) { + continue; + } + $column = $side === 'in' ? -$hop : $hop; + $queues = []; + foreach ($frontiers[$side] as $source) { + $neighbors = array_values(array_unique($adjacency[$side][$source] ?? [])); + usort($neighbors, static fn (string $a, string $b): int => [$nodes[$b]['in'] + $nodes[$b]['out'], $nodes[$a]['label'], $a] <=> [$nodes[$a]['in'] + $nodes[$a]['out'], $nodes[$b]['label'], $b]); + $queues[$source] = $neighbors; + } + $selected = []; + $offset = 0; + do { + $hasNext = false; + foreach ($queues as $queue) { + if (! isset($queue[$offset])) { + continue; + } + $hasNext = true; + $id = $queue[$offset]; + if (! isset($placed[$id]) && count($selected) < $settings->maxRows && count($placed) < $settings->maxNodes) { + $placed[$id] = $column; + $selected[] = $id; + } + } + $offset++; + } while ($hasNext); + // Order by the mean position of adjacent already-placed parents, then exact identity. + $ranks = array_flip($frontiers[$side]); + $score = []; + foreach ($selected as $id) { + $parents = $adjacency[$side === 'in' ? 'out' : 'in'][$id] ?? []; + $values = []; + foreach ($parents as $parent) { + if (isset($ranks[$parent])) { + $values[] = $ranks[$parent]; + } + } + sort($values); + $score[$id] = $values === [] ? 0 : $values[intdiv(count($values), 2)]; + } + usort($selected, static fn (string $a, string $b): int => [$score[$a], $nodes[$a]['label'], $a] <=> [$score[$b], $nodes[$b]['label'], $b]); + $byColumn[$column] = $frontiers[$side] = $selected; + foreach ($queues as $source => $queue) { + $hidden = array_values(array_filter($queue, static fn (string $id): bool => ! isset($placed[$id]))); + if ($hidden !== []) { + $overflow[] = ['source' => $source, 'direction' => $side, 'count' => count($hidden), 'ids' => $hidden]; + } + } + } + } + $finalOverflow = []; + foreach ($overflow as $entry) { + $ids = array_values(array_filter($entry['ids'], static fn (string $id): bool => ! isset($placed[$id]))); + if ($ids !== []) { + $finalOverflow[] = [...$entry, 'ids' => $ids, 'count' => count($ids)]; + } + } + $rows = max(1, ...array_map('count', $byColumn)); + $height = $rows * 46 - 8; + $positions = []; + foreach ($columns as $index => $column) { + $members = $byColumn[$column] ?? []; + $top = intdiv(($rows - count($members)) * 46, 2); + foreach ($members as $row => $id) { + $positions[$id] = ['x' => $index * 204, 'y' => $top + $row * 46, 'column' => $column, 'row' => $row]; + } + } + + return new self($positions, $finalOverflow, self::rootPaths($focus, $adjacency['in'], $settings->maxPaths), $columns, count($columns) * 204 - 28, $height); + } + + /** + * @param array> $in + * @return list> */ + private static function rootPaths(string $focus, array $in, int $limit): array + { + foreach ($in as &$parents) { + sort($parents, SORT_STRING); + } + unset($parents); + $queue = [$focus]; + $toward = [$focus => null]; + $paths = []; + for ($i = 0; isset($queue[$i]) && count($paths) < $limit; $i++) { + $id = $queue[$i]; + if (($in[$id] ?? []) === []) { + $path = []; + for ($next = $id; $next !== null; $next = $toward[$next]) { + $path[] = $next; + } + $paths[] = $path; + } + foreach ($in[$id] ?? [] as $parent) { + if (! array_key_exists($parent, $toward)) { + $toward[$parent] = $id; + $queue[] = $parent; + } + } + } + + return $paths; + } +} diff --git a/packages/admin/src/Data/DataBrowserSettings.php b/packages/admin/src/Data/DataBrowserSettings.php index 7734f88e..16be978a 100644 --- a/packages/admin/src/Data/DataBrowserSettings.php +++ b/packages/admin/src/Data/DataBrowserSettings.php @@ -91,7 +91,17 @@ public function allows(string $slug): bool return ! in_array($slug, $this->excluded, true); } - /** Clamp a caller-supplied page size into [1, maxPageSize]; null means "use the configured default". */ + /** + * Clamp a caller-supplied page size into [1, maxPageSize]; null means "use the configured default". + * + * THE DASHBOARD DOES NOT ARRIVE HERE WITH A SURPRISE. Its listing query is parsed against + * `Table\TableSettings::boundedBy($this->pageSize, $this->maxPageSize)`, so the size it hands `list()` + * is already inside both bounds and this returns it unchanged — which is exactly what lets the + * rows-per-page control offer the sizes the listing can actually serve, and what lets `page-size` decide + * the default there rather than being shadowed by a size the page states on every call. Both bounds + * still bite for a direct `DataBrowser::list()` call, which has no query to have been parsed against + * them and may ask for anything. + */ public function clampPageSize(?int $requested): int { if ($requested === null) { diff --git a/packages/admin/src/Data/DataFilter.php b/packages/admin/src/Data/DataFilter.php index ca68a90e..60fbe0ca 100644 --- a/packages/admin/src/Data/DataFilter.php +++ b/packages/admin/src/Data/DataFilter.php @@ -78,29 +78,46 @@ public function label(): string } /** - * The query-string form of a list of filters, so every link that must preserve them is built from one - * place. A single equality keeps the short `fk`/`fv` spelling that relation links use. + * This filter set as query PARAMETERS, which is the form every link builder wants. + * + * A LINK CARRIES A FILTER AS DATA, NOT AS A STRING IT WAS HANDED. `ListingQuery` is given this array as + * the parameters it must not lose, and it writes them out with `http_build_query` alongside its own — + * which is what lets a sort link, a page link and a search form each rebuild the whole URL from parsed + * values rather than concatenating someone else's fragment onto their own. * * @param list $filters + * @return array> */ - public static function toQuery(array $filters): string + public static function toParameters(array $filters): array { if ($filters === []) { - return ''; + return []; } + // TWO SPELLINGS, ONE MEANING. `fk`/`fv` is a single equality and is what every relation link + // produces — short enough to read in a status bar. Both are validated identically downstream. if (count($filters) === 1 && $filters[0]->operator === self::EQ) { - return 'fk='.urlencode($filters[0]->column).'&fv='.urlencode($filters[0]->value); + return ['fk' => $filters[0]->column, 'fv' => $filters[0]->value]; } - $parts = []; - foreach ($filters as $filter) { - $parts[] = 'fc[]='.urlencode($filter->column) - .'&fo[]='.urlencode($filter->operator) - .'&fv[]='.urlencode($filter->value); - } + return [ + 'fc' => array_map(static fn (self $filter): string => $filter->column, $filters), + 'fo' => array_map(static fn (self $filter): string => $filter->operator, $filters), + 'fv' => array_map(static fn (self $filter): string => $filter->value, $filters), + ]; + } - return implode('&', $parts); + /** + * The query-string form, for the one caller that still wants a string. + * + * Built by http_build_query rather than by concatenating urlencode() calls: the hand-built version was + * correct and was one edit away from not being, which is exactly the class of bug this wave is removing. + * + * @param list $filters + */ + public static function toQuery(array $filters): string + { + return http_build_query(self::toParameters($filters)); } /** A one-line description of what this filter narrows to, for the banner above a filtered listing. */ diff --git a/packages/admin/src/Data/DataQueryEngine.php b/packages/admin/src/Data/DataQueryEngine.php index e52280aa..2284d743 100644 --- a/packages/admin/src/Data/DataQueryEngine.php +++ b/packages/admin/src/Data/DataQueryEngine.php @@ -7,6 +7,7 @@ use BackedEnum; use DateTimeInterface; use Firefly\Actuator\Introspection\SensitiveValueMasker; +use Firefly\Admin\RowComparator; use Firefly\Data\Repository\CrudRepository; use Firefly\Data\Repository\EloquentRepository; use Firefly\Data\Repository\Page; @@ -181,7 +182,7 @@ private function fetch( ?string $term, array $filters = [], ): array { - $pageable = new Pageable($page, $perPage, $this->sort($sort, $direction)); + $pageable = new Pageable($page, $perPage, $this->sort($sort, $direction, $schema)); if (($term !== null || $filters !== []) && $repository instanceof EloquentRepository) { $specifications = []; @@ -327,10 +328,60 @@ private function fetchInPhp( } if ($sort !== null) { - usort($matched, function (array $a, array $b) use ($sort, $direction): int { - $comparison = $this->compare($a['values'][$sort] ?? null, $b['values'][$sort] ?? null); + // The same ordering the actuator listings use, from the same class — a sort drifts as quietly as + // a filter does, and the drift would show as one column header meaning two different orders on + // two pages of one dashboard. It is asked for the COLUMN, once, rather than pair by pair: a + // column that mixes numbers with anything else has no consistent pairwise answer (see + // RowComparator::forColumn()), and an inconsistent comparison here reorders the rows that + // straddle a page boundary between one request and the next. Emptiness is ranked OUTSIDE the + // direction flip: an em-dash is the absence of a value rather than a value that sorts low, so it + // stays last under `desc` too. + $values = array_map(static fn (array $row): mixed => $row['values'][$sort] ?? null, $matched); + $compare = $sort === $schema->identifier + ? RowComparator::forIdentity($values) + : RowComparator::forColumn($values); + + // The same tiebreak the SQL paths get, for the same reason: `usort` is stable in PHP 8, but the + // ARRAY it is stabilising is rebuilt from the repository on every request, so stability of the + // sort is not stability of the page boundary. See sort() above. Like the empty rank above it, + // and for the same reason, it sits OUTSIDE the direction flip: an identity is not a second + // ordering. It is appended only when the identifier is not already the sort column, for the + // same reason `ORDER BY id DESC, id ASC` is not emitted. + // + // THE TIEBREAK IS A COLUMN TOO, AND IS CHOSEN THE SAME WAY — `forColumn()` over everything the + // identifier column holds, never `compare()` pair by pair. `Table\InMemoryListing::order()` + // does exactly this beside it, and RowComparator says why: a per-pair choice between the + // arithmetic and the natural comparison can close a cycle (`1.10 < 1.9 < 1.9-beta < 1.10` on a + // column of versions), and an ordering that is not a strict weak ordering entitles `usort()` to + // answer anything at all. An inconsistent tiebreak breaks the whole ordering just as thoroughly + // as an inconsistent primary, which is the instability this branch exists to remove. + $tiebreak = $schema->identifier; + $breakTie = $tiebreak === null || $tiebreak === $sort ? null : RowComparator::forIdentity(array_map( + static fn (array $row): mixed => $row['values'][$tiebreak] ?? null, + $matched, + )); + + usort($matched, static function (array $a, array $b) use ($sort, $direction, $compare, $tiebreak, $breakTie): int { + $left = $a['values'][$sort] ?? null; + $right = $b['values'][$sort] ?? null; + + $rank = RowComparator::rankEmpty($left, $right); + + if ($rank !== 0) { + return $rank; + } + + $comparison = $compare($left, $right); - return $direction === 'desc' ? -$comparison : $comparison; + if ($direction === 'desc') { + $comparison = -$comparison; + } + + // `$breakTie` is null for exactly the two cases that have no tiebreak to apply — no + // identifier, or an identifier that IS the sort column — so testing it tests both. + return $comparison !== 0 || $breakTie === null + ? $comparison + : $breakTie($a['values'][$tiebreak] ?? null, $b['values'][$tiebreak] ?? null); }); } @@ -375,8 +426,10 @@ private function passes(array $values, array $filters): bool DataFilter::NE => $string !== $filter->value, DataFilter::CONTAINS => $string !== null && str_contains(mb_strtolower($string), mb_strtolower($filter->value)), DataFilter::STARTS => $string !== null && str_starts_with(mb_strtolower($string), mb_strtolower($filter->value)), - DataFilter::GT => $string !== null && $this->compare($value, $filter->value) > 0, - DataFilter::LT => $string !== null && $this->compare($value, $filter->value) < 0, + // The SCALAR STRING, not the raw value: see compare() for why a bool must reach it as the + // `'1'`/`''` the driver would have bound and not as the word the listing renders. + DataFilter::GT => $string !== null && $this->compare($string, $filter->value) > 0, + DataFilter::LT => $string !== null && $this->compare($string, $filter->value) < 0, DataFilter::NULL => $value === null, DataFilter::NOT_NULL => $value !== null, default => $string === $filter->value, @@ -430,15 +483,38 @@ private function sortColumn(DataSchema $schema, ?string $requested): ?string : null; } - private function sort(?string $column, string $direction): ?Sort + /** + * The ORDER BY a listing goes out with: what was asked for, then the identifier as a tiebreak. + * + * EVERY LISTING IS ORDERED, EVEN WHEN NOBODY ASKED — that much was already true, and it is why + * sortColumn() falls back to the identifier. What was missing is the second half. Ordering by a column + * with duplicate values leaves the tied rows in whatever order the engine finds convenient, and it is + * allowed to find a different one convenient for the query behind page 1 and the query behind page 2: + * a row is then returned on both and another on neither, and the operator reads a table that is missing + * records which are really there. `ORDER BY status DESC, id ASC` has no such freedom. + * + * THE TIEBREAK IS ALWAYS ASCENDING. It is an identity, not a second ordering — mirroring it with the + * primary direction would make the order WITHIN a tie depend on the direction it is breaking the tie + * inside, which is the same instability with extra steps. And it is appended only when the identifier + * is not already the sort column, because `ORDER BY id DESC, id ASC` is at best noise and at worst an + * index the planner declines to use. + */ + private function sort(?string $column, string $direction, DataSchema $schema): ?Sort { if ($column === null) { return null; } $sort = Sort::by($column); + if ($direction === 'desc') { + $sort = $sort->descending(); + } + + $identifier = $schema->identifier; - return $direction === 'desc' ? $sort->descending() : $sort; + return $identifier !== null && $identifier !== $column && in_array($identifier, $schema->sortable(), true) + ? $sort->and(Sort::by($identifier)) + : $sort; } private function term(?string $search): ?string @@ -543,24 +619,26 @@ private function truncate(string $value, ?int $limit): string return mb_substr($value, 0, $limit).self::ELLIPSIS; } - /** Null-last ordering, so a nullable column does not sort its empties into the middle of the values. */ - private function compare(mixed $a, mixed $b): int + /** + * How `greater than` and `less than` compare on the unpaged path. + * + * IT IS GIVEN THE CELL'S SCALAR STRING, NEVER THE RAW VALUE, and the difference is not cosmetic. The + * paged sibling of this predicate is `where(col, '>', ?)` in SQL: the driver binds a bool as `1`/`0`, so + * `pinned > 0` selects the pinned rows. RowComparator renders a bool for a READER — `true`/`false` — and + * a word is not numeric, so the pair would fall to the natural-text comparison where `t` and `f` sort + * after every digit: `greater than 0` would match EVERY row and `less than 1` none, on a repository that + * cannot page, while the identical filter over a pageable resource answered correctly. `(string) true` + * is `'1'` and `(string) false` is `''`, which are the shapes the binding has, so passing the string + * keeps one filter meaning one thing on both paths. That is the same drift the search predicate above + * warns about, arriving through the operand rather than through a second implementation. + * + * It is RowComparator's value comparison and nothing else — no empty-last rank. In SQL the empty string + * is simply the smallest string, and ranking empties last here would be the same drift again. Where + * empties go is a question about a LISTING's order, which is asked in the sort, not here. + */ + private function compare(string $a, string $b): int { - if ($a === null && $b === null) { - return 0; - } - if ($a === null) { - return 1; - } - if ($b === null) { - return -1; - } - - if (is_scalar($a) && is_scalar($b)) { - return is_numeric($a) && is_numeric($b) ? ($a + 0) <=> ($b + 0) : strnatcasecmp((string) $a, (string) $b); - } - - return 0; + return RowComparator::compare($a, $b); } /** diff --git a/packages/admin/src/ExplorerQuery.php b/packages/admin/src/ExplorerQuery.php new file mode 100644 index 00000000..304b0cc1 --- /dev/null +++ b/packages/admin/src/ExplorerQuery.php @@ -0,0 +1,60 @@ + $parameters */ + public function __construct(public string $path, public array $parameters, public int $depth, public string $direction) {} + + public static function fromRequest(Request $request, string $path, BeanGraphSettings $settings): self + { + $parameters = []; + foreach ($request->query() as $key => $value) { + if (is_string($key) && is_string($value) && strlen($value) <= 1024) { + $parameters[$key] = $value; + } + } + $raw = $parameters['depth'] ?? ''; + $depth = ctype_digit($raw) ? min(4, max(1, (int) $raw)) : $settings->depth; + $direction = in_array($parameters['dir'] ?? '', ['in', 'out', 'both'], true) ? $parameters['dir'] : 'both'; + $parameters['depth'] = (string) $depth; + $parameters['dir'] = $direction; + + return new self($path, $parameters, $depth, $direction); + } + + public function get(string $key): string + { + return $this->parameters[$key] ?? ''; + } + + /** @param array $changes */ + public function url(array $changes = []): string + { + $parameters = array_filter([...$this->parameters, ...$changes], static fn (string|int|null $value): bool => $value !== null && $value !== ''); + + return $this->path.($parameters === [] ? '' : '?'.http_build_query($parameters)); + } + + /** Parameters owned by other controls only. + * @return array */ + public function carried(string $qualifier): array + { + $parameters = $this->parameters; + foreach (['page', 'size', 'sort', 'dir', 'q'] as $key) { + unset($parameters[$qualifier.'_'.$key]); + } + + return $parameters; + } + + public function bean(string $id): string + { + return $this->url(['bean' => $id, 'module' => null, 'q' => null, 'neighbor' => null, 'rel' => null, 'in_page' => null, 'out_page' => null]); + } +} diff --git a/packages/admin/src/Format.php b/packages/admin/src/Format.php index a06b01ce..11d48653 100644 --- a/packages/admin/src/Format.php +++ b/packages/admin/src/Format.php @@ -85,7 +85,16 @@ public static function measurement(string $meterName, float $value): string return self::count($value); } - /** "3 minutes ago" for a unix timestamp, or an ISO instant when it is older than a day. */ + /** + * "3 minutes ago" for a unix timestamp, or a dated instant when it is older than a day. + * + * THE WIDEST THING THIS EMITS IS SIXTEEN CHARACTERS, not the six of `2h ago`, and a column sized for + * the age alphabet clips the date one. Past the 86400-second arm an age stops being informative — "37h + * ago" is a number the reader has to do arithmetic on — so the last arm gives the instant itself, and + * `2026-09-22 20:49` is what a `Stamp` column has to be wide enough to draw. See + * AdminAction::data()'s HTTP listing, whose When column is sized from this method's alphabet, and + * FormatStampTest, which pins both widths so a future arm cannot widen one without failing. + */ public static function since(float $timestamp, float $now): string { $delta = max(0.0, $now - $timestamp); @@ -99,6 +108,21 @@ public static function since(float $timestamp, float $now): string }; } + /** + * The full instant behind a `since()` age — `2026-09-22 20:49:26`, the nineteen characters + * `TableColumn::stamp()` takes as its default width. + * + * This is what a stamp cell carries on its `title`, and it exists because an age is LOSSY in both + * directions: `2h ago` does not say which two hours, and the dated arm rounds the seconds off. A reader + * correlating an exchange against a log line needs the instant, and a cell whose text is clipped by its + * column needs somewhere to recover the value from — the same contract `t-token`, `t-path` and `t-line` + * already keep with the full value on the title. + */ + public static function instant(float $timestamp): string + { + return date('Y-m-d H:i:s', (int) $timestamp); + } + /** The share one value takes of a maximum, clamped to 0..100 for a bar width. */ public static function percent(float $value, float $max): float { @@ -143,18 +167,40 @@ public static function elide(string $value, int $max): string return strlen($value) <= $max ? $value : '…'.substr($value, -($max - 1)); } + /** + * The half of a qualified name that IDENTIFIES it: `OrderController`, `store`. + * + * Parameterised on the separator because the dashboard qualifies four different things by three + * different characters — a class by `\`, a config key and a meter name by `.` — and the rendering is + * identical in all of them: the leaf on top, the stem dim underneath, and the stem is the half that may + * be elided when the column is narrow. + */ + public static function leafOf(string $value, string $separator = '\\'): string + { + $position = strrpos($value, $separator); + + return $position === false ? $value : substr($value, $position + strlen($separator)); + } + + /** The half that LOCATES it: `App\Http\Controllers`, `firefly.observability.metrics`. */ + public static function stemOf(string $value, string $separator = '\\'): string + { + $position = strrpos($value, $separator); + + return $position === false ? '' : substr($value, 0, $position); + } + /** A short, readable class name with its namespace kept as a separate, dimmable prefix. */ public static function shortClass(string $fqcn): string { - $position = strrpos($fqcn, '\\'); - - return $position === false ? $fqcn : substr($fqcn, $position + 1); + return self::leafOf($fqcn); } + /** The namespace WITH its trailing separator — the shape the bean graph and the entity map print. */ public static function namespaceOf(string $fqcn): string { - $position = strrpos($fqcn, '\\'); + $stem = self::stemOf($fqcn); - return $position === false ? '' : substr($fqcn, 0, $position + 1); + return $stem === '' ? '' : $stem.'\\'; } } diff --git a/packages/admin/src/GraphComponents.php b/packages/admin/src/GraphComponents.php new file mode 100644 index 00000000..2fad9197 --- /dev/null +++ b/packages/admin/src/GraphComponents.php @@ -0,0 +1,74 @@ +> $groups + * @param array $membership + */ + private function __construct(public array $groups, public array $membership) {} + + /** + * @param list $ids + * @param array> $out + */ + public static function of(array $ids, array $out): self + { + $index = $low = $active = $membership = []; + $stack = $groups = []; + $next = 0; + foreach ($ids as $root) { + if (isset($index[$root])) { + continue; + } + $frames = [[$root, 0, null]]; + while ($frames !== []) { + $top = count($frames) - 1; + [$node, $offset, $parent] = $frames[$top]; + if (! isset($index[$node])) { + $index[$node] = $low[$node] = $next++; + $stack[] = $node; + $active[$node] = true; + } + $neighbors = $out[$node] ?? []; + if ($offset < count($neighbors)) { + $target = $neighbors[$offset]; + $frames[$top][1]++; + if (! isset($index[$target])) { + $frames[] = [$target, 0, $node]; + } elseif (isset($active[$target])) { + $low[$node] = min($low[$node], $index[$target]); + } + + continue; + } + array_pop($frames); + if ($parent !== null) { + $low[$parent] = min($low[$parent], $low[$node]); + } + if ($low[$node] === $index[$node]) { + $group = []; + do { + $member = array_pop($stack); + if ($member === null) { + break; + } + unset($active[$member]); + $membership[$member] = count($groups); + $group[] = $member; + } while ($member !== $node); + sort($group); + $groups[] = $group; + } + } + } + + return new self($groups, $membership); + } +} diff --git a/packages/admin/src/Route/RouteBinding.php b/packages/admin/src/Route/RouteBinding.php new file mode 100644 index 00000000..504b2f12 --- /dev/null +++ b/packages/admin/src/Route/RouteBinding.php @@ -0,0 +1,34 @@ +notFoundMessage = isset($plan['pattern']) + ? ($plan['notFoundMessage'] ?? ArgumentResolver::notFoundSentence($plan['name'])) : null; + } + + public function supplied(): bool + { + // Resolver claims take precedence over every compiled kind, including query and service. + return $this->resolver !== null || $this->plan['kind'] === 'service'; + } + + public function defaultLabel(): string + { + $encoded = json_encode($this->plan['default'], JSON_UNESCAPED_SLASHES | JSON_INVALID_UTF8_SUBSTITUTE); + + return $encoded !== false ? $encoded : 'null'; + } +} diff --git a/packages/admin/src/Route/RouteBodyNode.php b/packages/admin/src/Route/RouteBodyNode.php new file mode 100644 index 00000000..b0e78be1 --- /dev/null +++ b/packages/admin/src/Route/RouteBodyNode.php @@ -0,0 +1,55 @@ + $children */ + public function __construct(public string $name, public ?string $type, public bool $list, public array $children = [], public ?string $note = null) {} + + /** @return list */ + public static function forBinding(RouteBinding $binding): array + { + $plan = $binding->plan; + $type = $plan['type']; + $shapes = $plan['dtos'] ?? []; + if ($type === null || ! isset($shapes[$type])) { + return array_map(static fn (string $name): self => new self($name, null, false), $plan['properties']); + } + $budget = 200; + + return self::walk($type, $shapes, [], $budget); + } + + /** + * @param array> $shapes + * @param list $path + * @return list + */ + private static function walk(string $type, array $shapes, array $path, int &$budget): array + { + $path[] = $type; + $nodes = []; + foreach ($shapes[$type] ?? [] as $name => $property) { + if (--$budget < 0) { + $nodes[] = new self('More properties', null, false, note: 'Display limit reached'); + break; + } + $class = $property['class']; + $note = match (true) { + $class !== null && in_array($class, $path, true) => 'Recursive reference', + count($path) >= 16 => 'Depth limit reached', + default => null, + }; + $children = $class !== null && $note === null ? self::walk($class, $shapes, $path, $budget) : []; + $nodes[] = new self($name, $class, $property['list'], $children, $note); + } + + return $nodes; + } +} diff --git a/packages/admin/src/Route/RouteDetail.php b/packages/admin/src/Route/RouteDetail.php new file mode 100644 index 00000000..353ba23d --- /dev/null +++ b/packages/admin/src/Route/RouteDetail.php @@ -0,0 +1,35 @@ + $registrations + * @param list $caller + * @param list $injected + * @param list $failures + * @param list $siblings + */ + public function __construct( + public RouteDescriptor $route, + public array $registrations, + public array $caller, + public array $injected, + public array $failures, + public array $siblings, + ) {} + + /** @return list */ + public function arguments(): array + { + $arguments = [...$this->caller, ...$this->injected]; + usort($arguments, static fn (RouteBinding $a, RouteBinding $b): int => $a->position <=> $b->position); + + return $arguments; + } +} diff --git a/packages/admin/src/Route/RouteInspector.php b/packages/admin/src/Route/RouteInspector.php new file mode 100644 index 00000000..a7303512 --- /dev/null +++ b/packages/admin/src/Route/RouteInspector.php @@ -0,0 +1,141 @@ +httpMethod.' '.$route->path; + } + + public function detail(string $key): ?RouteDetail + { + if (! $this->container->bound(RouteManifest::class)) { + return null; + } + $routes = $this->container->make(RouteManifest::class)->all(); + $matches = array_values(array_filter($routes, static fn (RouteDescriptor $route): bool => self::keyOf($route) === $key)); + if ($matches === []) { + return null; + } + // Laravel registers in manifest order: the final registration for a verb/path replaces earlier ones. + $route = $matches[array_key_last($matches)]; + $resolvers = $this->container->bound(HandlerMethodArgumentResolvers::class) + ? $this->container->make(HandlerMethodArgumentResolvers::class) : null; + $caller = $injected = $failures = []; + foreach ($route->bindings as $position => $plan) { + // supports() only. resolve() can read a security context, instantiate services, or throw. + $resolver = $resolvers?->resolverFor($plan); + $binding = new RouteBinding($position + 1, $plan, $resolver === null ? null : $resolver::class); + if ($binding->supplied()) { + $injected[] = $binding; + } else { + $caller[] = $binding; + array_push($failures, ...$this->failures($binding)); + } + } + + return new RouteDetail($route, $matches, $caller, $injected, $failures, array_values(array_filter( + $routes, static fn (RouteDescriptor $sibling): bool => $sibling->controllerClass === $route->controllerClass && self::keyOf($sibling) !== $key, + ))); + } + + /** @return array{uri: string, name: string|null, domain: string|null, middleware: list, patterns: array}|null */ + public function metadata(RouteDescriptor $descriptor): ?array + { + $router = $this->container->bound('router') ? $this->container->make('router') : null; + if (! $router instanceof Router) { + return null; + } + $uri = $descriptor->path === '/' ? '/' : trim($descriptor->path, '/'); + foreach ($router->getRoutes()->getRoutes() as $route) { + // Manifest routes have no domain. A domain-specific route with the same URI is a different route. + if ($route->uri() === $uri && $route->getDomain() === null && in_array($descriptor->httpMethod, $route->methods(), true)) { + $patterns = []; + foreach ($route->wheres as $name => $pattern) { + if (is_string($name) && is_string($pattern)) { + $patterns[$name] = $pattern; + } + } + + return ['uri' => $route->uri(), 'name' => $route->getName(), 'domain' => $route->getDomain(), + 'middleware' => array_values(array_filter($route->middleware(), is_string(...))), 'patterns' => $patterns]; + } + } + + return null; + } + + /** @return list */ + public function advice(RouteDescriptor $route): array + { + if (! $this->container->bound(ProxyPlan::class)) { + return []; + } + $plan = $this->container->make(ProxyPlan::class); + $kinds = $plan->adviceFor($route->controllerClass); + $rows = []; + foreach ($plan->methodsFor($route->controllerClass)[$route->methodName] ?? [] as $link) { + $kind = $kinds[$link['advice']] ?? null; + if ($kind === null) { + continue; + } + // Read raw rows across the existing Admin→Data edge; descriptor strings are data, never imports. + // An unbound interceptor is inert ONLY if that advice explicitly permits it; otherwise boot fails. + $inert = $kind->inertWhenUnbound; + $rows[] = ['id' => $link['advice'], 'order' => $kind->order, 'interceptor' => $kind->interceptorClass, + 'state' => $this->container->bound($kind->interceptorClass) ? 'LIVE' : ($inert ? 'INERT' : 'UNBOUND'), + 'contract' => json_encode($link['row'], JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES | JSON_INVALID_UTF8_SUBSTITUTE) ?: '{}']; + } + + return $rows; + } + + /** @return list */ + private function failures(RouteBinding $binding): array + { + $plan = $binding->plan; + $kind = $plan['kind']; + $failures = []; + $add = static function (int $status, string $code, string $reason) use (&$failures, $plan): void { + $failures[] = ['status' => $status, 'code' => $code, 'binding' => $plan['name'], 'reason' => $reason]; + }; + if ($plan['required'] && in_array($kind, ['path', 'query', 'header', 'file'], true)) { + $add(400, 'MISSING_PARAMETER', 'Required '.$kind.' value is absent.'); + } + if (($kind === 'file') || (in_array($kind, ['path', 'query', 'header'], true) && in_array($plan['type'], ['int', 'float', 'bool'], true))) { + $add(400, 'TYPE_CONVERSION_ERROR', $kind === 'file' ? 'Multiple uploads were sent to a single-file argument.' : 'The value cannot be converted to '.$plan['type'].'.'); + } + if ($kind === 'path' && isset($plan['pattern'])) { + $add(404, $plan['notFoundCode'] ?? 'RESOURCE_NOT_FOUND', $binding->notFoundMessage ?? 'Path pattern did not match.'); + } + if ($kind === 'file') { + $add(400, 'INVALID_UPLOAD', 'The upload did not complete.'); + } + if ($kind === 'body') { + $add(400, 'MALFORMED_BODY', 'The body reader rejects malformed JSON.'); + $add(400, 'INVALID_REQUEST', 'Unsupported Content-Type or a decoded value that is not an array.'); + if ($plan['type'] !== null && class_exists($plan['type'])) { + $add(400, 'UNBINDABLE_BODY', 'The decoded body cannot construct the DTO.'); + } + if ($plan['valid'] && $plan['type'] !== null) { + $add(422, 'VALIDATION_ERROR', 'Bean validation rejects the body before hydration.'); + } + } + + return $failures; + } +} diff --git a/packages/admin/src/RowComparator.php b/packages/admin/src/RowComparator.php new file mode 100644 index 00000000..25f543be --- /dev/null +++ b/packages/admin/src/RowComparator.php @@ -0,0 +1,152 @@ +', ?)` in SQL, + * where the empty string is simply the smallest string; ranking empties last there would make the same filter + * mean two things depending on whether the repository could page. + */ +final class RowComparator +{ + /** + * Where two values sit RELATIVE TO EACH OTHER on emptiness alone: 0 when both are empty or neither is, + * and otherwise the empty one last. Apply this before the direction, never through it — see the class + * docblock for why a descending sort must not mirror it. + */ + public static function rankEmpty(mixed $a, mixed $b): int + { + return self::emptiness($a) <=> self::emptiness($b); + } + + /** + * The ordering for ONE COLUMN: the choice between the two comparisons made ONCE, from everything the + * column holds, and then applied to every pair of it. + * + * THE CHOICE CANNOT BE MADE PER PAIR, and that is the whole reason this method exists rather than every + * caller reaching for `compare()`. Take a `version` column holding `1.10`, `1.9` and `1.9-beta` — a + * mixture no schema forbids and no validation catches. Pair by pair: `1.10 < 1.9`, because both are + * numeric and 1.1 is less than 1.9; `1.9 < 1.9-beta`, because one of them is not numeric and a prefix + * sorts before what extends it; and `1.9-beta < 1.10`, naturally, because 9 is less than 10. The relation + * closes a cycle, so it is not a strict weak ordering and `usort()` is entitled to answer anything: those + * three rows really do come back in three different orders depending on which order the payload arrived + * in, and the payload is rebuilt per request. That is exactly the "a row is then seen twice, and another + * never" that `Table\InMemoryListing`'s tiebreak exists to prevent — and the tiebreak cannot rescue it, + * because a tiebreak runs only where the primary comparison returned 0 and this one returns a confident, + * inconsistent non-zero. + * + * So the column commits before the first comparison: `compare()`'s arithmetic ONLY when every value in it + * is a number, and otherwise the natural text comparison for every pair. Empties are skipped by the scan + * rather than counted against it — they are ranked out by `rankEmpty()` before any of this is reached, + * and where a caller compares them anyway (a tiebreak does) they are the smallest text under either + * rule, so a nullable column of numbers keeps its arithmetic. `9` still precedes `100` in a column of + * durations, `Bean2` still precedes `Bean10` in a column of class names, and a column that mixes the two + * now answers the same way whatever order it was handed in. + * + * @param iterable $values every value the column holds across the rows about to be ordered + * @return Closure(mixed, mixed): int + */ + public static function forColumn(iterable $values): Closure + { + foreach ($values as $value) { + if ($value === null || $value === '' || is_numeric($value)) { + continue; + } + + return static fn (mixed $a, mixed $b): int => strnatcasecmp(self::text($a), self::text($b)); + } + + return self::compare(...); + } + + /** + * Identity cannot equate distinct spellings merely because their friendly ordering ties. + * + * @param iterable $values + * @return Closure(mixed, mixed): int + */ + public static function forIdentity(iterable $values): Closure + { + $compare = self::forColumn($values); + + return static fn (mixed $a, mixed $b): int => $compare($a, $b) ?: strcmp(self::text($a), self::text($b)); + } + + /** + * Order two values as a reader would expect them ordered, emptiness aside. + * + * Numbers compare as numbers, so `9` precedes `100` and a column of durations is not sorted by its first + * digit. The addition rather than a float cast keeps a nineteen-digit identifier exact: past 2^53 two + * adjacent snowflake ids cast to the same float and would compare equal. Everything else compares + * naturally and case-insensitively, so `Bean2` precedes `Bean10` and a list of class names does not split + * into an upper-case half and a lower-case one. + * + * THIS IS THE PAIRWISE RULE, and on its own it belongs only where there is no column to be consistent + * with: the unpaged `gt`/`lt` filter, which holds one cell against one operand the reader typed and asks + * a yes-or-no question about the two of them. A SORT goes through `forColumn()`, which picks between the + * two comparisons above once for the whole column — see there for the cycle a per-pair choice opens. + */ + public static function compare(mixed $a, mixed $b): int + { + if (is_numeric($a) && is_numeric($b)) { + return ($a + 0) <=> ($b + 0); + } + + return strnatcasecmp(self::text($a), self::text($b)); + } + + /** + * The text of a cell, for comparing it and for searching inside it. + * + * An array is rendered as its JSON rather than dropped, because a listing of, say, a bean's dependencies + * is an array cell an operator does searches inside; an object falls back to its type, which is the only + * thing that can be said about it without calling code this class does not own. + */ + public static function text(mixed $value): string + { + return match (true) { + $value === null => '', + is_bool($value) => $value ? 'true' : 'false', + is_scalar($value) => (string) $value, + is_array($value) => (string) json_encode($value, JSON_UNESCAPED_SLASHES), + default => get_debug_type($value), + }; + } + + private static function emptiness(mixed $value): int + { + return $value === null || $value === '' ? 1 : 0; + } +} diff --git a/packages/admin/src/Table/ColumnKind.php b/packages/admin/src/Table/ColumnKind.php new file mode 100644 index 00000000..54b7e3d6 --- /dev/null +++ b/packages/admin/src/Table/ColumnKind.php @@ -0,0 +1,82 @@ +value; + } + + /** Whether this kind is sized from its own alphabet rather than from what is left over. */ + public function isRigid(): bool + { + return match ($this) { + self::Pill, self::Number, self::Stamp, self::Meter, self::Actions => true, + self::Token, self::Path, self::Qualified, self::Text, self::Line => false, + }; + } + + /** A picture of another column, and a set of controls, have nothing to order by. */ + public function isOrderable(): bool + { + return $this !== self::Meter && $this !== self::Actions; + } +} diff --git a/packages/admin/src/Table/InMemoryListing.php b/packages/admin/src/Table/InMemoryListing.php new file mode 100644 index 00000000..a7825668 --- /dev/null +++ b/packages/admin/src/Table/InMemoryListing.php @@ -0,0 +1,147 @@ + + * + * @param list $rows the whole listing, keyed by column key + * @param list $searchable the keys `?q=` looks inside — never every key, because searching a + * column the page does not show answers a question about a value the + * reader cannot see + * @param string $tiebreak a key whose value is unique per row + * @return ListingPage + */ + public static function page(array $rows, ListingQuery $query, array $searchable, string $tiebreak): ListingPage + { + $ordered = self::order( + self::search($rows, $query->search, $searchable), + $query->sort ?? $tiebreak, + $query->direction, + $tiebreak, + ); + + $total = count($ordered); + $page = ListingPage::pageFor($total, $query); + + return ListingPage::sliced( + array_slice($ordered, ($page - 1) * $query->size, $query->size), + $total, + $query, + ); + } + + /** + * @template TRow of array + * + * @param list $rows + * @param list $searchable + * @return list + */ + private static function search(array $rows, ?string $term, array $searchable): array + { + if ($term === null || $searchable === []) { + return $rows; + } + + $needle = mb_strtolower($term); + + return array_values(array_filter($rows, static function (array $row) use ($needle, $searchable): bool { + foreach ($searchable as $key) { + if (str_contains(mb_strtolower(RowComparator::text($row[$key] ?? null)), $needle)) { + return true; + } + } + + return false; + })); + } + + /** + * @template TRow of array + * + * @param list $rows + * @return list + */ + private static function order(array $rows, string $column, string $direction, string $tiebreak): array + { + // ONE COMPARISON PER COLUMN, chosen before the first pair is looked at. A column that mixes numbers + // with anything else has no consistent answer pair by pair — see RowComparator::forColumn() for the + // cycle — and usort() then returns whatever the arrival order suggested, which is the unstable page + // this class's tiebreak exists to prevent. The tiebreak is a column too, and is chosen the same way: + // an inconsistent tiebreak breaks the whole ordering just as thoroughly as an inconsistent primary. + $values = self::valuesOf($rows, $column); + $compare = $column === $tiebreak + ? RowComparator::forIdentity($values) + : RowComparator::forColumn($values); + $breakTie = RowComparator::forIdentity(self::valuesOf($rows, $tiebreak)); + + usort($rows, static function (array $a, array $b) use ($column, $direction, $tiebreak, $compare, $breakTie): int { + $left = $a[$column] ?? null; + $right = $b[$column] ?? null; + + // Emptiness OUTSIDE the direction: an em-dash is the absence of a value, not a value that sorts + // low, so it stays at the end of the listing whichever way the column was asked to run. + $comparison = RowComparator::rankEmpty($left, $right); + + if ($comparison === 0) { + $comparison = $compare($left, $right); + + if ($direction === 'desc') { + $comparison = -$comparison; + } + } + + return $comparison !== 0 + ? $comparison + : $breakTie($a[$tiebreak] ?? null, $b[$tiebreak] ?? null); + }); + + return $rows; + } + + /** + * One column of the listing, missing cells included as the `null` the ordering will see — `array_column()` + * DROPS a row that lacks the key, and a column is judged numeric or not by what the comparison will + * actually be handed. + * + * @param list> $rows + * @return list + */ + private static function valuesOf(array $rows, string $key): array + { + return array_map(static fn (array $row): mixed => $row[$key] ?? null, $rows); + } +} diff --git a/packages/admin/src/Table/ListingPage.php b/packages/admin/src/Table/ListingPage.php new file mode 100644 index 00000000..5066df86 --- /dev/null +++ b/packages/admin/src/Table/ListingPage.php @@ -0,0 +1,115 @@ +` is. The rows are `list` as far as this object is + * concerned — it never looks inside one — but a caller that hands it a precisely-shaped `list` + * gets that shape back out of `$rows`, which is what keeps a view (and PHPStan at level max) from having to + * re-assert the shape of every cell it draws. + * + * @template TRow + */ +final readonly class ListingPage +{ + /** + * @param list $rows + */ + private function __construct( + public array $rows, + public int $total, + public int $page, + public ListingQuery $query, + ) {} + + /** + * @template TSliced + * + * @param list $rows the page's rows — already sliced, by whatever did the slicing + * @return self + */ + public static function sliced(array $rows, int $total, ListingQuery $query): self + { + $total = max(0, $total); + + return new self($rows, $total, self::pageFor($total, $query), $query); + } + + /** The requested page clamped into `[1, last]` — the page a caller must actually slice for. */ + public static function pageFor(int $total, ListingQuery $query): int + { + return min(max(1, $query->page), self::lastPageFor($total, $query->size)); + } + + /** Always at least 1: an empty listing has one (empty) page, not zero. */ + public static function lastPageFor(int $total, int $size): int + { + return $size > 0 ? max(1, (int) ceil($total / $size)) : 1; + } + + public function lastPage(): int + { + return self::lastPageFor($this->total, $this->query->size); + } + + public function from(): int + { + return $this->total === 0 ? 0 : ($this->page - 1) * $this->query->size + 1; + } + + public function to(): int + { + return min($this->total, $this->page * $this->query->size); + } + + public function hasPrevious(): bool + { + return $this->page > 1; + } + + public function hasNext(): bool + { + return $this->page < $this->lastPage(); + } + + public function isEmpty(): bool + { + return $this->rows === []; + } + + /** Whether the pager's page controls are worth rendering at all. */ + public function isPaged(): bool + { + return $this->lastPage() > 1; + } + + /** + * A window around the current page. Rendering every page of a four-hundred-page listing is a control + * nobody can use; the ends are kept by the pager itself, because "first" and "last" are the two jumps + * people actually make. + * + * @return list + */ + public function window(int $radius = 2): array + { + return range(max(1, $this->page - $radius), min($this->lastPage(), $this->page + $radius)); + } + + public function link(int $page): string + { + return $this->query->link(['page' => $page]); + } +} diff --git a/packages/admin/src/Table/ListingQuery.php b/packages/admin/src/Table/ListingQuery.php new file mode 100644 index 00000000..65d4eb17 --- /dev/null +++ b/packages/admin/src/Table/ListingQuery.php @@ -0,0 +1,296 @@ + $sortable the column keys this listing will accept in `?sort=` + * @param array> $carried parameters this listing does not own and must not lose + */ + public function __construct( + public TableSettings $settings, + public string $path, + public int $page, + public int $size, + public ?string $sort, + public string $direction, + public ?string $search, + public array $sortable = [], + public array $carried = [], + public string $qualifier = '', + public ?string $defaultSort = null, + public string $defaultDirection = 'asc', + ) {} + + /** + * @param list $sortable + * @param array> $carried + */ + public static function fromRequest( + Request $request, + TableSettings $settings, + string $path, + array $sortable = [], + ?string $defaultSort = null, + string $defaultDirection = 'asc', + array $carried = [], + string $qualifier = '', + ): self { + $read = static function (string $parameter) use ($request, $qualifier): ?string { + $value = $request->query($qualifier === '' ? $parameter : $qualifier.'_'.$parameter); + + return is_string($value) ? $value : null; + }; + + $page = $read('page'); + $size = $read('size'); + $requested = $read('sort'); + $search = trim($read('q') ?? ''); + + $sort = $requested !== null && in_array($requested, $sortable, true) ? $requested : $defaultSort; + + return new self( + settings: $settings, + path: $path, + page: $page !== null && ctype_digit($page) ? max(1, (int) $page) : 1, + size: $settings->clamp($size !== null && ctype_digit($size) ? (int) $size : null), + sort: $sort, + direction: $sort === null ? $defaultDirection : match ($read('dir')) { + 'desc' => 'desc', + 'asc' => 'asc', + default => $defaultDirection, + }, + search: $search === '' ? null : mb_substr($search, 0, self::MAX_SEARCH_LENGTH), + sortable: $sortable, + carried: $carried, + qualifier: $qualifier, + defaultSort: $defaultSort, + defaultDirection: $defaultDirection, + ); + } + + /** + * This listing's URL with some of its state replaced. An override is keyed by the UNQUALIFIED parameter + * name and `null` removes it, so `link(['page' => null])` is "the same view, from the top". + * + * @param array $overrides + */ + public function link(array $overrides = []): string + { + /** @var array $state */ + $state = [ + 'q' => $this->search, + 'sort' => $this->sort, + 'dir' => $this->direction, + 'size' => $this->size, + 'page' => $this->page, + ...$overrides, + ]; + + $parameters = $this->carried; + foreach ($this->meaningful($state) as $parameter => $value) { + $parameters[$this->name($parameter)] = $value; + } + + $query = http_build_query($parameters); + + return $query === '' ? $this->path : $this->path.'?'.$query; + } + + /** The link the column header points at: this column's ordering, from the first page. */ + public function sortLink(string $column): string + { + return $this->link(['sort' => $column, 'dir' => $this->nextDirection($column), 'page' => null]); + } + + public function nextDirection(string $column): string + { + return $this->isSortedBy($column) && $this->direction === 'asc' ? 'desc' : 'asc'; + } + + public function isSortedBy(string $column): bool + { + return $this->sort !== null && $this->sort === $column; + } + + /** The arrow a sorted header carries — empty for every other column, so the page has exactly one. */ + public function indicator(string $column): string + { + return $this->isSortedBy($column) ? ($this->direction === 'asc' ? '↑' : '↓') : ''; + } + + public function isFiltered(): bool + { + return $this->search !== null; + } + + /** + * The state a GET search form must re-submit, as hidden inputs. + * + * `q` is absent because the form's own input supplies it, and `page` is absent because a new search + * starts at the top — page 4 of a result set that no longer exists is an empty table with a pager. + * + * @return list + */ + public function hiddenFields(): array + { + $fields = []; + + foreach ($this->carried as $parameter => $value) { + foreach (is_array($value) ? $value : [$value] as $one) { + $fields[] = ['name' => is_array($value) ? $parameter.'[]' : $parameter, 'value' => $one]; + } + } + + foreach ($this->meaningful(['sort' => $this->sort, 'dir' => $this->direction, 'size' => $this->size]) as $parameter => $value) { + $fields[] = ['name' => $this->name($parameter), 'value' => $value]; + } + + return $fields; + } + + /** + * This listing's own parameters, qualified — what the OTHER listing on the same page has to carry. + * + * @return array + */ + public function own(): array + { + $own = []; + foreach ($this->meaningful(['q' => $this->search, 'sort' => $this->sort, 'dir' => $this->direction, 'size' => $this->size, 'page' => $this->page]) as $parameter => $value) { + $own[$this->name($parameter)] = $value; + } + + return $own; + } + + /** @param array> $parameters */ + public function carrying(array $parameters): self + { + return new self( + settings: $this->settings, + path: $this->path, + page: $this->page, + size: $this->size, + sort: $this->sort, + direction: $this->direction, + search: $this->search, + sortable: $this->sortable, + carried: [...$this->carried, ...$parameters], + qualifier: $this->qualifier, + defaultSort: $this->defaultSort, + defaultDirection: $this->defaultDirection, + ); + } + + /** + * The same listing at the size it was actually served. + * + * THIS EXISTS BECAUSE ONE SIDE ASKS AND THE OTHER SERVES. Every actuator listing is sliced by + * `InMemoryListing` at exactly `$size`, so for those this is never called. The data browser is the + * caller: `DataBrowser::list()` applies `firefly.admin.data.max-page-size` to whatever it is handed, and + * a page that merely ASSUMED the two agree would be assuming something it cannot see from the outside. + * `TableSettings::boundedBy()` composes that cap into the settings this query was parsed against + * precisely so they do agree, and this is the line that makes the agreement a fact rather than a hope. + * Were they ever to part, every number a pager draws — the range readout, the last page, whether `Next` + * is live — would still be computed from the size that was refused: over a served 50 a query stating 100 + * claims half as many pages as there are and disables `Next` with the second half of the table + * unreached. So the query is re-stated at the size that was served, and every link it then writes + * carries that size rather than the one it asked for. A re-stated size is the one size on this object + * that the offered set did not choose, which is why the rows-per-page control renders + * `TableSettings::offering()` rather than the set itself. + */ + public function sized(int $size): self + { + return $size === $this->size ? $this : new self( + settings: $this->settings, + path: $this->path, + page: $this->page, + size: $size, + sort: $this->sort, + direction: $this->direction, + search: $this->search, + sortable: $this->sortable, + carried: $this->carried, + qualifier: $this->qualifier, + defaultSort: $this->defaultSort, + defaultDirection: $this->defaultDirection, + ); + } + + /** + * The parameters worth writing down: everything that is set and is not already the default. + * + * `dir` is the subtle one. With no sort at all the direction says nothing, so its "default" is taken to + * be whatever it currently is and it drops out; with a sort, it is written only when it differs from + * the listing's declared default — which is how `/firefly/http` stays the URL of the newest-first page + * it opens on. + * + * @param array $state + * @return array + */ + private function meaningful(array $state): array + { + $sort = $state['sort'] ?? null; + + $meaningful = []; + foreach ($state as $parameter => $value) { + $default = match ($parameter) { + 'sort' => $this->defaultSort, + 'dir' => $sort === null || $sort === '' ? $value : $this->defaultDirection, + 'size' => $this->settings->pageSize, + 'page' => 1, + default => null, + }; + + if ($value !== null && $value !== '' && $value !== $default) { + $meaningful[$parameter] = (string) $value; + } + } + + return $meaningful; + } + + private function name(string $parameter): string + { + return $this->qualifier === '' ? $parameter : $this->qualifier.'_'.$parameter; + } +} diff --git a/packages/admin/src/Table/TableColumn.php b/packages/admin/src/Table/TableColumn.php new file mode 100644 index 00000000..21ffe1d2 --- /dev/null +++ b/packages/admin/src/Table/TableColumn.php @@ -0,0 +1,171 @@ +`'s `ch` is the 12.5px monospace advance the sheet declares on `table.ftable colgroup` + * (7.52px in Chromium), while `thead th` is 10px/700 uppercase in the UI face with `.12em` tracking. + * Measured across the labels a schema actually produces — `Id` 0.88, `Unit price` 0.95, + * `Failed login attempts` 0.99, `Created at` 1.02, `Warehouse manager` 1.12, `Customer` 1.14, + * `Amount` 1.20 — one header character costs between 0.88 and 1.20 of those `ch`, the top of the range + * being the short, round-lettered words rather than the long labels. Linux's system face puts + * `Amount` at 1.26, so 1.30 leaves room across both font stacks instead of clipping it at 1.25. + * + * IT IS AN ESTIMATE, AND THE HEADER THAT USES IT SAYS SO. PHP cannot measure a font, so a pathological + * label — twelve `W`s measures 1.52 — still overflows its column; the data browser therefore puts the + * full label on the ``'s `title`, exactly as `_cell` already does for a value it clips. + */ + public const float HEADER_CH_PER_CHARACTER = 1.30; + + /** + * What the ordering indicator adds to a SORTABLE header, in the same `ch`. + * + * `th .ord` is a 10px-wide inline-block inside an `inline-flex` anchor with `gap:3px`, and that width + * is declared rather than taken from the arrow — so the 13px is paid whether the column is the sorted + * one or not, which is what stops a header changing width under the click that sorts it. 13px over a + * 7.52px `ch` is 1.73, rounded up. + */ + public const float SORT_INDICATOR_CH = 1.75; + + private function __construct( + public string $key, + public string $label, + public ColumnKind $kind, + public float $width, + public bool $sortable, + public string $separator, + ) {} + + /** A chip. 7.5 characters is `DELETE` with room — the width the Routes page was clipping. */ + public static function pill(string $key, string $label, float $ch = 7.5, bool $sortable = true): self + { + return new self($key, $label, ColumnKind::Pill, $ch, $sortable, ''); + } + + public static function number(string $key, string $label, float $ch = 9, bool $sortable = true): self + { + return new self($key, $label, ColumnKind::Number, $ch, $sortable, ''); + } + + /** 19 characters is `2026-09-22 20:49:26`; an age reads shorter and is padded by the same rule. */ + public static function stamp(string $key, string $label, float $ch = 19, bool $sortable = true): self + { + return new self($key, $label, ColumnKind::Stamp, $ch, $sortable, ''); + } + + public static function meter(string $label, float $ch = 16): self + { + return new self('', $label, ColumnKind::Meter, $ch, false, ''); + } + + public static function actions(string $label = '', float $ch = 11): self + { + return new self('', $label, ColumnKind::Actions, $ch, false, ''); + } + + public static function token(string $key, string $label, float $weight = 2, bool $sortable = true): self + { + return new self($key, $label, ColumnKind::Token, $weight, $sortable, ''); + } + + public static function path(string $key, string $label, float $weight = 5, bool $sortable = true): self + { + return new self($key, $label, ColumnKind::Path, $weight, $sortable, ''); + } + + /** + * @param string $separator what the name is qualified by — `\` for a class, `.` for a config key or a + * meter name, and `''` when the two lines come from two different fields + * rather than from splitting one (the OAuth2 page's client id over client + * name), in which case the view supplies both halves itself + */ + public static function qualified(string $key, string $label, float $weight = 4, string $separator = '\\', bool $sortable = true): self + { + return new self($key, $label, ColumnKind::Qualified, $weight, $sortable, $separator); + } + + public static function text(string $key, string $label, float $weight = 4, bool $sortable = false): self + { + return new self($key, $label, ColumnKind::Text, $weight, $sortable, ''); + } + + public static function line(string $key, string $label, float $weight = 4, bool $sortable = true): self + { + return new self($key, $label, ColumnKind::Line, $weight, $sortable, ''); + } + + /** + * The same column, widened until its own HEADER fits beside its values. + * + * A rigid width is sized from the alphabet the CELLS can hold — nineteen characters for an ISO instant, + * seven and a half for the verb `DELETE` — and on a listing with hand-written labels that is the whole + * story, because the author picked the label against the width. It stops being the whole story the + * moment the label is derived. `thead th` is `white-space:nowrap` and `table.ftable td,th` is + * `overflow:hidden`, so a header wider than its column is cut mid-glyph; and under the + * `table-layout:fixed` this system runs on, the column cannot grow to rescue it the way the + * `table-layout:auto` the data browser used to run on did. So the width becomes the greater of the two + * needs, never the lesser — a column still holds its values, and now it also says what they are. + * + * A FLEXIBLE COLUMN IS RETURNED UNTOUCHED, because its `$width` is a weight and not a character count. + * Widening it would not widen a column; it would enlarge that column's share of the leftovers at its + * neighbours' expense, for a reason that has nothing to do with them — and a percentage column on a + * table this wide clears any header it is likely to be given anyway. + */ + public function fittingItsHeader(): self + { + if (! $this->isRigid()) { + return $this; + } + + $header = mb_strlen($this->label) * self::HEADER_CH_PER_CHARACTER + + ($this->isSortable() ? self::SORT_INDICATOR_CH : 0.0); + + return $header <= $this->width + ? $this + : new self($this->key, $this->label, $this->kind, $header, $this->sortable, $this->separator); + } + + public function cssClass(): string + { + return $this->kind->cssClass(); + } + + public function isRigid(): bool + { + return $this->kind->isRigid(); + } + + /** Sortable needs a key to sort BY and a kind that can be ordered — both, not either. */ + public function isSortable(): bool + { + return $this->sortable && $this->key !== '' && $this->kind->isOrderable(); + } +} diff --git a/packages/admin/src/Table/TableSettings.php b/packages/admin/src/Table/TableSettings.php new file mode 100644 index 00000000..5b18942a --- /dev/null +++ b/packages/admin/src/Table/TableSettings.php @@ -0,0 +1,219 @@ +` renders, so the + * configured default is ALWAYS a member of it: a `` can still say where it is. + * + * `max-height` IS INTERPOLATED INTO THE STYLESHEET, as the `--table-vh` custom property the scroll container + * reads, which makes it the one key on this object that is not merely wrong when it is wrong. A value is + * matched against a length pattern and refused outright rather than escaped, because there is no escaping + * that makes `70vh;}body{display:none` safe inside a ` +
diff --git a/packages/openapi/tests/BindingFixture/BindingController.php b/packages/openapi/tests/BindingFixture/BindingController.php new file mode 100644 index 00000000..3724c242 --- /dev/null +++ b/packages/openapi/tests/BindingFixture/BindingController.php @@ -0,0 +1,52 @@ +value(); + } + + /** @param mixed $payload */ + #[PostMapping('/binding/untyped')] + public function untyped(#[Valid] #[RequestBody] $payload): void {} + + #[GetMapping('/binding/pattern/{id}')] + public function pattern(#[PathVariable(pattern: PathVariable::UUID, notFoundCode: 'ITEM_NOT_FOUND')] string $id): string + { + return $id; + } + + #[GetMapping('/binding/plain/{id}')] + public function plain(#[PathVariable] string $id): string + { + return $id; + } + + #[ApiResponse(404, 'This identifier does not name an item.')] + #[GetMapping('/binding/declared/{id}')] + public function declared(#[PathVariable(pattern: PathVariable::UUID)] string $id): string + { + return $id; + } +} diff --git a/packages/openapi/tests/BindingFixture/Collaborator.php b/packages/openapi/tests/BindingFixture/Collaborator.php new file mode 100644 index 00000000..37dddead --- /dev/null +++ b/packages/openapi/tests/BindingFixture/Collaborator.php @@ -0,0 +1,13 @@ + 'data from a page controller']; + } + + #[GetMapping('/binding/page-markup')] + public function markup(): HtmlString + { + return new HtmlString('

A page

'); + } + + #[GetMapping('/binding/page-redirect')] + public function redirect(): RedirectResponse + { + return new RedirectResponse('/binding/page-markup'); + } +} diff --git a/packages/openapi/tests/Generator/OpenApiGeneratorTest.php b/packages/openapi/tests/Generator/OpenApiGeneratorTest.php index f619f12e..5f3b8947 100644 --- a/packages/openapi/tests/Generator/OpenApiGeneratorTest.php +++ b/packages/openapi/tests/Generator/OpenApiGeneratorTest.php @@ -152,8 +152,8 @@ expect($statuses($create))->toBe(['201', '400', '422', 'default']) // A bool query parameter must be coerced out of the wire's string, so 400 is reachable. ->and($statuses($show))->toBe(['200', '400', 'default']) - // A single `string` path variable cannot fail binding at all — no phantom 400. - ->and($statuses($cancel))->toBe(['204', 'default']) + // The string needs no coercion, but its UUID pattern can reject the request with 404. + ->and($statuses($cancel))->toBe(['204', '404', 'default']) ->and($cancel[204])->toBe(['description' => 'No content.']); }); diff --git a/packages/openapi/tests/Schema/ProblemSchemaTest.php b/packages/openapi/tests/Schema/ProblemSchemaTest.php index 5b03b4e6..7b8f5673 100644 --- a/packages/openapi/tests/Schema/ProblemSchemaTest.php +++ b/packages/openapi/tests/Schema/ProblemSchemaTest.php @@ -6,7 +6,11 @@ use Firefly\Kernel\Error\ErrorResponse; use Firefly\Kernel\Error\ErrorSeverity; use Firefly\Kernel\Error\FieldError; +use Firefly\Kernel\Exception\Business\ResourceNotFoundException; use Firefly\OpenApi\Schema\ProblemSchema; +use Firefly\Web\Error\ProblemMapper; +use Firefly\Web\Exception\ProblemDetailsRenderer; +use Illuminate\Http\Request; /** * The error component is only worth anything if it describes what the framework REALLY sends, so the tests @@ -52,6 +56,41 @@ expect($undocumented)->toBe([], 'ErrorResponse emits members the problem schema does not describe'); }); +/** + * The two RFC 9457 URI members, held against the document the framework really publishes. + * + * For one release this component said `type` was "OPTIONAL here where the RFC gives it a default" and that + * `instance` "carries a request PATH rather than a URI reference", and both halves outlived by one commit + * the conformance pass that made them false: ProblemDetailsRenderer emits `type` on EVERY document + * (`about:blank` unless `firefly.web.problem.type-uri` names a base) and `instance` is the root-relative + * reference ProblemMapper::instanceFor() builds. A description here is not decoration — it is printed into + * every generated openapi.json and into the docs of every generated client — and it is the one surface in + * this wave that no prose guard can read, because it is PHP. So it is asserted against the renderer's real + * output rather than against a sentence. + */ +it('describes `type` and `instance` as the URI references the renderer really publishes', function () { + /** @var array> $properties */ + $properties = ProblemSchema::schema()['properties']; + + /** @var array $published */ + $published = json_decode((string) (new ProblemDetailsRenderer)->render( + new ResourceNotFoundException('Order 42 not found'), + Request::create('/api/orders/42'), + )->getContent(), true, 512, JSON_THROW_ON_ERROR); + + expect($published)->toHaveKey('type') + // Emitted by default, which is why the description may not go on calling the member one a caller + // gets only when something explicitly passes it. + ->and($published['type'])->toBe('about:blank') + ->and($properties['type']['description'])->toContain('about:blank') + ->and($properties['type']['format'])->toBe('uri-reference') + // A URI reference on both sides of the contract, and root-relative — not the bare path. + ->and($published['instance'])->toBe(ProblemMapper::instanceFor(Request::create('/api/orders/42'))) + ->and($properties['instance']['format'])->toBe('uri-reference') + ->and($properties['instance']['description'])->toContain('root-relative') + ->and($properties['instance']['description'])->toContain('instanceFor'); +}); + /** * The two ids are two members, and the spec has to say which is which. Every problem+json response carries * `correlationId` (ProblemDetailsRenderer passes it on every render), and `traceId` stopped being "the diff --git a/packages/openapi/tests/Support/BindingContractTestCase.php b/packages/openapi/tests/Support/BindingContractTestCase.php new file mode 100644 index 00000000..61948d04 --- /dev/null +++ b/packages/openapi/tests/Support/BindingContractTestCase.php @@ -0,0 +1,13 @@ + FixtureDocument::psr4('BindingFixture')]; + } +} diff --git a/packages/openapi/tests/Web/BindingContractTest.php b/packages/openapi/tests/Web/BindingContractTest.php new file mode 100644 index 00000000..8b576933 --- /dev/null +++ b/packages/openapi/tests/Web/BindingContractTest.php @@ -0,0 +1,75 @@ +extend(BindingContractTestCase::class); + +it('does not promise validation for a Valid query, service or untyped body that dispatch never validates', function (string $path, string $verb, array $input) { + /** @var BindingContractTestCase $this */ + if ($verb === 'get') { + $this->getJson($path.'?'.http_build_query($input))->assertOk(); + } else { + $this->postJson($path, $input)->assertOk(); + } + $operation = FixtureDocument::operation(FixtureDocument::generatorFor('BindingFixture')->generate(), $path, $verb); + + expect($operation['responses'])->not->toHaveKey('422'); +})->with([ + 'query' => ['/binding/query', 'get', ['value' => 'accepted']], + 'service' => ['/binding/service', 'get', []], + 'untyped body' => ['/binding/untyped', 'post', ['value' => 'accepted']], +]); + +it('publishes the problem response for the path-pattern failure the dispatcher produces', function () { + /** @var BindingContractTestCase $this */ + $this->getJson('/binding/pattern/not-a-uuid')->assertNotFound()->assertJsonPath('code', 'ITEM_NOT_FOUND'); + $operation = FixtureDocument::operation(FixtureDocument::generatorFor('BindingFixture')->generate(), '/binding/pattern/{id}', 'get'); + + expect($operation['responses'])->toHaveKey('404') + ->and(data_get($operation, 'responses.404'))->toBe(['$ref' => ProblemSchema::RESPONSE_REF]); +}); + +it('does not invent a pattern failure for an unconstrained path parameter', function () { + /** @var BindingContractTestCase $this */ + $this->getJson('/binding/plain/anything')->assertOk(); + $operation = FixtureDocument::operation(FixtureDocument::generatorFor('BindingFixture')->generate(), '/binding/plain/{id}', 'get'); + + expect($operation['responses'])->not->toHaveKey('404'); +}); + +it('preserves declared response prose over a derived pattern failure', function () { + $operation = FixtureDocument::operation(FixtureDocument::generatorFor('BindingFixture')->generate(), '/binding/declared/{id}', 'get'); + + expect(data_get($operation, 'responses.404'))->toMatchArray([ + 'description' => 'This identifier does not name an item.', + 'content' => [ProblemSchema::MEDIA_TYPE => ['schema' => ['$ref' => ProblemSchema::REF]]], + ]); +}); + +it('derives an included page controllers media type from its return contract', function () { + /** @var BindingContractTestCase $this */ + $this->getJson('/binding/page-data')->assertOk()->assertJsonPath('message', 'data from a page controller'); + $document = FixtureDocument::generatorFor('BindingFixture', FixtureDocument::properties(includeHtml: true))->generate(); + $operation = FixtureDocument::operation($document, '/binding/page-data', 'get'); + + expect(data_get($operation, 'responses.200.content'))->toHaveKey('application/json') + ->not->toHaveKey('text/html') + ->and(data_get($operation, 'responses.200.content.application/json.schema.properties'))->toHaveKey('message'); +}); + +it('keeps actual markup and redirect responses for included page controllers', function () { + /** @var BindingContractTestCase $this */ + $this->get('/binding/page-markup')->assertOk()->assertSee('

A page

', false); + $this->get('/binding/page-redirect')->assertRedirect('/binding/page-markup'); + $document = FixtureDocument::generatorFor('BindingFixture', FixtureDocument::properties(includeHtml: true))->generate(); + $markup = FixtureDocument::operation($document, '/binding/page-markup', 'get'); + $redirect = FixtureDocument::operation($document, '/binding/page-redirect', 'get'); + + expect(data_get($markup, 'responses.200.content'))->toHaveKey('text/html') + ->and($redirect['responses'])->toHaveKey('302') + ->and(data_get($redirect, 'responses.302.headers'))->toHaveKey('Location'); +}); diff --git a/packages/security-oauth2-server/tests/Admin/AdminOAuth2PageFlowTest.php b/packages/security-oauth2-server/tests/Admin/AdminOAuth2PageFlowTest.php index d4485819..da4dc07a 100644 --- a/packages/security-oauth2-server/tests/Admin/AdminOAuth2PageFlowTest.php +++ b/packages/security-oauth2-server/tests/Admin/AdminOAuth2PageFlowTest.php @@ -60,25 +60,25 @@ protected function defineFireflyEnvironment(Application $app): void ->assertSee('orders:read') ->assertSee('client.create') // The one client-credentials token issued above is the one live authorization the svc row reports. - ->assertSee('1', false) + ->assertSee('1', false) // …counted in the `memory` driver's map, which belongs to the process that rendered this page. The // page says so, and prints `—` rather than `0` for the two clients this worker holds nothing for: // under php-fpm that zero would be every client's, whatever the pool is actually holding. ->assertSee('Active counts this worker only') - ->assertSee('—', false) + ->assertSee('—', false) // PKCE is reported as the endpoints ENFORCE it, not as the client registered it: public-spa carries // no `require_pkce` of its own, yet the stock server-wide default refuses its every authorization // request without a code_challenge. Its issuance cell is the one without `consent`, so this string // belongs to that row alone. - ->assertSee('self_contained · 300s · PKCE', false) + ->assertSee('self_contained · 300s · PKCE', false) // web-app holds the authorization_code grant and the stock consent default, so it carries both. - ->assertSee('self_contained · 300s · PKCE · consent', false) + ->assertSee('self_contained · 300s · PKCE · consent', false) // …and svc carries NEITHER. It is registered for client_credentials only: it never sends a // code_challenge and never reaches a consent screen, so the two rules do not describe it. Both // defaults are on, so a cell built from them would read `· PKCE · consent` beside a machine client // and hand the operator two requirements to check that nothing in the server enforces for it — on // the page opened to answer why a client cannot get a token. - ->assertSee('self_contained · 300s', false) + ->assertSee('self_contained · 300s', false) ->assertDontSee(OAuth2ServerCapstoneTestCase::WEB_APP_SECRET) ->assertDontSee(OAuth2ServerCapstoneTestCase::SVC_SECRET); diff --git a/packages/web/composer.json b/packages/web/composer.json index f962bf59..2686382e 100644 --- a/packages/web/composer.json +++ b/packages/web/composer.json @@ -28,11 +28,13 @@ "firefly/context": "*@dev", "firefly/kernel": "*@dev", "firefly/validation": "*@dev", + "illuminate/auth": "^13.0", "illuminate/contracts": "^13.0", "illuminate/http": "^13.0", "illuminate/log": "^13.0", "illuminate/routing": "^13.0", - "illuminate/support": "^13.0" + "illuminate/support": "^13.0", + "illuminate/validation": "^13.0" }, "extra": { "laravel": { diff --git a/packages/web/src/Error/ErrorFrame.php b/packages/web/src/Error/ErrorFrame.php index 236706cb..c06ea99f 100644 --- a/packages/web/src/Error/ErrorFrame.php +++ b/packages/web/src/Error/ErrorFrame.php @@ -8,6 +8,11 @@ * One stack frame, with the two things that make a trace readable: whether it is YOURS, and what the code * around it says. * + * Both of its halves — the path and the call — are offered SPLIT, because the page prints each on one line + * and a line runs out of width. `dir()`/`base()` and `callQualifier()`/`callFunction()` are the same idea + * twice: a qualifier the row spends first, and the token that identifies the frame and is spent last of + * all — on a phone, where the row wraps rather than shorten it, not at all. + * * A raw PHP trace is forty frames of which perhaps four are the application's, and the rest are the * framework walking its own dispatch. Marking the application's frames is what turns scrolling into * reading, and it is decided by path — a frame under `vendor/` belongs to a dependency — which is crude, @@ -21,6 +26,7 @@ { /** * @param array $excerpt line number => source text, empty for a vendor frame + * @param int $index the frame's 0-based position in the UNTRIMMED stack */ public function __construct( public string $file, @@ -29,5 +35,119 @@ public function __construct( public string $call, public bool $vendor, public array $excerpt = [], + public int $index = 0, ) {} + + /** The directory part of $shortFile, WITH its trailing separator; '' when the path has none. */ + public function dir(): string + { + $at = $this->separator(); + + return $at < 0 ? '' : substr($this->shortFile, 0, $at + 1); + } + + /** + * The file name, which is NEVER shortened. + * + * The page ellipsises the directory when a row runs out of width and never this: a row reading + * `app/Http/Controllers/…` has told a reader nothing, and the file name is the half they came for. + */ + public function base(): string + { + $at = $this->separator(); + + return $at < 0 ? $this->shortFile : substr($this->shortFile, $at + 1); + } + + /** + * The part of $call BEFORE its last qualifier — the class or the namespace — without the separator; + * '' when the call carries none (`throw`, `array_map()`, `{closure}`). + * + * This is the half of a call a row is allowed to clip, and it is the half a path clips too: for + * `Illuminate\Database\Eloquent\Builder->get()` it is everything the neighbouring `Builder.php` already + * says, printed once more only because a reader scanning for a namespace wants to see it. + */ + public function callQualifier(): string + { + $at = $this->callSplit(); + + return $at < 0 ? '' : substr($this->call, 0, $at); + } + + /** + * The function half of $call, WITH the separator that introduces it (`->get()`, `::make()`, `\collect()`). + * + * The LAST token a row shortens, for the same reason base() is never shortened at all. A Laravel trace + * is sixty `Illuminate\…` frames whose qualifiers differ by a segment or two and whose METHOD NAMES are + * the only tokens that tell them apart; a row that clipped its way to `Illuminate\Database\Eloq…` would + * have printed sixty identical lines. Having it in a span of its own is what lets the page rank it + * behind the directory and the qualifier — and, at phone width, wrap the row instead of spending it. + */ + public function callFunction(): string + { + $at = $this->callSplit(); + + return $at < 0 ? $this->call : substr($this->call, $at); + } + + /** + * The offset of the last `->`, `::` or `\` in $call, or -1 when there is none. + * + * Searched only BEFORE the first `{`, because a closure names itself inside braces and PHP 8.4 puts a + * file path in there — `App\Jobs\Sync::{closure:C:\app\Jobs\Sync.php:31}()` carries three separators + * that belong to a Windows path, and cutting at one of those would split the descriptor in half. + */ + private function callSplit(): int + { + $brace = strpos($this->call, '{'); + $scan = $brace === false ? $this->call : substr($this->call, 0, $brace); + + $at = -1; + foreach (['->', '::', '\\'] as $marker) { + $found = strrpos($scan, $marker); + + if ($found !== false && $found > $at) { + $at = $found; + } + } + + return $at; + } + + /** + * The Composer package this frame belongs to (`laravel/framework`), or null for application code. + * + * Read off $shortFile rather than $file, so it answers for exactly the path the page prints — and read + * from the LAST `vendor/`, because a dependency that vendors its own dependencies is still identified by + * the innermost one. The segment must be a whole directory name: `my-vendor/x/y` is a directory whose + * name merely ends in the word, and labelling an application frame `x/y` because of it would be a lie + * the reader has no way to check. + */ + public function package(): ?string + { + foreach (['vendor/', 'vendor\\'] as $marker) { + $at = strrpos($this->shortFile, $marker); + + if ($at === false || ($at !== 0 && $this->shortFile[$at - 1] !== '/' && $this->shortFile[$at - 1] !== '\\')) { + continue; + } + + $parts = preg_split('#[/\\\\]#', substr($this->shortFile, $at + strlen($marker))) ?: []; + + if (count($parts) >= 3 && $parts[0] !== '' && $parts[1] !== '') { + return $parts[0].'/'.$parts[1]; + } + } + + return null; + } + + /** The index of the last directory separator in $shortFile, or -1 when there is none. */ + private function separator(): int + { + $slash = strrpos($this->shortFile, '/'); + $back = strrpos($this->shortFile, '\\'); + + return max($slash === false ? -1 : $slash, $back === false ? -1 : $back); + } } diff --git a/packages/web/src/Error/ErrorPage.php b/packages/web/src/Error/ErrorPage.php index ff32b82c..394cf135 100644 --- a/packages/web/src/Error/ErrorPage.php +++ b/packages/web/src/Error/ErrorPage.php @@ -20,11 +20,13 @@ * welcome page's: centred, generous, a single column. A person meets this page in the same session in which * they meet those two, and a third visual language would just be noise. * - * THE SIGNATURE IS THE TRACE, because that is what the page is FOR. A raw PHP trace is forty frames of which - * four are yours; here the application's frames carry the accent rail and open by default with their source - * excerpt, and the vendor frames collapse to one dim line each. That distinction is the entire difference - * between scrolling a trace and reading one, and it is drawn with `
` and CSS — no JavaScript, so it - * works with scripts disabled and in whatever a container's minimal browser turns out to be. + * THE SIGNATURE IS THE TRACE, because that is what the page is FOR. A raw PHP trace is a hundred frames of + * which ten are yours; here the application's frames are the list — accented, one line each, the first one + * with its source already open — and every dependency frame sits behind a single disclosure below them, + * closed while there is a list above it to read and open when there is not. That split is the entire + * difference between scrolling a trace and reading one, and it is drawn with + * `
` and CSS — no JavaScript, so it works with scripts disabled and in whatever a container's + * minimal browser turns out to be. */ final class ErrorPage { @@ -39,10 +41,10 @@ public static function render(ErrorReport $report, ErrorPageSettings $settings): .'' .'
' .self::header($report, $settings) - .self::facts($report) + .self::facts($report, $settings) .self::detail($report) .self::footer($report, $settings) - .'
'; + .''.self::clipboard($settings, $report->reference !== '').''; } private static function header(ErrorReport $report, ErrorPageSettings $settings): string @@ -64,13 +66,238 @@ private static function header(ErrorReport $report, ErrorPageSettings $settings) if ($report->detailed && $report->message !== '') { $html .= '

'.self::e($report->message).'

'; } elseif (! $report->detailed) { - $html .= '

'.self::e(self::reassurance($report->status, $report->reference)).'

'; + // Not `muted`: the lede is the page's sentence, not an aside beside it. It is the one line a + // production reader is meant to read, and it is now the SAME line the problem document publishes + // for the same failure, so dimming it was the page disagreeing with itself about its own subject. + $html .= '

'.self::e(self::lede($report, $settings)).'

'; } - return $html.''; + return $html.self::actions($report, $settings).''; } - private static function facts(ErrorReport $report): string + /** + * The one sentence a production page says, chosen so that the page and the problem document agree — + * with one declared exception, which is the 405 and is the last paragraph here. + * + * THREE SOURCES, MOST SPECIFIC FIRST. A 405 the ROUTER raised knows something no exception message does + * — which verb the caller used — so it gets a sentence built from both. Then the AUTHORED sentence: + * "Order 42 does not exist.", "No such tenant.", the product's replacement for the router's 404. That is + * what problem+json has always published for the same failure, and a page that said "That page does not + * exist." instead made one error read two ways. Only when neither applies does the page fall back to its + * own reassurance, which is all a 5xx can honestly offer — and all a BARE `abort(403)` can, for which + * "authored" is a word the document uses about a sentence nobody wrote. See authoredSentence(). + * + * THE 405 BRANCH SITS ABOVE THE `authored-detail` GATE, AND IT IS NOT GOVERNED BY IT — which is worth + * stating because the ORDER is what decides it. That key exists to say whether the sentence an + * APPLICATION wrote may reach a person. Nothing of the application's is in this one: the verbs come off + * the `Allow` header the ROUTER put on its own exception, and the words are written by + * ProblemMapper::methodSentence(), which is the framework's own prose for this page and for the + * document alike. So there is nothing for the key to withhold, and turning it off to get the + * status-and-code page leaves this sentence exactly where it was — the page saying about a 405 what the + * document beside it already says in its `allowed` member. ErrorPageSettings and + * skeleton/config/firefly.php state that where an operator reads about the key, and ErrorPageTest pins + * it, because an undocumented exception to a documented key is the same bug as a wrong default. + * + * AND IT IS THE ONE PLACE THE TWO SURFACES DO NOT SAY THE SAME WORDS, which is stated here rather than + * left for a reader to discover, because the headline above would otherwise be read literally. The page + * says "That address does not accept a GET request. It accepts POST." where the document says "This + * address only accepts POST.", and the difference is a clause the document cannot write: it is built + * from the throwable alone and has no request to read the refused verb off. What the two DO share is + * the verb list and the prose that joins it, because both sentences come out of one builder — + * ProblemMapper::methodSentence(), which this branch calls with the request's method and + * methodNotAllowed() calls without one. ErrorPageTest pins the pair against each other, the same way it + * pins the 5xx lede against ProblemMapper::OPAQUE_WITH_REFERENCE, so neither wording can be re-decided + * on its own. + */ + private static function lede(ErrorReport $report, ErrorPageSettings $settings): string + { + if ($report->status === 405 && $report->allowed !== []) { + return ProblemMapper::methodSentence($report->allowed, $report->method); + } + + $authored = $settings->authoredDetail ? self::authoredSentence($report) : ''; + + if ($authored !== '') { + return $authored; + } + + return self::reassurance($report->status, $report->reference); + } + + /** + * The sentence somebody WROTE for this failure, or '' when all that is on offer is the reason phrase. + * + * A BARE `abort(403)` IS NOT AN AUTHORED SENTENCE, and taking it for one is how the most ordinary + * failure a Laravel application produces ended up with the worst lede on this page. + * ProblemMapper::httpMessage() substitutes statusText() for an empty message — the document needs SOME + * `detail`, and "Forbidden" is the honest one there, beside a `title` a machine reads — so + * authoredDetail() answers a non-empty string for every abort() that named no sentence. Printed as the + * lede that read "403 Forbidden" over "Forbidden": the same word twice, the second time in the one slot + * on the page reserved for telling a person something they did not already know. Worse, it made the + * reassurances for 401 and 403 — "You need to sign in to see that.", "You do not have access to that." + * — DEAD CODE at the default configuration, which is the only configuration most deployments run. + * + * So the test is not "is `publicDetail` non-empty" but "does it say anything the page does not already + * say": a value equal to the reason phrase beside the status code, or to the status text for the status, + * is treated as nothing authored and the page falls through to its own sentence. Both spellings are + * checked because they are two different sources — `reason` is what the renderer was handed, `statusText` + * is what ProblemMapper substituted — and they agree only by convention. Nothing an application actually + * wrote is affected: NOTHING_HERE, "No such tenant." and "Order 42 does not exist." are none of them a + * reason phrase, and an `abort(403, 'Forbidden')` that deliberately spells the word gets the sentence + * that explains it instead, which is the better page either way. + */ + private static function authoredSentence(ErrorReport $report): string + { + if ($report->publicDetail === '' || $report->publicDetail === $report->reason) { + return ''; + } + + return $report->publicDetail === ProblemMapper::statusText($report->status) ? '' : $report->publicDetail; + } + + /** + * What a reader can do next — and nothing this deployment did not configure. + * + * Every one of the four production screenshots ends at a fact grid: no link home, no way to sign in + * after a 401, no way to ask again after a 500. The offers are per STATUS AND PER VERB, because a wrong + * offer is worse than none: "Sign in" on a 404 tells a reader they were refused when they were not, and + * "Try again" on a failed POST offers to repeat a request a link is incapable of repeating. + * + * EVERY href ON THIS PAGE HAS PASSED ErrorPageSettings::url() — the configured values when the settings + * object was built, and the request's own address in retry() — so there is exactly one vocabulary for + * what may appear here and exactly one place that knows it. + */ + private static function actions(ErrorReport $report, ErrorPageSettings $settings): string + { + if (! $settings->actions) { + return ''; + } + + /** @var list $links */ + $links = []; + + if ($report->status === 401 && $settings->signIn !== '') { + $links[] = ['href' => $settings->signIn, 'label' => 'Sign in']; + } + + // A 5xx is the one failure whose reader can act without leaving the page they wanted: ask for it + // again. A 4xx cannot be retried into success — the address, the verb or the permission is wrong. + // + // AND ONLY IF THE REQUEST WAS A GET OR A HEAD, because A LINK CANNOT RE-ISSUE A BODY. An `` + // is a GET, whatever the request it claims to repeat: on a POST-only route it lands the reader on + // this wave's OWN 405 page ("That address does not accept a GET request. It accepts POST."), and on + // a route that answers both verbs it silently sends a DIFFERENT request — same address, no form + // fields, no idempotency — while the label says "again". The query string is carried; the verb and + // the body are not, and there is no markup that would carry them without a form and a script this + // page refuses to grow. So the offer is withheld rather than made falsely: a POST that 500s gets + // "Go home" and "Contact support", which are the two things that are actually true for it. + if ($report->status >= 500 && in_array($report->method, ['GET', 'HEAD'], true)) { + $retry = self::retry($report); + + if ($retry !== '') { + $links[] = ['href' => $retry, 'label' => 'Try again']; + } + } + + if ($settings->home !== '') { + $links[] = ['href' => $settings->home, 'label' => 'Go home']; + } + + if ($settings->support !== '') { + $links[] = ['href' => $settings->support, 'label' => 'Contact support']; + } + + if ($links === []) { + return ''; + } + + $html = ''; + } + + /** + * THE REQUEST THAT FAILED, not merely the path it was addressed to — or '' when this page declines to + * spell that address at all. + * + * "Try again" is the primary action on every 5xx, and in its first spelling it dropped the query string: + * `$report->path` comes from Laravel's `path()`, which answers `search` for /search?q=foo&page=2, so a + * reader whose SEARCH had failed was handed a link to an empty one and had to retype what they had + * already typed. The word "again" is a promise about the request, and a link that re-issues a different + * one breaks it silently — the page looks right, and only the reader knows what was lost. + * + * THE ADDRESS GOES THROUGH THE SAME GUARD AS EVERY OTHER HREF ON THIS PAGE, and the first spelling of + * this method did not — it trusted `$report->path` on the strength of a claim that turns out to be + * false. That claim was: the path is '/'-prefixed and ltrim()ed, so the href begins with exactly one + * slash, so it can carry neither a scheme nor a protocol-relative `//host`. The first two clauses hold + * and the conclusion does not, because `//host` is not the only spelling of an authority. Symfony + * refuses a backslash in a request target ONLY inside `Request::create()`; `prepareRequestUri()` — the + * path every real request takes — neither refuses nor normalises one, so a REQUEST_URI of + * `/\evil.example` reaches `path()` as `\evil.example` and this method as `/\evil.example`. For a + * special scheme the URL parser's relative-slash state treats `\` exactly like `/`, so a browser reads + * that as `https://evil.example/` — the PRIMARY action on the page, pointing off the origin, during the + * incident that is exactly when a 5xx lands on an arbitrary path and a reader clicks "Try again". + * + * ErrorPageSettings::url() has refused that spelling for every operator-supplied href since the action + * row existed, along with the tab, LF and CR a parser DELETES wherever they sit. This method asks it the + * same question and accepts the address only when it comes back UNCHANGED — not merely non-empty, since + * a guard that trimmed an edge would hand back a different request than the one that failed, and this + * link's whole promise is that it is the same one. Anything else, and actions() makes no offer: a link + * the page cannot spell truthfully is worse than a row with one fewer button on it. + * + * AND THE FRONT CONTROLLER'S OWN PREFIX IS PART OF THE ADDRESS, which the second spelling of this + * method also dropped. `$report->path` is Laravel's `path()`, which is Symfony's `getPathInfo()` and is + * base-URL-STRIPPED by design: a request for /app/index.php/orders/42 answers `orders/42`, because the + * router matches on the path info and the front controller is not part of what was asked for. Prefixed + * with a slash that is `/orders/42` — a URL the deployment does not serve, so on every box served under + * a base path the PRIMARY action on every 5xx page 404s or leaves the application entirely. The family + * has one settled spelling for this and it is `$request->getBaseUrl().$path`, which LoginPageAction, + * AuthorizationEndpoint, OAuth2LoginPageLinks and FakeAuthorizationServer all use and which + * `it keeps the base path a front controller is served under` pins in firefly/security. The value rides + * on the report as `ErrorReport::$baseUrl` and is '' for the ordinary rewrite-to-the-root deployment, + * where the concatenation is the path unchanged. + * + * THE GUARD SEES THE WHOLE HREF, not its tail. `getBaseUrl()` is raw — it is a prefix of REQUEST_URI + * matched against SCRIPT_NAME, not a value this package composed — so checking `$report->path` and then + * concatenating something in FRONT of it would be asking the question about a string that is not the + * one printed. The concatenation is built first and `ErrorPageSettings::url()` is asked about that, so + * the `/\evil.example` refusal above holds for the href a browser will actually read. + * + * THE QUERY IS SAFE ON ITS OWN TERMS. It is Symfony's `getQueryString()`, which percent-encodes to + * RFC 3986 — `"` is already `%22` + * and `<` is `%3C` before this page escapes anything — so it cannot end the attribute, cannot introduce + * a second `?`, and passes through htmlspecialchars byte for byte except for the `&` between pairs, + * which becomes `&` because that is how an ampersand is spelled inside an HTML attribute value. + * Symfony also sorts the pairs, so the link may read `?page=2&q=foo` where the reader typed + * `?q=foo&page=2`; it is the same request, and normalising is the price of a spelling this page can + * make promises about. + */ + private static function retry(ErrorReport $report): string + { + $target = $report->baseUrl.$report->path; + + if (ErrorPageSettings::url($target) !== $target) { + return ''; + } + + return $report->query === '' ? $target : $target.'?'.$report->query; + } + + /** + * What is true about this request, as a card grid — with the reference as its own composed cell. + * + * THE GRID DRAWS ITS OWN RULES. It used to be `gap:1px` over a line-coloured container, which is a neat + * trick until the fact count is not a multiple of the column count — and the column count is + * `auto-fit`, so it is not knowable here. Six facts in four columns left two DEAD BEIGE CELLS on every + * production page. Now the container is the panel ground and each cell draws a rule up and to the left + * with an outset shadow, which the container's `overflow:hidden` clips on the first row and column; a + * ragged last row is simply panel-coloured, like the panel it is in. + */ + private static function facts(ErrorReport $report, ErrorPageSettings $settings): string { $rows = [ 'Request' => $report->method.' '.$report->path, @@ -78,17 +305,8 @@ private static function facts(ErrorReport $report): string 'Category' => $report->category, 'Severity' => $report->severity, 'When' => $report->timestamp, - 'Reference' => $report->reference, ]; - // The correlation id beside the trace id, and only when the two differ: with tracing off the - // reference IS the correlation id, and a second row repeating it would teach a reader that the two - // ids are interchangeable — which is the confusion keeping them apart exists to prevent. The - // Reference row above is untouched, label and markup both; it is what a person is told to quote. - if ($report->correlationId !== '' && $report->correlationId !== $report->reference) { - $rows['Correlation'] = $report->correlationId; - } - if ($report->detailed) { $rows['Exception'] = $report->exceptionClass; $rows['Thrown at'] = $report->location; @@ -102,9 +320,81 @@ private static function facts(ErrorReport $report): string $html .= '
'.self::e($label).'
'.self::e($value).'
'; } + $html .= self::reference($report, $settings); + + // The correlation id beside the trace id, and only when the two differ: with tracing off the + // reference IS the correlation id, and a second row repeating it would teach a reader that the two + // ids are interchangeable — which is the confusion keeping them apart exists to prevent. + if ($report->correlationId !== '' && $report->correlationId !== $report->reference) { + $html .= '
Correlation
'.self::e($report->correlationId).'
'; + } + return $html.''; } + /** + * The reference, as ONE artefact a reader can take away. + * + * It was printed twice — in the 5xx sentence and in a REFERENCE cell — and neither copy could be + * copied, so the single action a production page offers was "retype this uuid". Now the sentence points + * here, the cell is `user-select:all` (one click takes the whole id, with no JavaScript at all, in + * whatever a container's minimal browser turns out to be), and a copy button is offered on top of that + * where the browser can honour one. + * + * The `
`/`
` pair is byte-for-byte what it was: it is what four tests and a support process both + * read, and the affordance is added BESIDE it in a second `
` — which a definition list allows and a + * `' + : ''; + + return '
Reference
'.$id.'
' + .'
'.$action.'Quote this if you report the problem.
'; + } + + /** + * Ten lines of progressive enhancement, and the only script this page carries. + * + * THE BUTTON SHIPS HIDDEN AND THIS REVEALS IT. A control that does nothing is worse than no control, and + * there are three ordinary ways for the clipboard to be unavailable BEFORE a click: scripts off, a + * Content-Security-Policy that refuses an inline script, and a plain-http origin (navigator.clipboard is + * a secure-context API). In every one of them the button stays hidden, the select-all cell is still + * there, and the page is exactly what it was before. + * + * THOSE THREE ARE NOT ALL OF THEM, which is why the click has a rejection arm. `writeText()` rejects + * with the API present and the guard already passed — the document is not focused (a plain DOMException, + * and the common one), the `clipboard-write` permission is denied, an embedding page's Permissions-Policy + * omits it — and those are exactly the cases where the control has already been REVEALED, so a bare + * `.then()` leaves the one reader who gets here clicking a button that does nothing, silently, while the + * console of the page whose whole job is to be quiet fills with an unhandled rejection. The button says + * so instead, and stays clickable: an unfocused document is transient, a second click after the page has + * focus succeeds, and the `user-select:all` cell beside it is the affordance that never needed a script. + * + * `firefly.web.error-page.copy-button` removes the control and this script entirely, for a deployment + * whose CSP must report zero inline scripts. + */ + private static function clipboard(ErrorPageSettings $settings, bool $rendered): string + { + if (! $settings->copyButton || ! $rendered) { + return ''; + } + + return ''; + } + private static function detail(ErrorReport $report): string { if (! $report->detailed) { @@ -133,50 +423,163 @@ private static function previous(ErrorReport $report): string return $html.''; } + /** + * The trace, split into the frames a reader came for and the ones they came through. + * + * A raw PHP trace is a hundred frames of which ten are the application's, and interleaving them is what + * makes a trace something to scroll rather than something to read. So the application's frames are the + * LIST — accented, in stack order, the first one with source already open — and the dependencies are a + * single disclosure underneath, closed. Nothing is hidden: the count is on both, and one click or one + * Enter opens the whole set. + * + * A STACK WITH NO APPLICATION FRAME IS NOT A REASON TO SHOW NOTHING. The split assumes there is + * something above the disclosure to be a list, and `ErrorFrame::$vendor` is decided by `/vendor/` in + * the path alone — so a stack has none whenever nothing application-owned is on it. Under php-fpm the + * entry script keeps that from happening: `public/index.php` is the application's, so it is the bottom + * frame of every request. It is exactly that floor a WORKER deployment removes — Octane, FrankenPHP + * worker mode and Vapor all boot from a front controller inside `vendor/` — and there every failure + * raised before application code runs (a routing miss, a 405, a container or bootstrap throw) has a + * stack that is dependencies end to end. Closing the disclosure there left the panel as a heading over + * one collapsed row with not a single frame in sight, which is a worse page than the interleaved trace + * this split replaced. So the disclosure is OPEN when it is the only thing in the panel: "nothing is + * hidden" has to hold in the case where hiding is all the page would otherwise do. + * + * Every frame keeps its position in the UNTRIMMED stack (`#37`), so a split list still reads as a stack + * and a budgeted one still says where its gaps are. + * + * THE HEADER COUNTS THE STACK, NOT THE ROWS. `max-frames` trims the list in the report, before any + * markup exists, so counting what is rendered would answer "7 of 40 in your code" for a stack of 104 + * and say nothing at all about the sixty-four frames that were dropped — a label that was honest + * before the budget existed and became a quiet lie the moment it did. The untrimmed totals are carried + * on the report for exactly this, and are named whenever they differ from what is shown; when nothing + * was trimmed the "of" is left out, because "40 of 40" is a question a reader should not have to ask. + */ private static function frames(ErrorReport $report): string { if ($report->frames === []) { return ''; } - $app = 0; + $own = []; + $vendor = []; foreach ($report->frames as $frame) { - if (! $frame->vendor) { - $app++; + if ($frame->vendor) { + $vendor[] = $frame; + } else { + $own[] = $frame; } } - $html = '

Stack trace ' - .self::e((string) $app).' of '.self::e((string) count($report->frames)).' in your code

    '; + $shown = count($report->frames); + $summary = $shown === $report->frameCount + ? $report->frameCount.' frames · '.$report->appFrameCount.' in your code' + : $shown.' of '.$report->frameCount.' frames · '.$report->appFrameCount.' in your code'; - $opened = 0; - foreach ($report->frames as $frame) { - $html .= self::frame($frame, $opened); + $html = '

    Stack trace '.self::e($summary).'

    '; + + if ($own !== []) { + $html .= '
      '; + $opened = false; + foreach ($own as $frame) { + $html .= self::frame($frame, ! $opened && $frame->excerpt !== []); + $opened = $opened || $frame->excerpt !== []; + } + $html .= '
    '; } - return $html.'
'; + return $html.self::dependencies($vendor, $report->frameCount - $report->appFrameCount, $own === []).''; } - private static function frame(ErrorFrame $frame, int &$opened): string + /** + * One application frame: a disclosure when there is source to disclose, a plain row when there is not. + * + * `name="firefly-frame"` is HTML's own exclusive accordion — opening one closes the rest, with no + * JavaScript and no CSS — and `` is focusable and Enter/Space-operable because it is a real + * control. A browser too old for the attribute simply lets several be open, which is the behaviour this + * page had before and is not a failure. + */ + private static function frame(ErrorFrame $frame, bool $open): string { - $where = $frame->shortFile.($frame->line === null ? '' : ':'.$frame->line); - $summary = ''.self::e($where).'' - .''.self::e($frame->call).''; - if ($frame->excerpt === []) { - // No body to expand into, so it renders as a plain row rather than as a control that does - // nothing when clicked. - return '
  • ' - .''.self::e($where).'' - .''.self::e($frame->call).'
    • '; + // No body to expand into, so it renders as a row rather than as a control that does nothing. + return '
  • '.self::row($frame).'
    • '; + } + + return '
  • ' + .''.self::row($frame).'' + .self::excerpt($frame).'
    • '; + } + + /** + * A frame on ONE LINE, whatever the width. + * + * The order is the order a reader scans: where in the stack, whose code, which directory, WHICH FILE, + * which line, what was called. + * + * WHAT THE ELLIPSIS TAKES, AND IN WHICH ORDER. `text-overflow:ellipsis` always drops the END of a span, + * so the only way to say which token gets shortened first is to give each one a span of its own. Four + * do: the directory, the file name, the line, and the call — split again into the qualifier and the + * FUNCTION, `->get()`, because a Laravel trace is sixty `Illuminate\…` frames whose method names are + * the only difference between them, and printing the call as one span clipped exactly that away: sixty + * rows reading `Illuminate\Database\Eloq…`. + * + * With the spans in place the CSS ranks them, and the ranking is the whole design: `.dir` first — + * its innermost directories are a real loss, but the file name, the line and the package badge beside + * it are enough to find the file — then `.cls`, which repeats a class name the file name already gave, + * and only then `.fn`. The file name and the line never shorten at all. What a rank does NOT mean is + * "cannot shrink": a span that refuses to shrink in a row that must keeps its full width and paints + * past the row's edge, where it is cut with no ellipsis at all, so every rank here is a shrink factor + * and `.fn`'s is simply the smallest. On a phone even the last rank is not spent: the row wraps and + * gives the call a line of its own rather than take a character off it. + */ + private static function row(ErrorFrame $frame): string + { + $package = $frame->package(); + $qualifier = $frame->callQualifier(); + + return ''.self::e('#'.$frame->index).'' + .($package === null ? '' : ''.self::e($package).'') + .''.self::e($frame->dir()).'' + .''.self::e($frame->base()).'' + .''.($frame->line === null ? '' : self::e(':'.$frame->line)).'' + .'' + .($qualifier === '' ? '' : ''.self::e($qualifier).'') + .''.self::e($frame->callFunction()).'' + .''; + } + + /** + * Every dependency frame behind one disclosure, with an honest count of what the budget left out. + * + * CLOSED IS A CHOICE ABOUT CONTEXT, NOT A PROPERTY OF THIS SET. It is right whenever the application's + * frames are above it, because then the reader has the frames they came for and this is the stack they + * came THROUGH. When there are none — a routing miss, a 405, anything thrown before application code + * runs, or any failure at all under a front controller that lives in `vendor/` — closing it makes the + * trace panel a heading and a collapsed row with no frame visible at all, so `$open` is passed in by + * the caller rather than decided here: this set does not know whether it is the context or the whole + * trace. + * + * @param list $vendor + * @param int $total dependency frames in the UNTRIMMED stack + * @param bool $open true when this disclosure is the only thing in the panel + */ + private static function dependencies(array $vendor, int $total, bool $open): string + { + if ($vendor === []) { + return ''; } - // The first two frames with source are opened; past that the page becomes a wall of code and the - // reader loses the shape of the stack. - $open = $opened < 2 ? ' open' : ''; - $opened++; + $label = $total.' frame'.($total === 1 ? '' : 's').' in your dependencies'; + $note = count($vendor) === $total ? '' : ''.self::e(count($vendor).' shown').''; - return '
  • '.$summary.self::excerpt($frame).'
    • '; + $html = '
    '.self::e($label).''.$note.'' + .'
      '; + + foreach ($vendor as $frame) { + $html .= '
    1. '.self::row($frame).'
      2. '; + } + + return $html.'
    '; } private static function excerpt(ErrorFrame $frame): string @@ -213,12 +616,22 @@ private static function footer(ErrorReport $report, ErrorPageSettings $settings) /** * A short, honest sentence for a production page — no message, no internals. * - * The 5xx sentence names the request reference, because that is the ONE thing a reader of a production - * page can do about a failure they cannot see: quote the id, so an operator can find the log line it - * stamps. The problem document has said "quote reference " since it carried `traceId`; the page a - * person actually looks at said only that the error had been logged, which left them nothing to quote. - * The wording mirrors ProblemMapper::OPAQUE_WITH_REFERENCE so a ticket reads the same whichever form - * the failure was seen in. + * The 5xx sentence POINTS AT the request reference, because that is the ONE thing a reader of a + * production page can do about a failure they cannot see: quote the id, so an operator can find the log + * line it stamps. The problem document has said "quote reference " since it carried `traceId`; the + * page a person actually looks at said only that the error had been logged, which left them nothing to + * quote. + * + * IT NO LONGER MIRRORS ProblemMapper::OPAQUE_WITH_REFERENCE, and the divergence is deliberate rather + * than drift. This sentence used to name the id inline, word for word as the document does, so a ticket + * read the same whichever form the failure was seen in — but that printed the same uuid twice on every + * production page, in prose and in the Reference cell, with no way to copy either. The page now prints + * it ONCE, in a cell that is `user-select:all` and may carry a Copy button (see reference()), and the + * lede points there. A problem document has no cell to point at, so it keeps the id inline and keeps its + * own wording. What the two surfaces still share is the ID ITSELF — the value a trace search resolves + * and a ticket is filed with — and it is only the SENTENCE that stopped being a cross-surface + * invariant. ErrorPageTest pins that divergence against the constant, so it cannot be quietly re-decided + * in either direction. */ private static function reassurance(int $status, string $reference): string { @@ -229,7 +642,7 @@ private static function reassurance(int $status, string $reference): string $status === 405 => 'That address does not accept this kind of request.', $status >= 500 => $reference === '' ? 'Something went wrong on our side. The error has been logged.' - : "Something went wrong on our side. It has been logged; quote reference {$reference} if you report it.", + : 'Something went wrong on our side. It has been logged; quote the reference below if you report it.', default => 'That request could not be completed.', }; } @@ -250,7 +663,7 @@ private static function css(): string :root{ color-scheme:light; --bg:#f7f6f3; --panel:#fff; --panel-2:#faf9f6; --line:#e7e3db; --line-2:#d6d0c4; - --ink:#20242a; --ink-2:#5f6672; --ink-3:#8d95a1; + --ink:#20242a; --ink-2:#5f6672; --ink-3:#696f7d; --brand:#e07a17; /* The brand as TEXT. #e07a17 is a 3.01:1 foreground on white — fine for a 9px dot or a 3px rail, and unreadable for the exception class it was being used on. Shapes and text need different oranges. */ @@ -265,7 +678,7 @@ private static function css(): string :root{ color-scheme:dark; --bg:#0f1214; --panel:#15191c; --panel-2:#181d21; --line:#252c32; --line-2:#333c44; - --ink:#e8ecef; --ink-2:#9aa5af; --ink-3:#6c7883; + --ink:#e8ecef; --ink-2:#9aa5af; --ink-3:#828e99; --brand:#ff9d3c; --brand-ink:#ff9d3c; --down:#ff8a7a; --down-bg:#2a1614; --warn:#ffc266; --warn-bg:#2a2114; @@ -285,11 +698,32 @@ private static function css(): string .status.down b{color:var(--down)} .status.warn b{color:var(--warn)} .status.idle b{color:var(--idle)} .code{margin:0;font-family:var(--mono);font-size:13px;letter-spacing:.04em;color:var(--ink-2)} .message{margin:6px 0 0;font-size:16px;line-height:1.5;color:var(--ink);overflow-wrap:anywhere} +.acts{display:flex;flex-wrap:wrap;gap:10px;margin-top:16px} +.act{display:inline-flex;align-items:center;font-size:13.5px;font-weight:600;text-decoration:none;padding:7px 14px;border-radius:8px;border:1px solid var(--line-2);color:var(--ink);background:var(--panel)} +.act:hover{border-color:var(--ink-3)} +.act:focus-visible{outline:2px solid var(--brand-ink);outline-offset:2px} +.act.primary{background:var(--ink);color:var(--panel);border-color:var(--ink)} +.act.primary:hover{opacity:.9} .muted{color:var(--ink-2)} -.facts{display:grid;grid-template-columns:repeat(auto-fit,minmax(190px,1fr));gap:1px;margin:0;background:var(--line);border:1px solid var(--line);border-radius:var(--r);overflow:hidden} -.facts>div{background:var(--panel);padding:11px 14px;min-width:0} +.facts{display:grid;grid-template-columns:repeat(auto-fit,minmax(190px,1fr));gap:0;margin:0;background:var(--panel);border:1px solid var(--line);border-radius:var(--r);overflow:hidden} +/* Each cell draws its own rule up and to the left. The first row's and first column's shadows fall outside + the padding box and are clipped by overflow:hidden, so nothing doubles the container border — and a + ragged last row is panel-coloured rather than the dead beige the line-coloured ground used to show. */ +.facts>div{background:var(--panel);padding:11px 14px;min-width:0;box-shadow:-1px -1px 0 var(--line)} .facts dt{font-size:11px;text-transform:uppercase;letter-spacing:.07em;color:var(--ink-2);margin:0 0 3px} .facts dd{margin:0;font-family:var(--mono);font-size:12.5px;overflow-wrap:anywhere} +/* .fact-ref is a div child of .facts, so it already has the cell's ground, padding and rule. Only what + makes it the REFERENCE cell is declared here. One click takes the whole id — no JavaScript at all, and + no dragging a selection across a wrapped uuid. */ +.fact-ref dd:first-of-type{user-select:all;-webkit-user-select:all} +.fact-ref .ref-act{display:flex;align-items:center;gap:8px;margin-top:6px;flex-wrap:wrap} +.fact-ref .hint{font-family:var(--sans);font-size:11.5px;color:var(--ink-2)} +.copy{font:inherit;font-size:11.5px;font-family:var(--sans);color:var(--ink);background:var(--panel-2);border:1px solid var(--line-2);border-radius:6px;padding:2px 9px;cursor:pointer} +.copy:hover{background:var(--bg)} +/* The ring is --brand-ink, not --brand: the button's ground is --panel-2, where #e07a17 measures + 2.86:1 — under SC 1.4.11's 3:1 floor for a non-text indicator. Same token, same reason, as the + two summary rings below. */ +.copy:focus-visible{outline:2px solid var(--brand-ink);outline-offset:1px} .panel{background:var(--panel);border:1px solid var(--line);border-radius:var(--r);overflow:hidden;min-width:0} .panel h2{margin:0;padding:12px 16px;font-size:13px;font-weight:650;border-bottom:1px solid var(--line);background:var(--panel-2);display:flex;justify-content:space-between;gap:12px;align-items:baseline} .panel h2 .n{font-weight:400;font-size:11.5px;color:var(--ink-3);font-family:var(--mono)} @@ -299,23 +733,69 @@ private static function css(): string .chain .cls{margin:0;font-family:var(--mono);font-size:12.5px;color:var(--brand-ink);overflow-wrap:anywhere} .chain .msg{margin:3px 0 0;overflow-wrap:anywhere} .chain .loc{margin:3px 0 0;font-family:var(--mono);font-size:12px;color:var(--ink-3);overflow-wrap:anywhere} -.frames{list-style:none;margin:0;padding:0;counter-reset:f} +.frames{list-style:none;margin:0;padding:0} .frames li{border-bottom:1px solid var(--line)} .frames li:last-child{border-bottom:0} -.frames .row,.frames summary{display:flex;gap:14px;align-items:baseline;padding:8px 16px;min-width:0;flex-wrap:wrap} +/* ONE LINE PER FRAME. The old rule was flex-wrap:wrap with overflow-wrap:anywhere on the path and + margin-left:auto on the call, so every row became a wrapped path plus a third line for the call — an + 87px pitch that made a hundred-frame trace 10,108 pixels tall. Only .dir and .cls may shrink. */ +.frames .row,.frames summary{display:flex;gap:8px;align-items:baseline;padding:7px 16px;min-width:0;flex-wrap:nowrap;white-space:nowrap} .frames summary{cursor:pointer;list-style:none} .frames summary::-webkit-details-marker{display:none} -.frames summary::before{content:"▸";color:var(--ink-3);font-size:10px;margin-right:-6px} +.frames summary::before{content:"▸";color:var(--ink-3);font-size:10px;flex:none} .frames details[open] summary::before{content:"▾"} -.frames .where{font-family:var(--mono);font-size:12.5px;overflow-wrap:anywhere} -.frames .call{font-family:var(--mono);font-size:12px;color:var(--ink-3);overflow-wrap:anywhere;margin-left:auto} -/* The application's own frames are the point of the page; the vendor ones are context. */ +/* THE RING IS DRAWN IN --brand-ink, NOT --brand. A focus indicator is a non-text contrast target: WCAG + 2.1 SC 1.4.11 asks for 3:1 against what it sits on, and #e07a17 measures 3.01:1 on a white panel and + 2.86:1 on --panel-2, which is exactly where the dependency disclosure's ring is drawn. Passing by 0.01 + on one ground and failing on the other is not a contrast decision, it is an accident. --brand-ink is + the token this file already keeps for the brand as a foreground: 5.63:1 and 5.35:1 on those two grounds + in light, and identical to --brand in dark, where both are #ff9d3c. */ +.frames summary:focus-visible{outline:2px solid var(--brand-ink);outline-offset:-2px} +.frames .ix{font-family:var(--mono);font-size:11px;color:var(--ink-3);flex:none;min-width:2.8em;text-align:right} +.frames .pkg{font-size:11px;line-height:1.7;color:var(--ink-2);background:var(--panel-2);border:1px solid var(--line);border-radius:5px;padding:0 5px;flex:none;max-width:13em;overflow:hidden;text-overflow:ellipsis} +.frames .dir{font-family:var(--mono);font-size:12.5px;color:var(--ink-2);min-width:0;flex:0 100 auto;overflow:hidden;text-overflow:ellipsis} +.frames .base{font-family:var(--mono);font-size:12.5px;color:var(--ink);flex:none} +.frames .ln{font-family:var(--mono);font-size:12.5px;color:var(--ink-2);flex:none} +/* The call is the path's twin: .cls is the qualifier and may be ellipsised, .fn is the method name and is + the token that tells sixty Illuminate frames apart, so it is the LAST thing the row gives up. A nested + flex rather than two row items, so the pair stays glued (the row's 8px gap would print + `Collection ->each()`). + + WHAT "LAST" IS MADE OF, because `flex:none` did not mean it. An unshrinkable span inside a shrinking row + does not stay WHOLE, it stays WIDE: its box keeps its content width, its own `text-overflow` therefore + never has a narrower box to draw an ellipsis in, and the glyphs simply run out of the row and are cut by + `.panel{overflow:hidden}` with nothing to mark the cut. Measured in Chrome at 375px against a 35-frame + Laravel trace, 34 of 35 rows painted their method name up to 138px beyond the panel's right edge, and + `->whereHasMorphRelationship` arrived on screen as `->wh`. + So the ORDER is stated with shrink factors rather than by refusing to shrink: .dir and .cls shrink at + 100 and .fn at 1, which spends the two discardable spans down to nothing before flexbox takes a single + character off the method name — and `overflow:hidden` here means that whatever is taken is taken inside + the row, with an ellipsis, instead of outside it in silence. At a 560-920px panel that ranking leaves + every method name whole except the closure descriptor below. */ +.frames .call{font-family:var(--mono);font-size:12px;color:var(--ink-3);margin-left:auto;min-width:0;flex:0 1 auto;display:flex;align-items:baseline;overflow:hidden} +.frames .call .cls{min-width:0;flex:0 100 auto;overflow:hidden;text-overflow:ellipsis} +/* The cap is the one guard: PHP 8.4 names a closure `{closure:/abs/path/file.php:14}`, so a "function + name" can be a hundred characters of absolute path. It sits far above any real method name and bites + only that case, which would otherwise take the whole row's width for one frame's descriptor. */ +.frames .call .fn{min-width:0;flex:0 1 auto;max-width:24em;overflow:hidden;text-overflow:ellipsis} +/* The application's own frames are the point of the page; the dependency ones are context. */ .frames li.own{border-left:3px solid var(--brand);background:var(--panel)} -.frames li.own .where{color:var(--ink);font-weight:600} +.frames li.own .base{font-weight:600} .frames li.vendor{border-left:3px solid transparent;background:var(--panel-2)} -/* De-emphasised by weight, ground and the missing rail — NOT by fading the text below readable contrast. - A vendor frame's path is still the thing a reader came for once they have ruled their own code out. */ -.frames li.vendor .where{color:var(--ink-2);font-weight:400} +/* De-emphasised by weight, ground and the missing rail — NOT by fading text below readable contrast. */ +.frames li.vendor .base{font-weight:400;color:var(--ink-2)} +.deps{border-top:1px solid var(--line);background:var(--panel-2)} +/* …except when it follows the heading directly, which is the vendor-only stack: the heading already draws + a border-bottom, and two adjacent 1px rules paint as one 2px one under a heading and nowhere else. */ +.panel h2+.deps{border-top:0} +.deps>summary{display:flex;gap:12px;align-items:baseline;padding:10px 16px;cursor:pointer;list-style:none;font-size:12.5px;color:var(--ink-2)} +.deps>summary::-webkit-details-marker{display:none} +.deps>summary::before{content:"▸";color:var(--ink-3);font-size:10px} +.deps[open]>summary::before{content:"▾"} +/* The one control a keyboard reader MUST operate to reach the dependency frames, on --panel-2. */ +.deps>summary:focus-visible{outline:2px solid var(--brand-ink);outline-offset:-2px} +.deps .dn{margin-left:auto;font-family:var(--mono);font-size:11.5px;color:var(--ink-3)} +.deps-list{border-top:1px solid var(--line)} .src{width:100%;border-collapse:collapse;font-family:var(--mono);font-size:12.5px;background:var(--panel-2);border-top:1px solid var(--line);display:block;overflow-x:auto} .src tr{display:table;width:100%;table-layout:fixed} .src td{padding:2px 10px;white-space:pre;vertical-align:top} @@ -328,7 +808,31 @@ private static function css(): string @media (max-width:560px){ .sheet{padding:32px 14px 48px} .status b{font-size:48px} - .frames .call{margin-left:0;width:100%} + .frames .pkg{display:none} + /* The qualifier goes entirely, before the method name loses a character: on a phone row + `Illuminate\Database\Eloquent\Builder` is what `Builder.php` two columns to its left already said, + and `->get()` is not said anywhere else. */ + .frames .call .cls{display:none} + /* AND THEN THE PHONE ROW WRAPS, because at 375px it provably cannot do both. A row has 295px there, and + `AddQueuedCookiesToResponse.php` — a real Laravel file name, which never shortens — is 226 of them; + `:46` and `->handle` have to go somewhere. Ranked shrinking has an answer for that and it is the wrong + one: measured at 375px it spent .fn down until 23 of 35 method names had lost characters and two had + lost all of them, rendering at zero width. So the phone takes the other branch — one more line instead + of a shorter name — and the directory goes with .pkg and .cls, because it was already ellipsised to + `vendor/la…` on every row at this width and dropping it is what holds the wrapped row to two lines + instead of the four a full-width .dir forces (measured: 1,961px of trace against 3,406px). + This is NOT the pre-wave rule that made a hundred frames 10,108 pixels tall. That one wrapped the PATH + ITSELF, with overflow-wrap:anywhere, at 87px a row; nothing here wraps inside a span, and the break + can only fall between two whole tokens. */ + .frames .dir{display:none} + .frames .row,.frames summary{flex-wrap:wrap} + .frames .call{margin-left:0} + .frames .ix{min-width:2.2em} + /* The call keeps the 24em guard it has everywhere and no tighter one. A phone used to cap it at 14em, + which was the right cap while the call had to share a line with a file name and was the wrong one the + moment it stopped: on its own 295px line, 14em cut `->sendRequestThroughRouter` and + `->whereHasMorphRelationship` for nothing. 24em still fits that line, and still bites the closure + descriptor it exists for. */ } CSS; } diff --git a/packages/web/src/Error/ErrorPageRenderer.php b/packages/web/src/Error/ErrorPageRenderer.php index 027fdd7d..e1274d5e 100644 --- a/packages/web/src/Error/ErrorPageRenderer.php +++ b/packages/web/src/Error/ErrorPageRenderer.php @@ -6,9 +6,13 @@ use DateTimeImmutable; use DateTimeInterface; +use Firefly\Kernel\Exception\FireflyException; +use Illuminate\Auth\AuthenticationException; use Illuminate\Contracts\View\Factory as ViewFactory; +use Illuminate\Http\Exceptions\HttpResponseException; use Illuminate\Http\Request; use Illuminate\Http\Response; +use Illuminate\Validation\ValidationException; use Throwable; /** @@ -40,6 +44,43 @@ public function __construct( private readonly ?ViewFactory $views = null, ) {} + /** + * Whether this package may answer for this throwable AT ALL — asked before either negotiation below, + * because both of them look at the REQUEST and neither looks at what was thrown. + * + * THREE THROWABLES BELONG TO LARAVEL'S OWN HANDLER, AND CLAIMING THEM DESTROYS THE FAILURE. + * Handler::render() consults renderViaCallbacks() — where this package's renderable lives — BEFORE its + * own `match (true)`, whose arms resolve `HttpResponseException` (which literally CARRIES the response + * to return), `AuthenticationException` (401, or the guest redirect to the login page) and + * `ValidationException` (422 with the field errors, or a redirect back with them in the session). Not + * one of the three is a FireflyException, and not one implements HttpExceptionInterface, so + * ProblemMapper::toFireflyException() drops every one of them to its default arm and answers + * 500 / `INTERNAL_ERROR` / "An unexpected error occurred." — an opaque internal error in place of a + * precisely described one, reported as a 500 into the bargain. A form POST that failed validation came + * back as a 500 with no `errors` member in it; a 401 came back as a 500. + * + * That is exactly the silent failure this whole surface exists to remove, so the rule is the plain one: + * a throwable Laravel resolves for itself is not this package's to describe, and the renderable returns + * null for it — for the PAGE as well as for the problem document, because `handles()` reads the Accept + * header alone and would otherwise draw a diagnostic 500 page over a browser's redirect-back-with-errors. + * + * THE LIST IS NAMED, AND THE TRIPWIRE UNDER IT IS NOT THIS PREDICATE'S OWN TEST. ErrorPageTest asserts + * this method against each of the three, which pins what THIS method does and would go on passing for + * ever if Laravel grew a fourth arm: the predicate knows nothing about the handler, so a test that + * constructs the three exceptions itself cannot notice a fourth. That test is therefore not the + * protection, and saying it was would have told a maintainer on a Laravel upgrade that the list + * re-checks itself. LaravelHandlerArmsTest is the protection: it reads `Handler::render()` out of the + * INSTALLED Laravel through reflection, extracts the classes its `match (true)` resolves, and fails + * unless that set is exactly these three — so a release that adds a fourth arm is a failing test naming + * the class to add here, and not a silent 500 in production. + */ + public function describes(Throwable $e): bool + { + return ! $e instanceof ValidationException + && ! $e instanceof AuthenticationException + && ! $e instanceof HttpResponseException; + } + /** Whether this request should be answered with the HTML page rather than with problem+json. */ public function handles(Request $request): bool { @@ -95,6 +136,61 @@ public function forcesJson(Request $request): bool return $this->settings->enabled && $this->settings->isJsonPath($request->path()); } + /** + * Whether this failure is answered with a problem document — the WHOLE of that decision, asked after + * `handles()` has already answered "is this a page". + * + * THE BUG THIS FIXES. The rule used to live in WebServiceProvider as + * `$e instanceof FireflyException || $request->expectsJson() || $page->forcesJson($request)`, and its + * gap was the commonest client there is. A WILDCARD Accept header is what a bare `curl` sends and what + * `fetch()` sends by default; an absent Accept is what a hand-rolled client sends. Neither NAMES + * text/html, so neither got the page — and `expectsJson()` is false for both (`wantsJson()` tests the + * FIRST acceptable type, and a wildcard is not a JSON type; `ajax()` is false) — so a router 404 on a + * path outside `json-paths` fell all the way through to Laravel's stock HTML page. A client that asked + * for anything was handed markup, while the documentation had promised it a problem document since the + * page shipped. + * + * THE FALLBACK IS THE LAST TERM, NOT THE FIRST. A FireflyException, a JSON client and a `json-paths` URL + * are answered exactly as they were; this term only adds an answer where the package previously gave + * none. + * + * AND IT CLAIMS EVERY CALLER THAT IS NEITHER A BROWSER NOR A JSON CLIENT — not only the one that named + * nothing at all. `! prefersHtml()` is true of a wildcard Accept and of an absent one, the two cases + * this term was written for, and it is equally true of `Accept: application/xml`, `text/plain` or + * `image/png`: a caller that named a concrete type this package does not render an error in. That is + * deliberate, and it is the narrower rule that would be the inconsistency. A FireflyException has + * ALWAYS been answered with problem+json whatever the Accept header said — the first term above, + * unchanged since before this method existed — so a fallback restricted to a literal wildcard would + * hand one XML client a problem document for a taxonomy 404 and Laravel's stock HTML page for a router + * 404, which is one application answering one failure two unrelated-looking ways and is the exact shape + * the class comment above opens by describing. Nor is there a third shape to offer: the + * MessageConverterRegistry converts what a CONTROLLER returns and an application may well add XML to + * it, but neither error renderer is wired through it, and problem+json is the only machine-readable + * document this package writes. So the key is `problem-fallback` rather than a wildcard-shaped name, + * and every sentence that documents it says what it actually claims: a caller that did not name + * text/html and did not ask for JSON. + * + * IT IS ALSO GATED ON THE FLAG, FOR `forcesJson()`'S REASON. The fallback asks `prefersHtml()`, and + * `prefersHtml()` folds `json-paths` in — so without `enabled` on the term, an `api/*` URL hit by a + * BROWSER would be claimed here with the page switched off, which is precisely the answer the same flag + * on `forcesJson()` was written to withhold (`forces nothing at all when the page is switched off`). + * One key would have meant two things: "do not draw the page" for one branch and "draw nothing at all" + * for the branch beside it. `firefly.web.error-page.enabled => false` keeps its documented meaning — + * this package stops adding answers and Laravel's own handler is left to it — while a FireflyException + * and a JSON client, which were never gated on the flag, are still answered exactly as before. + * + * AND A THROWABLE LARAVEL RESOLVES ITSELF NEVER REACHES HERE: see describes(), which the renderable + * asks first, and which is the only part of this negotiation that looks at what was thrown. + */ + public function rendersProblem(Throwable $e, Request $request): bool + { + if ($e instanceof FireflyException || $request->expectsJson() || $this->forcesJson($request)) { + return true; + } + + return $this->settings->enabled && $this->settings->problemFallback && ! $this->prefersHtml($request); + } + public function render(Throwable $e, Request $request): Response { $exception = ProblemMapper::toFireflyException($e); diff --git a/packages/web/src/Error/ErrorPageSettings.php b/packages/web/src/Error/ErrorPageSettings.php index ffcfc70c..98036562 100644 --- a/packages/web/src/Error/ErrorPageSettings.php +++ b/packages/web/src/Error/ErrorPageSettings.php @@ -23,6 +23,20 @@ * status (or `default`) to the application's own Blade view, so a public 404 can be the product's own page * while a 500 in staging is still the framework's diagnostic one. * + * `problem-paths` IS NOT A KEY AND `problem-fallback` IS. The question `json-paths` answers is "which URLs + * are machine surfaces"; the question this one answers is "what does a caller that is not a browser and did + * not ask for JSON get". They are different questions and only the first is about the URL. With the + * fallback on — the default, and what the documentation has always claimed — such a request is answered + * with the problem document, because that is the form a client can read and the page is for a person who + * asked for one. That population is wider than the wildcard it was written for: a WILDCARD Accept header + * and an absent one are in it, and so is a caller that named a concrete type this package cannot render an + * error in (`application/xml`, `text/plain`), which gets the document rather than Laravel's markup for the + * reason ErrorPageRenderer::rendersProblem() sets out — a FireflyException has always answered that same + * caller with problem+json, and the narrower rule would be the inconsistency. Off, such a request falls + * through to Laravel's handler exactly as it used to, and so it does whenever `enabled` is off: the + * fallback is a second answer this package offers, and `enabled => false` withdraws the answers rather than + * changing which one is given. + * * `trace` DEFAULTS TO `app.debug` and is enforced at render time, not merely at template time — the renderer * builds no frame list, opens no source file and copies no exception message when it is off. That is * deliberate: a page that assembled the details and then declined to print them would put a stack trace one @@ -36,17 +50,95 @@ * person who wants a driver message inside a JSON `detail` says so with `firefly.web.problem.disclose=true`; * nothing infers it. The two gates are independent so that turning one on never opens the other. * - * WHAT PRODUCTION SEES with `trace` off is the status, the reason phrase and the stable error code — the - * same `code` the problem+json carries, so a user can quote it into a support ticket and an operator can - * find it in the log. Not the message: an exception message is written for a developer and routinely names - * a table, a column, a class or an id. And not the footer's own advice about turning the trace on, which is - * useful on a staging box and is a free hint about the stack to anyone else — see `hints`. + * WHAT PRODUCTION SEES with `trace` off is the status, the reason phrase, the stable error code and the + * request's reference — the same `code` and `traceId` the problem+json carries, so a user can quote them + * into a support ticket and an operator can find them in the log. Never the RAW exception message: that + * sentence was written for a developer and routinely names a table, a column, a class or an id. And not the + * footer's own advice about turning the trace on, which is useful on a staging box and is a free hint about + * the stack to anyone else — see `hints`. + * + * THE ONE ADDITION IS AUTHORED, IT HAS ITS OWN KEY, AND IT IS THE PAGE'S LEDE. With `authored-detail` on + * (the default) the production page says the sentence the application ITSELF wrote for a caller — a + * FireflyException's "Order 42 does not exist.", or an `abort(404, 'No such tenant.')` — carried on the + * report as `ErrorReport::$publicDetail`, because that is exactly what the problem document beside it + * publishes as `detail`, and one failure reading two ways depending on which surface answered is its own + * kind of bug. None of this is an exemption from the paragraph above: ProblemMapper decides what counts as + * authored, withholds everything at 500 and above, and replaces the sentences the FRAMEWORK generated — + * the router's "The route … could not be found." and the route-model-binding 404s Laravel rewrites into + * it, which name a model class and a primary key. Turn the key off and `$publicDetail` is '', the lede + * goes back to the generic reassurance for the status, and there is no authored sentence for any renderer + * — this page or an application's own error view, which is handed the same report — to reach for at all. + * + * A BARE `abort(403)` AUTHORED NOTHING, and the page reads it that way whatever this key says. The REPORT + * still carries "Forbidden", because that is what the problem document publishes as `detail` and this + * property mirrors the document; the PAGE declines to lede with a word already printed beside the status + * code, and says "You do not have access to that." instead. That rule lives in + * ErrorPage::authoredSentence(), on the page, because it is a judgement about what a person is told rather + * than about what a caller is sent. + * + * WITH ONE EXCEPTION, AND IT IS NOT AN AUTHORED SENTENCE. A 405 the router raised keeps its verb sentence + * — "That address does not accept a GET request. It accepts POST." — whichever way this key is set, because + * nothing in it came from the application: ProblemMapper reads the verbs off the `Allow` header the ROUTER + * put on its own exception, and ProblemMapper::methodSentence() writes the words for the page and for the + * document alike. This key governs the DISCLOSURE of what an + * application said, and there is none to govern there — the same list of verbs is the `allowed` member of + * the problem document published for the same failure. An operator who turns the key off for the + * status-and-code page gets it everywhere else and gets this sentence still. */ final readonly class ErrorPageSettings { + /** + * WHERE A READER CAN GO NEXT — and the three values this class refuses to hold a hostile spelling of. + * + * THESE THREE ARE NOT PROMOTED, AND THAT IS THE WHOLE POINT. Each one is destined for an `href` on a + * page the framework hands itself, so the guard has to run on every construction path rather than on + * the one that happens to read configuration: `firefly/security` builds an ErrorPageSettings by hand + * for its login page, and a dozen tests construct one directly. A promoted property would put the + * caller's string into the object with nothing in between, and the guarantee this class advertises — + * that it cannot HOLD an unsafe URL, whoever built it — would have been true only of fromConfig(). The + * constructor body assigns each of them through self::url(), so it is true of all of them. + * + * WHAT PRINTS THEM is the action row in ErrorPage::actions(), which offers the one that fits the status + * — `signIn` on a 401, `home` and `support` wherever they are set — and offers nothing it was not + * given: an empty value produces no link rather than a guessed route name. The row was written against + * properties that were already safe, which is the point of guarding here instead of at the point of + * printing. See self::url() for the vocabulary, and `actions` below for switching the row off entirely. + */ + public string $home; + + public string $signIn; + + public string $support; + + /** + * THE FOURTH OPERATOR-SUPPLIED URI, AND THE ONLY ONE THAT NEVER REACHES AN `href`. + * + * RFC 9457 §3.1.1's `type` is a URI that identifies the KIND of problem, and the RFC's own words for it + * are "dereferenceable" — the member exists so that a person can open it. That is what makes it the same + * hazard as the three above wearing different clothes: nothing on the error PAGE prints it, so the + * `href` audit that produced self::url() looked straight past it, and every API console, every IDE HTTP + * client and every documentation viewer that renders a problem document turns `type` into a link. A + * `javascript:` base configured here is the same stored XSS the three properties above are guarded + * against, published to a wider audience and arriving in a tool the reader trusts more than a 500 page. + * + * THE SENTINEL IS COMPARED WITH `===`, WHICH IS WHY THE TRIM IS NOT COSMETIC. ProblemType::of() asks + * whether the base IS 'about:blank' and whether it IS '', and a Helm block scalar's trailing newline or + * a here-doc's trailing space makes both answers false — so the deployment that meant "leave the default + * alone" silently entered BASE-URI mode and published `" about:blank/resource-not-found"` as a + * dereferenceable URI. Config::string() does not trim, Laravel's Env does not trim a REAL environment + * variable, and this was the one configuration-supplied URI in this class that reached its consumer + * verbatim. See self::typeUri() for the vocabulary, which is self::url()'s with one member swapped. + */ + public string $typeUri; + /** * @param list $jsonPaths path patterns that are answered as problem+json whatever the client asked for * @param array $views status (or `default`) => the Blade view to render instead + * @param string $home the "Go home" target; '' offers no link. Guarded: see self::url() + * @param string $signIn the 401's sign-in target; '' offers no link. Guarded: see self::url() + * @param string $support the "Contact support" target; '' offers no link. Guarded: see self::url() + * @param bool $actions whether the page offers any navigation at all + * @param string $typeUri the RFC 9457 `type` base; '' omits the member. Guarded: see self::typeUri() */ public function __construct( public bool $enabled = true, @@ -57,7 +149,33 @@ public function __construct( public array $jsonPaths = ['api/*'], public array $views = [], public bool $disclose = false, - ) {} + string $home = '/', + string $signIn = '', + string $support = '', + public bool $actions = true, + // HOW MANY FRAMES THE PAGE BUILDS AT ALL. Applied as a trim in ErrorReport, before markup: a page + // that renders a hundred frames and hides ninety has still escaped and shipped a hundred. + public int $maxFrames = 40, + // Whether the report CARRIES the sentence the problem document publishes, so a page can use it as + // its lede. Off, `ErrorReport::$publicDetail` is '' and there is nothing for a renderer to print. + // What counts as authored is ProblemMapper's decision, not this key's — see the class comment. + public bool $authoredDetail = true, + // Progressive enhancement, and the only script this page has ever carried: see ErrorPage::clipboard(). + public bool $copyButton = true, + // Whether a caller that is neither a browser nor a JSON client — a wildcard Accept header, no + // Accept at all, or a named type this package renders no error in — gets a problem document rather + // than Laravel's own page. See ErrorPageRenderer::rendersProblem() for the case this closes. + public bool $problemFallback = true, + // RFC 9457 §3.1.1's `type`: 'about:blank' (the default, and what Spring's ProblemDetail emits), '' + // to omit the member, or a BASE URI from which the stable error code derives one. See ProblemType. + // Guarded like the three above, through the same constructor seam: see the property's docblock. + string $typeUri = ProblemType::BLANK, + ) { + $this->home = self::url($home); + $this->signIn = self::url($signIn); + $this->support = self::url($support); + $this->typeUri = self::typeUri($typeUri); + } /** * Whether $path is one this application serves as an API, and therefore must answer with a problem @@ -118,6 +236,20 @@ public static function fromConfig(Config $config): self // Explicit, and only explicit: no fallback to app.debug, no fallback to `trace`. See the class // comment for the leak that a shared gate produced. disclose: $config->bool('firefly.web.problem.disclose', false), + typeUri: $config->string('firefly.web.problem.type-uri', ProblemType::BLANK), + // Handed over RAW: the constructor runs each of these through url(), so this call site cannot + // be the one that forgets. The default home is the site root, because a page with no way off it + // is the state every one of these screenshots was in. + home: $config->string('firefly.web.error-page.home', '/'), + signIn: $config->string('firefly.web.error-page.sign-in', ''), + support: $config->string('firefly.web.error-page.support', ''), + actions: $config->bool('firefly.web.error-page.actions', true), + // Clamped rather than trusted, like excerpt-lines above it: 0 would render a trace with no + // frames in it, and a million would put the 10,108-pixel page back. + maxFrames: max(1, min(500, $config->int('firefly.web.error-page.max-frames', 40))), + authoredDetail: $config->bool('firefly.web.error-page.authored-detail', true), + copyButton: $config->bool('firefly.web.error-page.copy-button', true), + problemFallback: $config->bool('firefly.web.error-page.problem-fallback', true), ); } @@ -145,4 +277,117 @@ private static function views(array $configured): array return $views; } + + /** + * A URL this page will put in an `href`, or '' when it is not one. + * + * IT IS PUBLIC BECAUSE IT IS THE PACKAGE'S WHOLE VOCABULARY FOR "SAFE HERE", and there is one href on + * the page that is not operator-supplied: the "Try again" link, which is the address the REQUEST was + * sent to. That one skipped this method in its first spelling and trusted Laravel's `path()` instead — + * and `path()` will hand back `\evil.example` for a REQUEST_URI of `/\evil.example`, because Symfony + * rejects a backslash in a request target only in `Request::create()`, never in the `prepareRequestUri()` + * path a real request takes. Prefixed with a slash that is `/\evil.example`, which the paragraph below + * spends nine lines explaining is an authority wearing a path's clothes. A second guard would have been + * a second thing to keep right; ErrorPage::retry() calls THIS one and refuses any address it alters or + * drops. The constructor's three assignments below are the other callers. + * + * THE ATTACK THIS CLOSES. These values arrive from configuration, which in a real deployment means a + * templated environment variable — a Helm value, a CI-rendered .env, a tenant-provisioning job. The page + * runs them through htmlspecialchars, which escapes quotes and angle brackets and does NOTHING to a + * scheme: `javascript:alert(document.cookie)` reaches the DOM intact, on the application's own origin, + * on a page a person opens while already confused. That is stored XSS the framework hands itself. + * + * THE ALLOW-LIST IS THE WHOLE VOCABULARY, and it is deliberately small. An ABSOLUTE PATH (`/login`) is + * the normal answer and cannot carry a scheme. An `http(s)://` URL is the other one. Everything else is + * dropped: `data:` and `vbscript:` are the other two script-bearing schemes, `file:` is not a link a + * browser should follow from here, `mailto:` is a legitimate wish that this page does not serve (put the + * address behind an https support URL), and a protocol-relative `//host/…` is refused because it + * silently leaves the origin — which on an error page is indistinguishable from a phishing redirect. + * + * A PATH IS ONLY A PATH IF A BROWSER READS IT AS ONE, and two rules of the URL standard make that a + * narrower set than "begins with a slash". For a special scheme the parser's relative-slash state treats + * `\` EXACTLY LIKE `/`, so `/\host/…` is the protocol-relative case wearing a different separator: + * Chrome, Firefox and Safari all resolve `/\evil.test/phish` against this origin as + * `https://evil.test/phish`, which is the classic bypass of a filter that only looks for `//`. And + * before any of that the parser DELETES every ASCII tab, LF and CR from the input, so `//evil.test` + * IS `//evil.test` by the time anything reads it. Both are refused: the second character of a path may + * not open an authority, and a value carrying INSIDE it a character the parser would delete is DROPPED + * rather than normalised — a guard that keeps a string the browser will re-read differently has decided + * nothing. `/` alone, the default home, is the one path with no second character and is kept by name. + * + * Refusing those characters instead of stripping them is also what keeps the scheme test honest. + * `java\tscript:` is only a javascript: URL because a browser strips the tab; this method never has to + * decide what the browser means by it, because the value is gone before either branch runs. + * + * THE EDGES ARE THE PARSER'S BUSINESS, AND TRIMMING THEM IS THAT SAME RULE READ PROPERLY rather than a + * softening of it. Before it does anything else the standard strips every LEADING and TRAILING C0 + * control and space from the input, so a value padded at its edges is re-read by the browser as EXACTLY + * the value this method would have allowed: nothing is left undecided, and refusing it would delete a + * link an operator configured over a character no reader will ever see. The deployment mechanism this + * method exists for is the one that adds them — a Helm block scalar and a here-doc-rendered `.env` both + * end in a newline, and Laravel's `Env` does not trim a REAL environment variable the way Dotenv trims + * a `.env` line — and a trailing space was already being KEPT verbatim here while the newline spelling + * of the same padding dropped the whole link. So the edges are trimmed first and every refusal below is + * about the INTERIOR. That gives up no ground, because each hostile value trims into another this + * method already refuses: `//evil.test` into `//evil.test`, `/\evil.test` into `/\evil.test`, + * `javascript:…` into `javascript:…`. + */ + public static function url(string $value): string + { + $value = trim($value, "\x00..\x20"); + + if ($value === '' || strpbrk($value, "\t\n\r") !== false) { + return ''; + } + + if (preg_match('#^https?://#i', $value) === 1) { + return $value; + } + + return $value === '/' || preg_match('#^/[^/\\\\]#', $value) === 1 ? $value : ''; + } + + /** + * A base this class will let ProblemType build a published `type` out of, or the RFC's own sentinel. + * + * IT IS self::url()'s VOCABULARY WITH ONE MEMBER SWAPPED, and the swap is the whole difference between + * the two methods. An `href` on the error page may be an ABSOLUTE PATH, because a page is served from an + * origin and a path resolves against it. A problem `type` may not: RFC 9457 §3.1.1 wants a URI that + * identifies the problem kind across deployments, ProblemType::of() appends a slug to it, and a relative + * base would produce a type that means a different thing read from a different document. So the allowed + * set here is `''` (omit the member), the `about:blank` sentinel, and an absolute `http(s)://` base — + * nothing else. Everything self::url() refuses is refused for self::url()'s reasons, on top: a + * `javascript:` base is the same stored XSS in a member a console renders as a link, and `//evil.test` + * is the same silent change of origin. + * + * THE FALLBACK IS THE DEFAULT, NOT SILENCE. A value this method cannot read becomes 'about:blank' — the + * documented default and the RFC's own "no specific type" — rather than '' , because '' is a deliberate + * position an operator takes (publish the pre-9457 document byte for byte) and a typo must not be able + * to take it for them. A hostile base is answered by removing the hostility, not by removing the member. + * + * THE EDGES ARE TRIMMED FIRST, for self::url()'s reason and one more that is specific to this key. The + * URL standard strips leading and trailing C0 controls and space before it parses, so a padded value is + * re-read by every consumer as exactly the value this method allows; and ProblemType::of() compares the + * base to its two sentinels with `===`, so an untrimmed `"about:blank\n"` — which is what a Helm block + * scalar and a here-doc-rendered `.env` both hand over — would miss BOTH branches and publish + * `"about:blank\n/resource-not-found"` as a dereferenceable URI. Trimming is what makes the sentinels + * mean what an operator wrote. An INTERIOR tab, LF or CR is the opposite case and is refused, exactly as + * in self::url(): the parser deletes those from the middle, which is the whole reason `javascript:` + * is a javascript: URL, and a guard that keeps a string the reader will re-read differently has decided + * nothing. + */ + public static function typeUri(string $value): string + { + $value = trim($value, "\x00..\x20"); + + if ($value === '' || $value === ProblemType::BLANK) { + return $value; + } + + if (strpbrk($value, "\t\n\r") !== false) { + return ProblemType::BLANK; + } + + return preg_match('#^https?://#i', $value) === 1 ? $value : ProblemType::BLANK; + } } diff --git a/packages/web/src/Error/ErrorReport.php b/packages/web/src/Error/ErrorReport.php index 35e8ca7b..5fe5d6d2 100644 --- a/packages/web/src/Error/ErrorReport.php +++ b/packages/web/src/Error/ErrorReport.php @@ -23,6 +23,12 @@ * ErrorResponse::fromException() the JSON renderer uses, so the page a browser sees and the payload a client * sees describe the same error with the same vocabulary. A support ticket quoting the code off the page * finds the same code in the log. + * + * PATHS ARE SHORTENED BY SourcePaths, NOT HERE. Three lines of str_starts_with used to live in this class, + * and they were the reason a 500 page measured 10,108 pixels: they miss whenever a deployment names one + * directory two ways, and then not one of a hundred rows is shortened. The rule is a collaborator now + * because it is worth unit-testing on its own — a symlinked base path is a case no rendered trace can be + * asked to produce. */ final readonly class ErrorReport { @@ -38,8 +44,35 @@ * rows holding one value teach a reader that the ids are interchangeable, which is the confusion the * two members exist to prevent. * + * `path` AND `query` ARE TWO FIELDS BECAUSE THEY ARE TWO PROMISES. `path` is root-relative and built + * as '/'.ltrim($request->path(), '/'), which is what makes it safe to put in an href: it begins with + * exactly one slash, so it cannot carry a scheme and cannot become protocol-relative. `query` is what + * Laravel's `path()` THROWS AWAY — it answers `search` for /search?q=foo&page=2 — and the page's "Try + * again" link is the one place that loss is not cosmetic: a 5xx reader who is offered their search + * back without their search terms has been handed a different request than the one that failed. It is + * Symfony's `getQueryString()`, which is normalised (pairs sorted, empty query answered as null, taken + * here as '') and percent-encoded to RFC 3986, so `<`, `>` and `"` are already `%3C`, `%3E` and `%22` + * before htmlspecialchars ever sees them and no spelling of it can end the attribute it sits in. The + * fact grid still shows `path` alone: the grid is a statement about the request, and a query string is + * where a session token or a search a person would rather not screenshot tends to live. + * + * `baseUrl` IS THE THIRD PIECE OF THE SAME ADDRESS, and it exists because `path` is base-URL-STRIPPED. + * Laravel's `path()` is Symfony's `getPathInfo()`, which answers `orders/42` for a request to + * /app/index.php/orders/42 — the front controller's own prefix is deliberately not in it, because a + * route is matched on the path info and nothing else. The "Try again" link is the one place that + * absence is not cosmetic: on a deployment served under a base path, a href built from `path` alone + * names a URL the deployment never serves, so the primary action on every 5xx page points off the + * application. It is Symfony's `getBaseUrl()` — the same value `LoginPageAction` and the OAuth2 link + * builders prepend for exactly this reason — and it is '' for the ordinary rewrite-to-the-root + * deployment, where the concatenation is `path` unchanged. The grid is untouched by it for the same + * reason it omits the query: it states which resource was asked for, and the front controller is no + * more part of that than a search term is. ErrorPage::retry() puts the CONCATENATION through + * ErrorPageSettings::url(), never the halves, because a guard that checked the tail and trusted the + * head would have been asking about a string the page does not print. + * * @param list $frames * @param list $previous + * @param list $allowed */ private function __construct( public int $status, @@ -49,8 +82,13 @@ private function __construct( public string $severity, public string $method, public string $path, + public string $query, public string $timestamp, public bool $detailed, + // The front controller's own prefix, '' when there is none — see the `path`/`query` note above. + // It carries a default so that a report assembled by hand (a renderer test, an Octane-shaped + // fixture) is not obliged to know about a deployment shape it is not exercising. + public string $baseUrl = '', public string $exceptionClass = '', public string $message = '', public string $location = '', @@ -58,17 +96,36 @@ private function __construct( public array $previous = [], public string $reference = '', public string $correlationId = '', + /** @var list */ + public array $allowed = [], + public string $publicDetail = '', + public int $frameCount = 0, + public int $appFrameCount = 0, ) {} public static function of(Throwable $e, Request $request, ErrorPageSettings $settings, string $basePath, int $status, string $reason, string $timestamp): self { - $payload = ErrorResponse::fromException(ProblemMapper::toFireflyException($e), instance: $request->path(), timestamp: $timestamp)->toArray(); + $payload = ErrorResponse::fromException(ProblemMapper::toFireflyException($e), instance: ProblemMapper::instanceFor($request), timestamp: $timestamp)->toArray(); // The reference is the id a person can act on: the W3C trace id when this request has one, the // correlation id otherwise. The correlation id is carried beside it, never replaced by it. $reference = TraceContext::referenceFor($request); $correlationId = CorrelationIdFilter::of($request); + // The verbs a 405 permits are already parsed, HEAD-filtered and published as an extension member; + // the page threw them away and shrugged instead. Read with an explicit is_array + foreach + + // is_string loop rather than array_filter, which cannot give PHPStan at level max a list. + $allowed = []; + if (is_array($payload['allowed'] ?? null)) { + foreach ($payload['allowed'] as $method) { + if (is_string($method)) { + $allowed[] = $method; + } + } + } + + $publicDetail = $settings->authoredDetail ? ProblemMapper::authoredDetail($e) : ''; + $public = new self( status: $status, reason: $reason, @@ -77,16 +134,33 @@ public static function of(Throwable $e, Request $request, ErrorPageSettings $set severity: is_string($payload['severity'] ?? null) ? $payload['severity'] : '', method: $request->getMethod(), path: '/'.ltrim($request->path(), '/'), + query: $request->getQueryString() ?? '', timestamp: $timestamp, detailed: false, + baseUrl: $request->getBaseUrl(), reference: $reference, correlationId: $correlationId, + allowed: $allowed, + publicDetail: $publicDetail, ); if (! $settings->trace) { return $public; } + $roots = SourcePaths::roots($e, $basePath); + + // Built once, counted, then budgeted — three statements rather than one expression, because the + // counts describe the UNTRIMMED stack and the page needs both numbers to say "8 of 104 frames · 10 + // in your code" without lying about either half. + $frames = self::frames($e, $roots, $settings->excerptLines); + $appFrames = 0; + foreach ($frames as $frame) { + if (! $frame->vendor) { + $appFrames++; + } + } + return new self( status: $public->status, reason: $public->reason, @@ -95,28 +169,83 @@ public static function of(Throwable $e, Request $request, ErrorPageSettings $set severity: $public->severity, method: $public->method, path: $public->path, + query: $public->query, timestamp: $public->timestamp, detailed: true, + baseUrl: $public->baseUrl, exceptionClass: $e::class, message: $e->getMessage(), - location: self::shorten($e->getFile(), $basePath).':'.$e->getLine(), - frames: self::frames($e, $basePath, $settings->excerptLines), - previous: self::previous($e, $basePath), + location: SourcePaths::shorten($e->getFile(), $roots).':'.$e->getLine(), + frames: self::budget($frames, $settings->maxFrames), + previous: self::previous($e, $roots), reference: $reference, correlationId: $correlationId, + allowed: $allowed, + publicDetail: $publicDetail, + frameCount: count($frames), + appFrameCount: $appFrames, ); } + /** + * The frames the page will actually build, in stack order. + * + * A HARD TRIM, NOT A STYLE. The alternative — render every frame and hide the tail with CSS — keeps a + * hundred frames in the DOM that a screen reader still walks and a find-in-page still matches, and + * costs the same hundred escapes on a page that renders while the application is already failing. + * + * YOUR FRAMES ARE NEVER WHAT GETS TRIMMED. Taking the first N would drop an application frame sixty + * deep — a controller called from a queue worker, a listener under the event dispatcher — which is + * precisely the frame a reader opened this page for. So the budget is spent on application frames + * first and filled with vendor frames in stack order, and the result is still in stack order because + * both passes walk the same list. + * + * @param list $frames + * @return list + */ + private static function budget(array $frames, int $max): array + { + if (count($frames) <= $max) { + return $frames; + } + + $keep = []; + + foreach ($frames as $i => $frame) { + if (! $frame->vendor && count($keep) < $max) { + $keep[$i] = true; + } + } + + foreach (array_keys($frames) as $i) { + if (count($keep) >= $max) { + break; + } + + $keep[$i] = true; + } + + $kept = []; + foreach ($frames as $i => $frame) { + if (isset($keep[$i])) { + $kept[] = $frame; + } + } + + return $kept; + } + /** * The throw site first, then the call stack — which is the order a reader wants and the opposite of the * order `getTrace()` returns it in relative to `getFile()`. PHP's trace starts at the CALLER of the * throwing frame, so the throwing line itself appears nowhere in it and has to be prepended. * + * @param list $roots * @return list */ - private static function frames(Throwable $e, string $basePath, int $excerptLines): array + private static function frames(Throwable $e, array $roots, int $excerptLines): array { - $frames = [self::frame($e->getFile(), $e->getLine(), 'throw', $basePath, $excerptLines)]; + $frames = [self::frame($e->getFile(), $e->getLine(), 'throw', $roots, $excerptLines, 0)]; foreach ($e->getTrace() as $entry) { $file = is_string($entry['file'] ?? null) ? $entry['file'] : ''; @@ -126,23 +255,27 @@ private static function frames(Throwable $e, string $basePath, int $excerptLines $type = $entry['type'] ?? ''; $function = $entry['function']; - $frames[] = self::frame($file, $line, $class.$type.$function.'()', $basePath, $excerptLines); + $frames[] = self::frame($file, $line, $class.$type.$function.'()', $roots, $excerptLines, count($frames)); } return $frames; } - private static function frame(string $file, ?int $line, string $call, string $basePath, int $excerptLines): ErrorFrame + /** + * @param list $roots + */ + private static function frame(string $file, ?int $line, string $call, array $roots, int $excerptLines, int $index): ErrorFrame { $vendor = $file === '' || str_contains($file, '/vendor/') || str_contains($file, '\\vendor\\'); return new ErrorFrame( file: $file, - shortFile: $file === '' ? '[internal function]' : self::shorten($file, $basePath), + shortFile: $file === '' ? '[internal function]' : SourcePaths::shorten($file, $roots), line: $line, call: $call, vendor: $vendor, excerpt: $vendor ? [] : self::excerpt($file, $line, $excerptLines), + index: $index, ); } @@ -189,9 +322,10 @@ private static function excerpt(string $file, ?int $line, int $radius): array * an InvalidRequestException, the container wraps a constructor throw, and the message on the outermost * exception is the least specific one in the chain. * + * @param list $roots * @return list */ - private static function previous(Throwable $e, string $basePath): array + private static function previous(Throwable $e, array $roots): array { $chain = []; $seen = 0; @@ -201,19 +335,10 @@ private static function previous(Throwable $e, string $basePath): array $chain[] = [ 'class' => $e::class, 'message' => $e->getMessage(), - 'location' => self::shorten($e->getFile(), $basePath).':'.$e->getLine(), + 'location' => SourcePaths::shorten($e->getFile(), $roots).':'.$e->getLine(), ]; } return $chain; } - - private static function shorten(string $file, string $basePath): string - { - if ($basePath !== '' && str_starts_with($file, $basePath)) { - return ltrim(substr($file, strlen($basePath)), '/\\'); - } - - return $file; - } } diff --git a/packages/web/src/Error/ProblemMapper.php b/packages/web/src/Error/ProblemMapper.php index 38fa1c32..689c3f48 100644 --- a/packages/web/src/Error/ProblemMapper.php +++ b/packages/web/src/Error/ProblemMapper.php @@ -9,6 +9,7 @@ use Firefly\Kernel\Error\ErrorResponse; use Firefly\Kernel\Error\ErrorSeverity; use Firefly\Kernel\Exception\FireflyException; +use Illuminate\Http\Request; use Illuminate\Http\Response; use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface; use Symfony\Component\HttpKernel\Exception\MethodNotAllowedHttpException; @@ -28,11 +29,13 @@ * limit is named next, because a reader can act on it: the request was not wrong, the server stopped it, * and a 503 with `Retry-After` says so where a 500 with the engine's sentence says "your fault, no idea * why". A Symfony/Illuminate HttpExceptionInterface keeps its REAL status; its message is the author's when - * `abort(404, '…')` supplied one, and the ROUTER's when the router raised it — and the router's sentences + * `abort(404, '…')` supplied one, and the FRAMEWORK's otherwise — and every framework-generated sentence * ("The route api/x could not be found.", "The GET method is not supported for route api/x. Supported - * methods: POST.") are replaced with ones written for a person, because "route" is the framework's word, - * the path is already in `instance`, and the allowed methods belong in an `allowed` extension member and - * the `Allow` header, not inside a sentence a client would have to parse. + * methods: POST.", and the route-model-binding 404s Laravel's handler rewrites into the router's own + * exception class) is replaced with one written for a person, because "route" is the framework's word, the + * path is already in `instance`, a model class and a primary key are nobody's business but the log's, and + * the allowed methods belong in an `allowed` extension member and the `Allow` header, not inside a sentence + * a client would have to parse. See httpMessage() for which shapes are generated and where each comes from. * * ANYTHING ELSE IS AN ACCIDENT, AND ITS MESSAGE IS NOT FOR THE CLIENT. A QueryException stringifies the * failing SQL *and its bindings*; a TypeError names an absolute path on the server; a PDOException names the @@ -95,10 +98,106 @@ public static function statusText(int $status): string } /** - * An HttpException's message is the author's when abort() supplied one and the router's when the router - * raised it. Laravel's router has phrased its 404 as "The route {uri} could not be found." for its whole - * life; that exact shape is the only one replaced, so `abort(404, 'No such tenant.')` still reaches the - * client verbatim. + * The sentence this failure may say to whoever asked, or '' when it has none. + * + * ONE RULE, ASKED TWICE. The HTML page and the problem document describe the same failure, and until + * this existed they described it differently: the document published "Order 42 does not exist." — the + * application's own sentence, which is the entire point of the taxonomy — while the page beside it said + * "That page does not exist." A person reading the page and an operator reading the log were looking at + * two different errors. + * + * The gate is the STATUS and the KIND of throwable, not the class hierarchy. Below 500 a + * FireflyException's message was written FOR the client, and an HttpExceptionInterface's is the author's + * when abort() supplied one — a judge caught that `abort(404, 'No such tenant.')` raises an + * HttpException and not a FireflyException, so testing for the taxonomy alone would withhold exactly the + * sentences an application took the trouble to write. At 500 and above nothing is authored: a + * QueryException's message is the failing SQL and its bindings, and toFireflyException() has already + * replaced it with the opaque one. The mapping is reused rather than restated so the sentences the + * FRAMEWORK generates — the router's 404 and 405, and the route-model-binding 404s Laravel rewrites + * into the router's own exception class — come out here exactly as they go onto the wire, withheld on + * both surfaces or published on both. + */ + public static function authoredDetail(Throwable $e): string + { + if (! $e instanceof FireflyException && ! $e instanceof HttpExceptionInterface) { + return ''; + } + + $mapped = self::toFireflyException($e, disclose: false); + + return $mapped->httpStatus() < 500 ? $mapped->getMessage() : ''; + } + + /** + * The request path as an RFC 9457 `instance`: a ROOT-RELATIVE reference, leading slash and all — with + * the four characters that would turn the slash after it into an AUTHORITY percent-encoded. + * + * `$request->path()` answers `orders/42`, and a relative reference is resolved against the document's + * base URI — which for a problem served from /api/orders/42 makes `api/orders/42` mean + * /api/api/orders/42. One character, and the member stops identifying the occurrence it exists to + * identify. Spring's ProblemDetail sets `instance` from the request URI for the same reason. + * + * THE PUBLISHED DOCUMENT IS FIXED, AND BOTH SURFACES NOW ARRIVE HERE. ProblemDetailsRenderer — the one + * surface that puts `instance` on the wire — passes this method's answer, so a client receives the + * root-relative form; ErrorReport, which hands the value to ErrorResponse and reads back the code, the + * category, the severity and the 405's verbs, is the other caller, and the page's own path is built + * beside it. That is the point of the rule living here rather than at either call site: two spellings of + * one reference would eventually disagree about one request, and the day the shape changes again it + * changes once. This paragraph used to record that the renderer had not been moved over yet, and the + * conformance pass that moved it retired the note, which is what that note promised. + * + * AND ADDING THAT LEADING SLASH IS EXACTLY WHAT MAKES THE SECOND CHARACTER DANGEROUS, which is why the + * encoding is part of the same rule rather than a caller's problem. `$request->path()` does NOT strip a + * backslash from a real request — Symfony refuses one only inside `Request::create()`, never in the + * `prepareRequestUri()` path a served request takes, which is the quirk + * ErrorPageSettings::url()'s docblock spends nine lines on — so a REQUEST_URI of `/\evil.test/phish` + * answers `\evil.test/phish` here. UNPREFIXED that is harmless: the URL parser's relative state treats + * the leading `\` as a single separator and resolves it against this origin. PREFIXED it is + * `/\evil.test/phish`, which enters special-authority-ignore-slashes state and resolves to + * `https://evil.test/phish` — `//host` wearing a path's clothes, the attack ErrorPageSettings::url() + * exists to refuse, in a member an API console, an IDE HTTP client and a documentation viewer all + * render as a link. ASCII tab, LF and CR are the same hazard by the other rule the parser applies + * before it reads anything: it DELETES all three, so `//evil.test` IS `//evil.test` by the time a + * browser looks at it, and a leading one that used to be stripped along with the rest of the input's + * leading whitespace is now safely behind a slash where it is not. + * + * PERCENT-ENCODING RATHER THAN REFUSING, because RFC 3986 does not admit any of the four in a path + * segment in the first place: `%5C`, `%09`, `%0A` and `%0D` are never a separator to any URL parser, so + * the reference still identifies the occurrence it was asked about — which a fallback to `/` would + * throw away on exactly the requests an operator most wants to see. The rule is applied HERE, at the + * one place the reference is built, so ErrorReport's copy of the member is guarded by the same line; + * the page's own `href` is a different value with a different vocabulary and is refused outright by + * ErrorPage::retry() through ErrorPageSettings::url(). + */ + public static function instanceFor(Request $request): string + { + return '/'.str_replace( + ['\\', "\t", "\n", "\r"], + ['%5C', '%09', '%0A', '%0D'], + ltrim($request->path(), '/'), + ); + } + + /** + * An HttpException's message is the AUTHOR's when abort() supplied one and the FRAMEWORK's when the + * framework raised it, and only the framework's is replaced — so `abort(404, 'No such tenant.')` still + * reaches the client verbatim. The replacement lives here, in the one mapping both renderers go + * through, so the problem document's `detail` and the page's lede are fixed by a single rule and cannot + * come to disagree about the same 404. + * + * THREE GENERATED SHAPES, AND TWO OF THEM DISCLOSE. Laravel's router has phrased its miss as "The route + * {uri} could not be found." for its whole life; "route" is the framework's word for something the + * caller never named, and the path is already in `instance`. The other two do not come from the router + * at all: Handler::prepareException() rewrites a ModelNotFoundException and a + * BackedEnumCaseNotFoundException into `new NotFoundHttpException($e->getMessage(), $e)` BEFORE any + * renderable callback is consulted, which puts "No query results for model [App\Models\Order] 42" and + * "Case [pending] not found on Backed Enum [App\Enums\Status]." — an application FQCN and a primary + * key — on the wire to whoever followed a stale link. A missed route-model binding is the most ORDINARY + * 404 an application has, so leaving these two out would make the disclosing case the common one. + * + * Laravel's remaining rewrites are deliberately left alone because they name nothing: a + * RecordsNotFoundException becomes a flat "Not found.", a RequestExceptionInterface a flat "Bad + * request.", and either is already a sentence a person can read. */ private static function httpMessage(HttpExceptionInterface $e): string { @@ -112,13 +211,68 @@ private static function httpMessage(HttpExceptionInterface $e): string return self::NOTHING_HERE; } + if ($e->getStatusCode() === 404 && ( + str_starts_with($message, 'No query results for model [') + || (str_starts_with($message, 'Case [') && str_contains($message, '] not found on Backed Enum [')) + )) { + return self::NOTHING_HERE; + } + return $message; } /** - * The verbs a 405 permits live in the exception's `Allow` header, which the router always sets. HEAD is - * dropped from the sentence and the extension because Symfony adds it beside every GET and no person - * chooses it; it stays in the header the renderer copies through, where the standard wants it. + * What a 405 says, for WHICHEVER surface is asking — the problem document's `detail` and the HTML + * page's lede are both built here. + * + * ONE BUILDER BECAUSE THE VERBS ARE ONE FACT. The list is parsed off the router's `Allow` header once, + * HEAD-filtered once (Symfony adds it beside every GET and no person chooses it; it stays in the header + * the renderer copies through, where the standard wants it) and joined into prose once. Two copies of + * that joining is how a page and a document come to disagree about a failure whose whole content is a + * list of three words, and this wave exists because they had already come to disagree about others. + * + * THE TWO SURFACES STILL SAY DIFFERENT SENTENCES, AND THAT IS WHAT `$refused` IS FOR. A problem + * document is built from the throwable alone — toFireflyException() takes a throwable and nothing + * else, and is called both from a renderer that has a request and from places that have none — so it + * cannot name the verb the caller actually used, and says only what the address accepts. The HTML page + * has the request in hand and names both, because a person who has just been refused is owed the + * refusal and not only the menu. So the difference between the two sentences is exactly the clause the + * document has no way to write; it is one ternary here rather than two wordings in two files, and + * ErrorPageTest pins the pair against each other so neither can be reworded alone. + * + * WITH NO VERBS THERE IS NO MENU, and both surfaces say so the same way: a router that set an empty + * `Allow` header has told the caller nothing to act on, and naming the verb that failed on its own + * would be a sentence with no next step in it. The page never reaches this arm — lede() takes the 405 + * branch only when the list is non-empty, and falls through to its own reassurance otherwise — so the + * arm exists for the document, which must answer something for every 405 it is handed. + * + * @param list $allowed the verbs the address accepts, HEAD already dropped + * @param string $refused the verb the caller used, when the caller is known; '' when it is not + */ + public static function methodSentence(array $allowed, string $refused = ''): string + { + if ($allowed === []) { + return 'This address does not accept that method.'; + } + + // Written with "does not" rather than a contraction because that is the page's voice ("That page + // does not exist.", "You do not have access to that.") and because an apostrophe here would reach + // the markup as `'`. + $verbs = count($allowed) === 1 + ? $allowed[0] + : implode(', ', array_slice($allowed, 0, -1)).' or '.$allowed[count($allowed) - 1]; + + return $refused === '' + ? "This address only accepts {$verbs}." + : "That address does not accept a {$refused} request. It accepts {$verbs}."; + } + + /** + * The verbs a 405 permits live in the exception's `Allow` header, which the router always sets. They + * are published as an `allowed` extension member as well as spelled into the sentence, so a client + * never has to parse prose to learn them — and so the HTML page beside this document can print the + * same list without re-reading the header. The sentence itself comes from methodSentence(), which the + * page calls too; see there for why the document's wording is the one that names no refused verb. */ private static function methodNotAllowed(MethodNotAllowedHttpException $e): FireflyException { @@ -128,14 +282,8 @@ private static function methodNotAllowed(MethodNotAllowedHttpException $e): Fire explode(',', is_string($header) ? $header : ''), ), static fn (string $method): bool => $method !== '' && $method !== 'HEAD')); - $sentence = match (count($allowed)) { - 0 => 'This address does not accept that method.', - 1 => "This address only accepts {$allowed[0]}.", - default => 'This address only accepts '.implode(', ', array_slice($allowed, 0, -1)).' or '.$allowed[count($allowed) - 1].'.', - }; - return new FireflyException( - $sentence, + self::methodSentence($allowed), 'METHOD_NOT_ALLOWED', 405, ErrorCategory::Framework, diff --git a/packages/web/src/Error/ProblemType.php b/packages/web/src/Error/ProblemType.php new file mode 100644 index 00000000..99c9435b --- /dev/null +++ b/packages/web/src/Error/ProblemType.php @@ -0,0 +1,59 @@ + + */ + public static function roots(Throwable $e, string $basePath): array + { + $roots = []; + + if ($basePath !== '') { + array_push($roots, ...self::spellings($basePath)); + + // realpath() ONCE, for the base — never per frame, because this runs on a page that is already + // rendering under duress and a stat per stack frame is a hundred syscalls for cosmetics. It + // answers false for a path that no longer exists, and a false is simply not a root. + $real = realpath($basePath); + if ($real !== false) { + array_push($roots, ...self::spellings($real)); + } + } + + foreach (self::files($e) as $file) { + foreach (self::VENDOR_MARKERS as $marker) { + // The OUTERMOST vendor segment — strpos here, strrpos in shorten()'s fallback, and the + // difference is not a taste. A dependency that ships its own nested `vendor/` is ordinary + // (this repository vendors one: symplify/monorepo-builder), and reading the LAST segment + // would derive `/vendor/symplify/monorepo-builder/` as a root. Because shorten() + // deliberately prefers the LONGEST matching root, that root then beats the real project + // root, and the package's own `src/Builder.php` prints as a bare `src/Builder.php`: a + // dimmed vendor row wearing an application path, naming a prefix no other row shares and + // no reader can put back. The markers carry a leading separator, so strpos cannot be fooled + // by a directory merely NAMED `my-vendor` — the lookalike that makes strrpos right for + // naming a dependency does not argue for it when deriving a project root. + $at = strpos($file, $marker); + if ($at !== false) { + $roots[] = substr($file, 0, $at + 1); + } + } + } + + $roots = array_values(array_unique($roots)); + + usort($roots, static fn (string $a, string $b): int => strlen($b) <=> strlen($a)); + + return $roots; + } + + /** + * A directory contributed as a root under BOTH separator spellings, because it does not reveal its own. + * + * A base path is a configured string, not a parsed path, and it arrives without the one fact this + * function would need in order to pick a separator. Guessing cost the class everything on Windows: + * `Application::basePath()` answers `C:\srv\app`, PHP reports trace files as + * `C:\srv\app\Http\Controllers\OrderController.php`, and appending a forward slash built the mixed root + * `C:\srv\app/` — a string that no path on that machine can start with. Both of the first two answers + * the class docblock promises, the literal prefix AND the realpath()-normalised one, were dead by + * construction there, and the three lines of `str_starts_with` this class replaced got the case right, + * so it is behaviour owed rather than a new promise. + * + * WHY IT LOOKED FINE. The third answer, the root derived from a `\vendor\` segment, is spelled by the + * frame itself and so survives — and it rescues exactly the rows nobody reads. Every Laravel trace + * passes through framework frames, so the dimmed dependency rows shortened while the APPLICATION rows, + * the ones that carry excerpts and are the whole reason the page exists, each printed a full absolute + * path and wrapped. Not a missing feature: the 10,108-pixel page, inverted and reproduced on the other + * platform, masked by the one answer that still worked. + * + * Spelling both rather than detecting one keeps the platform out of the question. shorten() skips any + * root that does not prefix the file, so the spelling that is wrong for this machine costs a failed + * `str_starts_with` and nothing else, and the two are the same length, so neither can displace the + * other in the longest-first order the rest of the class is built on. + * + * @return list + */ + private static function spellings(string $path): array + { + $trimmed = rtrim($path, '/\\'); + + return [$trimmed.'/', $trimmed.'\\']; + } + + /** + * The path as a person reads it: the LONGEST root that prefixes it, removed. + * + * Longest rather than first, and decided here rather than left to the order the list arrived in: in a + * monorepo a frame lives under both the repository and the package, and describing `src/X.php` as + * `packages/web/src/X.php` buries the part that varies under the part every row shares. roots() already + * sorts longest-first, but this is a public function that takes any list, and a rule that only holds for + * one caller's ordering is not a rule. + * + * @param list $roots + */ + public static function shorten(string $file, array $roots): string + { + if ($file === '') { + return ''; + } + + $best = ''; + + foreach ($roots as $root) { + // '/' is never a root: it matches every absolute path and would shorten each of them by exactly + // one character, which is the worst of both answers. + if ($root === '' || $root === '/' || $root === '\\' || strlen($root) <= strlen($best)) { + continue; + } + + if (str_starts_with($file, $root)) { + $best = $root; + } + } + + if ($best !== '') { + return substr($file, strlen($best)); + } + + foreach (self::VENDOR_MARKERS as $marker) { + $at = strrpos($file, $marker); + if ($at !== false) { + return substr($file, $at + 1); + } + } + + return $file; + } + + /** + * Every file named by the throwable, its trace and its `previous` chain. + * + * The chain is walked because that is where the real cause usually is — firefly/web wraps a binding + * failure, the container wraps a constructor throw — and its frames are printed on the same page. + * + * @return list + */ + private static function files(Throwable $e): array + { + $files = []; + $seen = 0; + $current = $e; + + while ($current instanceof Throwable && $seen <= self::CHAIN) { + $files[] = $current->getFile(); + + foreach ($current->getTrace() as $entry) { + if (is_string($entry['file'] ?? null)) { + $files[] = $entry['file']; + } + } + + $current = $current->getPrevious(); + $seen++; + } + + return $files; + } +} diff --git a/packages/web/src/Exception/ProblemDetailsRenderer.php b/packages/web/src/Exception/ProblemDetailsRenderer.php index bc5007f7..f9a8b63d 100644 --- a/packages/web/src/Exception/ProblemDetailsRenderer.php +++ b/packages/web/src/Exception/ProblemDetailsRenderer.php @@ -4,17 +4,23 @@ namespace Firefly\Web\Exception; +use BackedEnum; use DateTimeImmutable; use DateTimeInterface; +use Firefly\Kernel\Error\ErrorCategory; use Firefly\Kernel\Error\ErrorResponse; +use Firefly\Kernel\Error\ErrorSeverity; use Firefly\Web\Error\ErrorPageSettings; use Firefly\Web\Error\ProblemMapper; +use Firefly\Web\Error\ProblemType; use Firefly\Web\Filter\CorrelationIdFilter; use Firefly\Web\Trace\TraceContext; use Illuminate\Http\Request; use Illuminate\Http\Response; +use Psr\Log\LoggerInterface; use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface; use Throwable; +use UnitEnum; /** * Renders any throwable as an application/problem+json response via ErrorResponse::fromException (a thin map @@ -43,12 +49,41 @@ * thing that changes the value is switching tracing on. The correlation id is not absorbed: it keeps * `X-Correlation-Id` untouched and gains `correlationId`, and the trace id is echoed on its own header * (`firefly.web.trace-id.header`, `X-Trace-Id` by default, '' to disable) only when there is one. + * - A BODY, unconditionally. The encoder is total: an invalid UTF-8 byte is substituted rather than + * raised, and anything json_encode still refuses — or anything an encoded object's OWN jsonSerialize() + * or accessor throws, which JSON_THROW_ON_ERROR never sees and which catching JsonException therefore + * never caught — falls back to a minimal document. That document keeps every standard member, the + * reference among them, every field error's name and sentence, and every extension member's NAME, and + * drops only the values that can be the cause. This method used to throw JsonException out of the + * error handler on a latin-1 byte in a driver message, which turned a described failure into a blank + * 500 with no document at all. + * - AND THE DEGRADATION SAYS SO, in the two places a person looks. An error-handling subsystem that + * fails INVISIBLY is the failure this whole surface exists to remove, and for one release the fallback + * above was exactly that: it caught a Throwable an application's own accessor had raised, dropped the + * open namespace on the floor, and published a document a healthy one could not be told apart from — + * same status, same code, same category, no marker, and not one log line anywhere in the process. A + * misconfigured application lost its extension members from every problem document it published, over + * and over, with no way for an operator to learn that it was happening or why. So: the caught + * throwable is REPORTED through the optional logger below, with the document's own reference in the + * context so the log line and the body a caller is holding join up; and the document itself NAMES what + * it could not carry, because minimal() renders an unencodable member as its type instead of deleting + * it — Firefly\Actuator\Introspection\ConfigPropsEndpoint::value() has answered the same question that + * way since it shipped, and saying `"balance": "App\Models\Balance"` is more use to everyone than a + * member that silently was not there. */ final class ProblemDetailsRenderer { /** Seconds a caller is told to wait before retrying a 503. Short, for the reason in the class comment. */ public const int RETRY_AFTER_SECONDS = 5; + /** + * How deep encodable() walks an extension member's arrays before it stops describing and starts naming. + * ConfigPropsEndpoint's number, for ConfigPropsEndpoint's reason: a bound is what makes the walk total + * against a structure that refers to itself, and eight levels is deeper than any context an application + * hangs off a throw site. + */ + private const int MAX_DEPTH = 8; + /** * THE DISCLOSURE GATE IS THE PROBLEM DOCUMENT'S OWN. For one release this path shared the HTML page's * `trace` (which follows `app.debug`), and that was the wrong gate for a machine surface: every local @@ -57,13 +92,25 @@ final class ProblemDetailsRenderer * `ErrorPageSettings::$disclose` (`firefly.web.problem.disclose`) is read instead, defaults to false and * inherits from nothing. The settings object is optional so a JSON-only deployment that never bound one * still renders — and when it is absent the default is the SAFE one. + * + * THE LOGGER IS THE DEGRADED DOCUMENT'S WITNESS, and it has no configuration key of its own on purpose. + * It is not a feature an application turns on: it is the record of a failure INSIDE the error handler, + * and a key whose off position means "lose the only evidence" is a key nobody should be offered. It is + * optional for the same reason the settings object is — a JSON-only deployment, or one of the dozens of + * `new ProblemDetailsRenderer` in this repository's tests, binds no logger and must still render — and + * WebServiceProvider hands over the application's when there is one. Nothing else in this class reads + * it; see report(), which is also where a logger that throws is dealt with. */ - public function __construct(private readonly ?ErrorPageSettings $settings = null) {} + public function __construct( + private readonly ?ErrorPageSettings $settings = null, + private readonly ?LoggerInterface $logger = null, + ) {} public function render(Throwable $e, Request $request): Response { // An absent settings object means the SAFE answer, not the open one — see the constructor. $disclose = $this->settings instanceof ErrorPageSettings && $this->settings->disclose; + $typeUri = $this->settings instanceof ErrorPageSettings ? $this->settings->typeUri : ProblemType::BLANK; $correlationId = CorrelationIdFilter::of($request); $reference = TraceContext::referenceFor($request); @@ -75,13 +122,28 @@ public function render(Throwable $e, Request $request): Response // It is passed THROUGH ErrorResponse rather than written onto the array afterwards: the DTO's // member list is what the published OpenAPI component is generated and guarded from, so a member // appended here would be one no generated client decodes. - $payload = ErrorResponse::fromException( + $problem = ErrorResponse::fromException( $exception, - instance: $request->path(), + // RFC 9457 §3.1.5: `instance` is a URI REFERENCE, and a relative one resolves against the + // document's base URI — so the bare `api/orders/42` this used to pass, served from + // /api/orders/42, identified /api/api/orders/42. The rule is ProblemMapper's because the HTML + // page beside this one builds the same reference, and two spellings of it would eventually + // disagree about the same request. + instance: ProblemMapper::instanceFor($request), traceId: $reference, timestamp: (new DateTimeImmutable)->format(DateTimeInterface::ATOM), correlationId: $correlationId, - )->toArray(); + // RFC 9457 §3.1.1, through the same seam and for the same reason: the member is DECLARED on + // ErrorResponse and published by ProblemSchema, so it travels on the DTO rather than being + // written onto toArray()'s output, where every generated client would drop it. It arrives here + // as an argument rather than through a second `new ErrorResponse(...)` because that second + // construction site would have a default for every member — and would therefore drop, in + // silence, whatever member this class grows next. An absent settings object means the + // documented default, like the gate above it. + type: ProblemType::of($exception->errorCode(), $typeUri), + ); + + $payload = $problem->toArray(); $headers = [ 'Content-Type' => 'application/problem+json', @@ -105,9 +167,346 @@ public function render(Throwable $e, Request $request): Response } return new Response( - json_encode($payload, JSON_THROW_ON_ERROR | JSON_UNESCAPED_SLASHES), + $this->encode($payload, $reference), $exception->httpStatus(), $headers, ); } + + /** + * The payload as JSON, whatever the payload turns out to contain. + * + * THE RENDERER RUNS WHILE THE APPLICATION IS ALREADY FAILING, and json_encode had a live failure mode on + * exactly that path: JSON_THROW_ON_ERROR turns a single byte that is not valid UTF-8 — anywhere in the + * document — into a JsonException thrown OUT of this method, so the error handler fails while handling + * the error and the caller receives no document at all. Those bytes are not exotic: a driver message + * quoting a latin-1 column value, a request header echoed into an extension member at the throw site, a + * file name off a filesystem that is not UTF-8. + * + * JSON_INVALID_UTF8_SUBSTITUTE answers that case properly — the offending bytes become U+FFFD and the + * document is still the document. The try/catch is the belt to that pair of braces, and it catches + * THROWABLE RATHER THAN JsonException BECAUSE JSON_THROW_ON_ERROR DOES NOT COVER THE WHOLE FAILURE SET. + * Two different kinds of thing end up here: + * + * - What json_encode refuses AND reports — recursion, a resource, an INF or NAN an application put in + * an extension member. This is the arm JSON_THROW_ON_ERROR turns into a JsonException. + * - What json_encode never sees as an error at all. Encoding an object CALLS the application's own + * code — JsonSerializable::jsonSerialize(), an Eloquent accessor, a decrypting cast — and whatever + * that code raises propagates straight out of json_encode with no error state set and no + * JsonException anywhere in it. Catching JsonException left this arm open. + * + * The second arm is ordinary, not exotic, and it is worst exactly here: extension members are `mixed` + * and are chosen at the throw site by withExtensions(), FieldError::$rejectedValue is `mixed` too, and + * an object whose accessor reads the database is most likely to throw when the database is the thing + * that already failed — which is to say, while this renderer is describing that failure. Neither arm is + * a reason to answer a caller with nothing. + * + * AND NEITHER ARM IS A REASON TO SAY NOTHING EITHER, which is what the first spelling of this method + * did. `catch (Throwable) { $json = false; }` swallowed an arbitrary application exception whole: the + * RuntimeException an Eloquent accessor raised under preventLazyLoading went into that pair of braces + * and out of the process, and the degraded document that came back was byte-indistinguishable from a + * healthy one. Both halves of that are fixed below — the catch BINDS the throwable and report() hands + * it to the logger, and minimal() names every member it could not carry instead of deleting it — so + * the one arm of this subsystem that can still fail is the one arm nobody could previously observe. + * + * @param array $payload + * @param ?string $reference the document's own `traceId`, so the log line and the body a caller is + * holding can be joined up — the whole point of publishing one + */ + private function encode(array $payload, ?string $reference): string + { + try { + // JSON_THROW_ON_ERROR is what makes the catch the WHOLE fallback: with it set json_encode never + // answers false, it raises, so there is no second failure mode for this method to miss. + return json_encode($payload, JSON_UNESCAPED_SLASHES | JSON_INVALID_UTF8_SUBSTITUTE | JSON_THROW_ON_ERROR); + } catch (Throwable $cause) { + $this->report($cause, $reference); + + return self::minimal($payload); + } + } + + /** + * Says that a problem document was degraded, and why, to whoever is listening. + * + * IT IS WRAPPED IN ITS OWN try/catch, and that is not belt-and-braces theatre. This runs inside the + * error handler, with an application already failing, and the logger is a live collaborator: a handler + * writing to a full disk, a channel whose remote endpoint is the thing that went down, a Monolog + * processor reading the same broken context. A throw from HERE would escape render() and produce + * exactly the blank 500 the encoder was made total to prevent — the bug back again, one layer up and + * wearing the fix's own clothes. So a logger that fails costs the log line and nothing else. + * + * The message says what happened to the DOCUMENT, because that is what an operator is holding when + * they come looking; `exception` carries the cause under the key PSR-3 reserves for it, which is the + * key Laravel's formatter already knows to expand into a class, a message and a trace. + */ + private function report(Throwable $cause, ?string $reference): void + { + if (! $this->logger instanceof LoggerInterface) { + return; + } + + $context = ['exception' => $cause, 'reference' => $reference ?? '']; + + try { + $this->logger->error( + 'The problem document could not be encoded and was degraded: a member it was given cannot be JSON.', + $context, + ); + } catch (Throwable) { + // See the docblock: the renderer still owes the caller a document, and it is about to return one. + } + } + + /** + * A document that CANNOT fail to encode: the standard members ErrorResponse declares, each rebuilt from + * a value whose type is checked here rather than trusted, with every string passing through the same + * substitution. + * + * WHAT IS DROPPED IS WHAT COULD BE THE CAUSE, AND NOTHING ELSE — down to the VALUE, not the member that + * holds it and not the array that holds the member. Only a NON-string reaches this branch — + * JSON_INVALID_UTF8_SUBSTITUTE has already answered every bad byte — so what brought us here is an + * extension member an application chose at the throw site, a FieldError::$rejectedValue (`mixed`, so + * literally whatever the client sent), a structure too deep or too circular to walk, or an object whose + * own accessor threw mid-encode. The open namespace is therefore RENDERED, not deleted: encodable() + * replaces a value json_encode could refuse with the name of its type and keeps every value that was + * never in question, so `allowed` — which ProblemMapper itself publishes as an extension on a 405 — + * still lists the verbs, and a `balance` that cannot be carried says `App\Models\Balance` instead of + * vanishing. Deleting the namespace wholesale is what made a degraded document unreadable AND + * indistinguishable; ConfigPropsEndpoint::value() has rendered the unencodable as its type since it + * shipped, for the same stated reason, and one framework should answer one question one way. + * `errors` keeps its four declared-string members and drops the one `mixed` one — see fieldErrors(), + * which is where the difference between an operator's context and a sentence shown to a person is + * argued. The standard members STAY, because ErrorResponse declares every one of them `?string` + * (`int`, for the status) and array_diff_key() keeps a same-named extension out of the document, so + * not one of them can be why we are here and dropping them buys nothing: + * + * - `category` and `severity` pinned to Internal/Error for every exception publishes a document that + * contradicts its own status and code — a 409 whose category reads `internal`, a 422 a generated + * client branching on `category == 'validation'` renders down the wrong arm. They are re-derived + * through tryFrom() rather than copied so the published schema's enum constraint holds whatever the + * payload turns out to say. Keeping `category: validation` is only half the promise, and + * fieldErrors() is the other half: the arm the client renders has to have something in it. + * - `detail`, `traceId` and `correlationId` dropped publishes exactly the document whose body a person + * cannot correlate, against this class's promise that quoting the reference is possible from the body + * alone — and replaces an authored sub-500 sentence that ProblemMapper's disclosure gate had already + * cleared for publication with an opaque internal one that withholds nothing it had not already let + * through. + * + * The literal at the bottom is unreachable and is written anyway: a renderer on the error path does not + * get to assume. + * + * @param array $payload + */ + private static function minimal(array $payload): string + { + $status = is_int($payload['status'] ?? null) ? $payload['status'] : 500; + $category = ErrorCategory::tryFrom(self::member($payload, 'category', '')) ?? ErrorCategory::Internal; + $severity = ErrorSeverity::tryFrom(self::member($payload, 'severity', '')) ?? ErrorSeverity::Error; + + $document = [ + 'status' => $status, + 'title' => self::member($payload, 'title', ErrorResponse::titleFor($status)), + 'code' => self::member($payload, 'code', 'INTERNAL_ERROR'), + 'category' => $category->value, + 'severity' => $severity->value, + 'detail' => self::member($payload, 'detail', ProblemMapper::OPAQUE), + ]; + + // The rest are optional in the DOCUMENT as well as on the DTO — toArray() writes each one only when + // it is non-null — so an absent member stays absent here rather than becoming an invented empty + // string. The order is STANDARD_MEMBERS', so the degraded document reads like the full one. + foreach (['type', 'instance', 'traceId', 'correlationId', 'timestamp'] as $member) { + $value = $payload[$member] ?? null; + + if (is_string($value)) { + $document[$member] = $value; + } + } + + // LAST of the standard members, where STANDARD_MEMBERS has it, so the degraded document still reads + // like the full one. + $errors = self::fieldErrors($payload); + + if ($errors !== []) { + $document['errors'] = $errors; + } + + // AND THE OPEN NAMESPACE AFTER THEM, which is where ErrorResponse::toArray() puts it: the standard + // members lead every problem document this framework publishes and the extensions follow, so the + // degraded one diffs against the full one member for member rather than looking like a third shape. + foreach (self::extensions($payload) as $name => $value) { + $document[$name] = $value; + } + + $json = json_encode($document, JSON_UNESCAPED_SLASHES | JSON_INVALID_UTF8_SUBSTITUTE); + + return is_string($json) + ? $json + : '{"status":500,"title":"Internal Server Error","code":"INTERNAL_ERROR","category":"internal","severity":"error"}'; + } + + /** + * The RFC 9457 open namespace of the payload — every member that is not one ErrorResponse defines — + * with each value rendered as something json_encode cannot refuse. + * + * THE NAMES ARE THE POINT. An application puts context on an exception at the throw site precisely + * because that context is what makes the failure legible — the tenant, the order id, the upstream it + * called — and for one release this method did not exist and the whole namespace was deleted the moment + * ANY member of the document failed to encode. One unreadable `balance` took `tenant` and `orderId` + * with it, and took the framework's OWN extension with it too: ProblemMapper publishes a 405's verb + * list as `allowed`, a plain list of strings that can never be why an encode failed. Keeping the + * members and rendering the values is strictly more truthful than dropping them, and strictly less + * disclosure than the healthy document already published — an object that would have been serialised + * with every property it owns is named by its class and nothing else. + * + * @param array $payload + * @return array + */ + private static function extensions(array $payload): array + { + $members = []; + + foreach (array_diff_key($payload, array_flip(ErrorResponse::STANDARD_MEMBERS)) as $name => $value) { + $members[$name] = self::encodable($value); + } + + return $members; + } + + /** + * One value of the open namespace, rendered as something json_encode cannot refuse and — the half that + * matters here — cannot have to ASK THE APPLICATION about. + * + * TOTALITY IS THE WHOLE REQUIREMENT, and it is why this walks types rather than trying values. Scalars + * and null are already JSON. Arrays are descended, to a bound, because a circular one is reachable + * through a reference and an unbounded walk would exchange a failed encode for a failed stack. A + * non-finite float is NAMED rather than typed: get_debug_type() would say `float`, which is the single + * least interesting true thing about an INF, and an INF in an extension member is not hypothetical — + * json_decode('{"ratio": 1e999}') is exactly that, so any caller can post one. Enums answer from their + * own case, which is a property read and not a method call. EVERYTHING ELSE — an object, a resource, a + * closure — becomes get_debug_type(), and get_debug_type() is the only answer available: asking an + * object for its value is calling the application's code, and calling the application's code is what + * threw on the way in here. That is ConfigPropsEndpoint::value()'s reasoning and very nearly its + * shape, minus the one branch it has that this cannot have — it descends into an object's properties, + * and it may, because the objects it walks are config DTOs the framework built itself. + */ + private static function encodable(mixed $value, int $depth = 0): mixed + { + if ($value === null || is_bool($value) || is_int($value) || is_string($value)) { + return $value; + } + + if (is_float($value)) { + if (is_finite($value)) { + return $value; + } + + return is_nan($value) ? 'NAN' : ($value > 0 ? 'INF' : '-INF'); + } + + if (is_array($value)) { + if ($depth > self::MAX_DEPTH) { + return 'array'; + } + + $mapped = []; + + foreach ($value as $key => $item) { + $mapped[$key] = self::encodable($item, $depth + 1); + } + + return $mapped; + } + + if ($value instanceof BackedEnum) { + return $value->value; + } + + if ($value instanceof UnitEnum) { + return $value->name; + } + + return get_debug_type($value); + } + + /** + * The field errors of the payload, rebuilt from the members FieldError DECLARES to be strings. + * + * DROPPING `errors` WHOLESALE DROPPED FAR MORE THAN THE CAUSE. FieldError declares `$field` and + * `$message` as `string` and `$code`/`$constraint` as `?string`, so not one of those four can ever be + * what json_encode refused; `$rejectedValue` is the only `mixed` member, and it is the CLIENT'S own + * value — the validators fill it with `Arr::get($data, $field)` straight off the decoded request body, + * which is how a posted `{"ratio": 1e999}` arrives here as INF. So one caller posting one number used + * to get back a 422 that said `category: validation` and carried no `errors` member at all, taking + * every OTHER field's sentence down with it — a document that invites a generated client down the + * field-error arm and then hands that arm nothing. The rejected value goes. The field, the sentence, + * the code and the constraint stay. + * + * AND THE REJECTED VALUE GOES RATHER THAN BEING NAMED, which is the opposite of what happens to an + * extension member one line above, on purpose. `rejectedValue` is an ECHO: its whole contract is "this + * is the value you sent", and a client renders it back to the person who sent it. Writing `"float"` + * there would not be a degraded truth, it would be a false sentence shown to a human — nobody posted + * the word float. An extension member has no such contract: it is context an application chose for an + * OPERATOR, and naming its type is the most useful thing that can honestly be said about a value + * nobody can read. An absent `rejectedValue` is already a shape every client handles, because + * FieldError declares it nullable and omits it when there is none. + * + * Every member is type-checked rather than trusted, for minimal()'s reason: we are on this path + * precisely because the payload was not the shape it claims to be. An entry that is not an array, or + * one that keeps no member at all, contributes nothing instead of contributing an empty object — and + * checking the type is also what keeps this total, since reading only strings out of the payload + * cannot re-enter the application code that may have thrown on the way in. + * + * @param array $payload + * @return list> + */ + private static function fieldErrors(array $payload): array + { + $entries = $payload['errors'] ?? null; + + if (! is_array($entries)) { + return []; + } + + $errors = []; + + foreach ($entries as $entry) { + if (! is_array($entry)) { + continue; + } + + $kept = []; + + // FieldError::toArray()'s order, minus the one member that could be why we are here. + foreach (['field', 'message', 'code', 'constraint'] as $member) { + $value = $entry[$member] ?? null; + + if (is_string($value)) { + $kept[$member] = $value; + } + } + + if ($kept !== []) { + $errors[] = $kept; + } + } + + return $errors; + } + + /** + * One standard member of the payload, when it is the string ErrorResponse declares it to be. + * + * The type check is not ceremony: minimal() runs because something in this payload was not what the + * document's shape says it is, and a member read on trust here would take the fallback down with it. + * + * @param array $payload + */ + private static function member(array $payload, string $name, string $fallback): string + { + $value = $payload[$name] ?? null; + + return is_string($value) ? $value : $fallback; + } } diff --git a/packages/web/src/WebServiceProvider.php b/packages/web/src/WebServiceProvider.php index 98a26ec3..52564a71 100644 --- a/packages/web/src/WebServiceProvider.php +++ b/packages/web/src/WebServiceProvider.php @@ -8,7 +8,6 @@ use Firefly\Context\Boot\BootPass; use Firefly\Context\Boot\FireflyServiceProvider; use Firefly\Context\Scan\AppScan; -use Firefly\Kernel\Exception\FireflyException; use Firefly\Validation\Constraint\BeanValidator; use Firefly\Validation\Constraint\ConstraintManifest; use Firefly\Validation\Constraint\ConstraintManifestCompiler; @@ -35,6 +34,7 @@ use Illuminate\Contracts\Foundation\Application; use Illuminate\Contracts\View\Factory as ViewFactory; use Illuminate\Http\Request; +use Psr\Log\LoggerInterface; use Throwable; /** @@ -64,9 +64,26 @@ public function passes(): array private function registerBindings(): void { if (! $this->app->bound(ProblemDetailsRenderer::class)) { - $this->app->singleton(ProblemDetailsRenderer::class, static fn (Container $app): ProblemDetailsRenderer => new ProblemDetailsRenderer( - $app->make(ErrorPageSettings::class), - )); + $this->app->singleton(ProblemDetailsRenderer::class, static function (Container $app): ProblemDetailsRenderer { + // The logger is what makes a DEGRADED problem document observable: when the encoder falls + // back, the throwable it caught is an arbitrary application exception, and without this + // argument it is recorded in no place at all. Optional and resolved defensively for the + // ResponseFactory's reason two closures down — Laravel aliases Psr\Log\LoggerInterface to + // the concrete 'log' key in registerCoreContainerAliases() whether or not LogServiceProvider + // ever registered anything, so bound() on the CONTRACT answers true in a bare container and + // the make() then throws. A logger that cannot be made is the same as none bound, and this + // renderer must not be the binding that fails to construct on an error path. + $logger = null; + if ($app->bound('log')) { + try { + $logger = $app->make(LoggerInterface::class); + } catch (BindingResolutionException) { + // No log manager: the document is still rendered, and the degradation is unwitnessed. + } + } + + return new ProblemDetailsRenderer($app->make(ErrorPageSettings::class), $logger); + }); } if (! $this->app->bound(ErrorPageSettings::class)) { @@ -206,10 +223,22 @@ private function registerBindings(): void * * The order is the whole of it. A browser that names `text/html` gets the HTML page — which is what * fixes a person clicking a stale link and being shown a raw JSON blob, the behaviour every - * FireflyException had. Everything else keeps the previous rule exactly: a FireflyException, or a - * request that wants JSON, renders as problem+json. A throwable that is NEITHER — an unrouted URL hit by - * a client that asked for neither — still falls through to Laravel's handler, because inventing a - * response shape for a caller that expressed no preference is not this package's decision to make. + * FireflyException had. + * + * Everything else keeps the previous rule and gains the case it was missing: a FireflyException, a + * request that wants JSON and a `json-paths` URL all render as problem+json, and so now does every + * caller that is neither a browser nor a JSON client — a wildcard Accept header from a bare curl, no + * Accept at all, or a named type this package renders no error in, such as `application/xml` — each of + * which used to fall through to Laravel's stock HTML page. The predicate itself lives in + * ErrorPageRenderer::rendersProblem(), beside handles() and prefersHtml(), because it is the same + * negotiation asked a third way; this provider keeps only the wiring. + * `firefly.web.error-page.problem-fallback => false` restores the fall-through. + * + * BOTH OF THOSE READ THE REQUEST, SO WHAT WAS THROWN IS ASKED FIRST. describes() is the one term that + * looks at the throwable, and it is asked ahead of both branches because the three exceptions Laravel's + * own `match (true)` resolves AFTER this callback runs — HttpResponseException, AuthenticationException, + * ValidationException — are not ours to answer in either shape. Returning null for them is what lets a + * 422 stay a 422 with its field errors, and a 401 stay a 401. */ private function registerProblemDetailsRenderable(): void { @@ -221,11 +250,15 @@ private function registerProblemDetailsRenderable(): void $handler->renderable(function (Throwable $e, Request $request) { $page = $this->app->make(ErrorPageRenderer::class); + if (! $page->describes($e)) { + return null; + } + if ($page->handles($request)) { return $page->render($e, $request); } - if ($e instanceof FireflyException || $request->expectsJson() || $page->forcesJson($request)) { + if ($page->rendersProblem($e, $request)) { return $this->app->make(ProblemDetailsRenderer::class)->render($e, $request); } diff --git a/packages/web/tests/CapstoneErrorPagesProductionTest.php b/packages/web/tests/CapstoneErrorPagesProductionTest.php new file mode 100644 index 00000000..9e58840e --- /dev/null +++ b/packages/web/tests/CapstoneErrorPagesProductionTest.php @@ -0,0 +1,86 @@ +get('/err/missing', ['Accept' => 'text/html'])->baseResponse->getContent(); + + expect($html)->toContain('Order 42 does not exist.') + ->toContain('ORDER_NOT_FOUND') + ->toContain('
    Reference
    ') + ->toContain('Go home') + // Nothing internal, and not the page's own advice about how to turn the trace on. + ->not->toContain('ResourceNotFoundException') + ->not->toContain('Stack trace') + ->not->toContain('APP_DEBUG') + ->not->toContain('OrderService.php'); +}); + +it('withholds a generic failure\'s cause while still handing over an id to quote', function () { + /** @var ProductionErrorPagesCapstoneTestCase $this */ + $response = $this->get('/err/wrecked', ['Accept' => 'text/html']); + $html = (string) $response->baseResponse->getContent(); + + $response->assertStatus(500); + + expect($html)->toContain('Something went wrong on our side.') + ->toContain('quote the reference below if you report it') + ->toContain('Try again') + ->not->toContain('The fixture failed on purpose.') + ->not->toContain('the inner cause') + ->not->toContain('LogicException') + ->not->toContain('Caused by'); +}); + +it('keeps the document opaque for a client too, with the same status and code', function () { + /** @var ProductionErrorPagesCapstoneTestCase $this */ + $this->getJson('/err/wrecked') + ->assertStatus(500) + ->assertJsonPath('code', 'INTERNAL_ERROR') + ->assertJsonPath('type', 'https://api.example.test/problems/internal-error') + ->assertJsonMissing(['detail' => 'The fixture failed on purpose.']); +}); + +it('names the verbs on both surfaces for a 405 the router raised', function () { + /** @var ProductionErrorPagesCapstoneTestCase $this */ + // Only the router produces a real Allow header, which is why this is a capstone and not a unit test. + $document = $this->getJson('/err/submit'); + $document->assertStatus(405) + ->assertJsonPath('code', 'METHOD_NOT_ALLOWED') + ->assertJsonPath('allowed', ['POST']) + ->assertJsonPath('detail', 'This address only accepts POST.') + ->assertHeader('Allow'); + + $page = $this->get('/err/submit', ['Accept' => 'text/html']); + $page->assertStatus(405); + + expect((string) $page->baseResponse->getContent()) + ->toContain('That address does not accept a GET request. It accepts POST.'); +}); + +it('keeps an author\'s abort() sentence and replaces the router\'s, on the page as on the wire', function () { + /** @var ProductionErrorPagesCapstoneTestCase $this */ + $authored = (string) $this->get('/err/aborted', ['Accept' => 'text/html'])->baseResponse->getContent(); + $router = (string) $this->get('/err/no-such-route', ['Accept' => 'text/html'])->baseResponse->getContent(); + + expect($authored)->toContain('No such tenant.') + ->and($router)->toContain('There is nothing at this address.') + ->not->toContain('could not be found'); +}); + +it('offers sign-in on a 401 and nothing of the sort on a 403', function () { + /** @var ProductionErrorPagesCapstoneTestCase $this */ + $refused = (string) $this->get('/err/refused', ['Accept' => 'text/html'])->baseResponse->getContent(); + $denied = (string) $this->get('/err/denied', ['Accept' => 'text/html'])->baseResponse->getContent(); + + expect($refused)->toContain('Sign in') + ->toContain('Authentication is required to access this resource.') + ->and($denied)->toContain('Access is denied.') + ->toContain('Go home') + ->not->toContain('Sign in'); +}); diff --git a/packages/web/tests/CapstoneErrorPagesTest.php b/packages/web/tests/CapstoneErrorPagesTest.php new file mode 100644 index 00000000..f343e6b3 --- /dev/null +++ b/packages/web/tests/CapstoneErrorPagesTest.php @@ -0,0 +1,66 @@ +get('/err/missing', ['Accept' => 'text/html,application/xhtml+xml']); + $page->assertStatus(404)->assertHeader('Content-Type', 'text/html; charset=UTF-8'); + + $document = $this->getJson('/err/missing'); + $document->assertStatus(404) + ->assertHeader('Content-Type', 'application/problem+json') + ->assertJsonPath('code', 'ORDER_NOT_FOUND') + ->assertJsonPath('detail', 'Order 42 does not exist.') + ->assertJsonPath('instance', '/err/missing') + ->assertJsonPath('type', 'https://api.example.test/problems/order-not-found'); + + // The same words on both surfaces, which is the whole point of sharing ProblemMapper. + expect((string) $page->baseResponse->getContent())->toContain('Order 42 does not exist.') + ->toContain('ORDER_NOT_FOUND'); +}); + +it('answers a bare curl with problem+json instead of Laravel\'s stock page', function () { + /** @var ErrorPagesCapstoneTestCase $this */ + // THE BLOCKER: `Accept: */*` is what curl and fetch() send by default, and a route-level throwable that + // is not a FireflyException used to fall all the way through to Laravel's HTML handler. + $this->get('/err/nothing-here', ['Accept' => '*/*']) + ->assertStatus(404) + ->assertHeader('Content-Type', 'application/problem+json') + ->assertJsonPath('code', 'RESOURCE_NOT_FOUND') + ->assertJsonPath('instance', '/err/nothing-here'); + + // Symfony supplies a browser Accept by default; null removes it from the real request. + $this->call('GET', '/err/nothing-here', server: ['HTTP_ACCEPT' => null]) + ->assertStatus(404) + ->assertHeader('Content-Type', 'application/problem+json'); +}); + +it('bounds the debug page it renders for a hundred-frame failure', function () { + /** @var ErrorPagesCapstoneTestCase $this */ + $html = (string) $this->get('/err/wrecked', ['Accept' => 'text/html'])->baseResponse->getContent(); + + $rows = substr_count($html, '
  • ') + substr_count($html, '
  • '); + + expect($rows)->toBeLessThanOrEqual(25) + ->and($rows)->toBeGreaterThan(0) + // Shortened, split, and on one line: no row carries the absolute path that made this page 10,108 + // pixels tall, and no frame's file name is inside a directory span. + ->and($html)->not->toContain('/') + ->toContain('
    ') + ->toContain('The fixture failed on purpose.') + ->toContain('the inner cause'); +}); + +it('renders its own page for a failure with no reference of its own, and never throws doing it', function () { + /** @var ErrorPagesCapstoneTestCase $this */ + $response = $this->get('/err/wrecked', ['Accept' => 'text/html']); + + $response->assertStatus(500)->assertHeader('X-Correlation-Id'); + + expect((string) $response->baseResponse->getContent())->toContain('
    Reference
    '); +}); diff --git a/packages/web/tests/CapstoneWebIntegrationTest.php b/packages/web/tests/CapstoneWebIntegrationTest.php index 94df64bc..9c560b0f 100644 --- a/packages/web/tests/CapstoneWebIntegrationTest.php +++ b/packages/web/tests/CapstoneWebIntegrationTest.php @@ -2,7 +2,11 @@ declare(strict_types=1); +use Firefly\Web\Error\ProblemMapper; +use Firefly\Web\Tests\Fixtures\Filters\CarriedResponseFilter; use Firefly\Web\Tests\Support\WebCapstoneTestCase; +use Illuminate\Support\Facades\Log; +use Psr\Log\AbstractLogger; use Symfony\Component\HttpFoundation\Response; uses(WebCapstoneTestCase::class); @@ -78,18 +82,41 @@ ->assertHeader('X-Filter-Trail', 'BA'); }); -it('renders a generic Throwable as problem+json ONLY when the request expects JSON (expectsJson gate)', function () { +it('renders a generic Throwable as a page ONLY for a caller that NAMED text/html, and as problem+json otherwise', function () { /** @var WebCapstoneTestCase $this */ - // GET /boom/generic throws a plain \RuntimeException (NOT a FireflyException). The RFC-7807 renderable is - // gated on `$e instanceof FireflyException || $request->expectsJson()`, so a generic error becomes - // problem+json for a JSON client and otherwise falls through to Laravel's default handler. + // GET /boom/generic throws a plain \RuntimeException (NOT a FireflyException). The renderable asks + // ErrorPageRenderer::handles() first — the page is for a caller that NAMED text/html — and + // rendersProblem() second, which claims a JSON client, a json-paths URL and, through + // `firefly.web.error-page.problem-fallback`, a caller that named nothing acceptable at all. $this->getJson('/boom/generic') ->assertStatus(500) ->assertHeader('Content-Type', 'application/problem+json'); - // Same route, non-JSON Accept: the generic Throwable must NOT be rendered as problem+json. Dropping the - // expectsJson() gate (rendering generic Throwables unconditionally) would make this branch wrongly return - // problem+json and fail the assertion below. + // A WILDCARD is not an opinion. `Accept: */*` is what a bare curl and a default fetch() send, and + // `acceptsHtml()` answers true for it — so keying the page off that would hand every unadorned + // command-line request a page of markup. It gets the document instead. + $this->call('GET', '/boom/generic', server: ['HTTP_ACCEPT' => '*/*']) + ->assertStatus(500) + ->assertHeader('Content-Type', 'application/problem+json'); + + // And so does a caller that sent no Accept at all. '' is as close as this harness gets: Symfony's + // Request::create() — which every call() here goes through — REPLACES an absent HTTP_ACCEPT with a + // browser's, and the predicate reads `headers->get('Accept', '')`, so an empty header and an absent + // one are the same string by the time it is asked. ErrorPageTest covers the genuinely absent one. + $this->call('GET', '/boom/generic', server: ['HTTP_ACCEPT' => '']) + ->assertStatus(500) + ->assertHeader('Content-Type', 'application/problem+json'); + + // And so does a caller that named something concrete this application renders no error in. `Accept: + // application/xml` is not a browser and is not a JSON client, and the key claims it deliberately: a + // FireflyException has always answered this same caller with a problem document, so claiming only the + // wildcard would give one client two unrelated-looking shapes for two 404s. + $this->call('GET', '/boom/generic', server: ['HTTP_ACCEPT' => 'application/xml']) + ->assertStatus(500) + ->assertHeader('Content-Type', 'application/problem+json'); + + // Same route, a caller that NAMED text/html: this one, and only this one, gets the page. Dropping the + // handles() branch (rendering every generic Throwable as a document) would fail the assertion below. $html = $this->get('/boom/generic', ['Accept' => 'text/html']); /** @var Response $base */ @@ -97,6 +124,57 @@ expect((string) $base->headers->get('Content-Type'))->not->toContain('application/problem+json'); }); +/* + * AND THREE THROWABLES ARE NOT THIS PACKAGE'S TO ANSWER IN EITHER SHAPE. Handler::render() consults + * renderViaCallbacks() — where the renderable above is registered — BEFORE the `match (true)` that resolves + * HttpResponseException, AuthenticationException and ValidationException. None of the three is a + * FireflyException or an HttpExceptionInterface, so ProblemMapper drops each to its default arm and answers + * 500 / INTERNAL_ERROR / "An unexpected error occurred." — a described failure replaced by an opaque one, + * which is the shape this wave exists to remove. Only the real pipeline can falsify it: a unit test of the + * predicate cannot see which exceptions Laravel's own handler would have resolved one arm later. + */ +it('leaves a Laravel ValidationException to Laravel, so a 422 keeps its status and its field errors', function () { + /** @var WebCapstoneTestCase $this */ + // POST /boom/validated runs Laravel's own validator, not LaraFly's #[Valid] — an ordinary thing for an + // application built on this framework to do. + $this->postJson('/boom/validated', ['email' => 'nope']) + ->assertStatus(422) + ->assertJsonPath('errors.email.0', 'The email field must be a valid email address.'); + + // The wildcard caller is the one the fallback introduced and the one it broke: claiming it turned + // Laravel's redirect-back-with-errors into a 500 problem+json with no `errors` member in it. + $wildcard = $this->call('POST', '/boom/validated', ['email' => 'nope'], server: ['HTTP_ACCEPT' => '*/*']); + + $wildcard->assertStatus(302)->assertSessionHasErrors(['email']); + expect((string) $wildcard->headers->get('Content-Type'))->not->toContain('application/problem+json'); +}); + +it('leaves a Laravel AuthenticationException to Laravel, so a 401 stays a 401 and a person still reaches the login page', function () { + /** @var WebCapstoneTestCase $this */ + $this->getJson('/boom/unauthenticated') + ->assertStatus(401) + ->assertJsonPath('message', 'Unauthenticated.'); + + // Handler::unauthenticated() sends anyone who did not ask for JSON to the login page. Claiming the + // exception published a 500 to both, which is a sign-in prompt turned into an internal error. + $this->call('GET', '/boom/unauthenticated', server: ['HTTP_ACCEPT' => '*/*']) + ->assertStatus(302) + ->assertRedirect('/boom/login'); +}); + +it('leaves an HttpResponseException to Laravel, so the response the application already built survives', function () { + /** @var WebCapstoneTestCase $this */ + // The most literal form of the failure: this exception CARRIES the response to return, and Laravel's + // handler returns it verbatim one `match` arm after the renderable. Describing it instead threw that + // response away and answered 500. Thrown from the FILTER CHAIN rather than from a controller, because + // Illuminate\Routing\Route::run() catches it inside the route and it would never reach the handler at + // all — see CarriedResponseFilter. + $response = $this->call('GET', '/'.CarriedResponseFilter::PATH, server: ['HTTP_ACCEPT' => '*/*']); + + $response->assertStatus(418); + expect((string) $response->getContent())->toBe(CarriedResponseFilter::BODY); +}); + it('answers a malformed #[PathVariable(pattern:)] segment with the entity\'s own 404 through the real pipeline', function () { /** @var WebCapstoneTestCase $this */ // RoomsController::show declares PathVariable::UUID with ROOM_NOT_FOUND; the resolver refuses the @@ -147,3 +225,142 @@ ->and($detail)->toContain($traceId) ->and($detail)->not->toContain('kaboom'); }); + +it('withholds the 404 sentence LARAVEL generates, after its own handler has rewritten the exception', function () { + /** @var WebCapstoneTestCase $this */ + // The whole point of driving this through the real pipeline: Handler::prepareException() turns + // BoomController::enumCase()'s BackedEnumCaseNotFoundException into a plain NotFoundHttpException + // carrying "Case [pending] not found on Backed Enum [App\Enums\Status]." BEFORE renderViaCallbacks() + // reaches LaraFly's renderable — a ModelNotFoundException, the ordinary route-model-binding miss, takes + // the identical path. A unit test that constructs the mapper's input by hand cannot prove that ordering; + // this one fails the moment Laravel moves the rewrite or changes the wording. + $response = $this->getJson('/boom/enum-case'); + + $response->assertStatus(404) + ->assertHeader('Content-Type', 'application/problem+json') + ->assertJsonPath('code', 'RESOURCE_NOT_FOUND') + ->assertJsonPath('detail', ProblemMapper::NOTHING_HERE); + + // The class name is the disclosure, so the assertion is against the RAW body and not the decoded member: + // a sentence smuggled into `title` or an extension would satisfy the path assertion above. + expect((string) $response->getContent())->not->toContain('Enums') + ->and((string) $response->getContent())->not->toContain('Backed Enum'); +}); + +/* + * A BYTE THAT IS NOT UTF-8 USED TO COST THE WHOLE DOCUMENT. The renderer encoded with JSON_THROW_ON_ERROR, + * so one latin-1 byte anywhere in the payload raised a JsonException OUT of the error handler and the + * caller received a blank 500 from the web server with nothing in it. Both cases below are driven through + * the real kernel for that reason: the blank 500 is not something the renderer returns, it is what the + * layer above it does with the exception the renderer threw, and a test that calls render() directly can + * only ever observe the throw. + */ +it('answers a controller that threw with a non-UTF-8 byte with a problem document, not a blank 500', function () { + /** @var WebCapstoneTestCase $this */ + $response = $this->getJson('/errors/latin-1'); + + $response->assertStatus(409) + ->assertHeader('Content-Type', 'application/problem+json') + ->assertJsonPath('code', 'LEDGER_CONFLICT') + ->assertJsonPath('category', 'business'); + + // Decodable, and still the sentence it was built from: the byte was substituted, not the document lost. + /** @var array $payload */ + $payload = json_decode((string) $response->getContent(), true, 512, JSON_THROW_ON_ERROR); + expect($payload['detail'])->toContain('The ledger for') + ->and($payload['detail'])->toContain('disagrees.') + ->and($payload['traceId'])->toBe($response->headers->get('X-Correlation-Id')); +}); + +it('answers a member json_encode refuses with the minimal document, still describing the failure it was built for', function () { + /** @var WebCapstoneTestCase $this */ + // INF in an extension member is what substitution cannot answer, so this is the fallback on the wire. + $response = $this->getJson('/errors/unencodable'); + + $response->assertStatus(409) + ->assertHeader('Content-Type', 'application/problem+json') + ->assertJsonPath('status', 409) + ->assertJsonPath('title', 'Conflict') + ->assertJsonPath('code', 'LEDGER_CONFLICT') + // Degraded, not contradictory: a 409 whose category read `internal` would have a client branching + // on the status and a client branching on the category disagreeing about the same document. + ->assertJsonPath('category', 'business') + ->assertJsonPath('severity', 'warning') + ->assertJsonPath('detail', 'The ledger disagrees.') + // The member stays and says what it holds. It used to be deleted, which published a degraded + // document that a healthy one could not be told apart from. + ->assertJsonPath('ratio', 'INF'); + + /** @var array $payload */ + $payload = json_decode((string) $response->getContent(), true, 512, JSON_THROW_ON_ERROR); + expect($payload['traceId'])->toBe($response->headers->get('X-Correlation-Id')) + ->and($payload['correlationId'])->toBe($response->headers->get('X-Correlation-Id')); +}); + +it('answers an extension member whose own accessor throws with the minimal document, not a blank 500', function () { + /** @var WebCapstoneTestCase $this */ + // The failure json_encode does not REPORT, it merely propagates: encoding an object calls the + // application's code, so a jsonSerialize() or an Eloquent accessor that throws comes out of + // json_encode with no JsonException anywhere in it. `catch (JsonException)` let it escape render(), + // and the caller got the same blank 500 this whole task exists to remove. + $response = $this->getJson('/errors/throwing-extension'); + + $response->assertStatus(409) + ->assertHeader('Content-Type', 'application/problem+json') + ->assertJsonPath('status', 409) + ->assertJsonPath('code', 'LEDGER_CONFLICT') + ->assertJsonPath('category', 'business') + ->assertJsonPath('detail', 'The ledger disagrees.') + // Named rather than deleted, which is get_debug_type()'s answer and ConfigPropsEndpoint's idiom. + ->assertJsonPath('balance', 'JsonSerializable@anonymous'); + + // The thrower's own sentence is an internal detail and must not ride out on the document either. + expect((string) $response->getContent())->not->toContain('accessor could not read'); +}); + +/* + * AND THE SWALLOWED THROWABLE REACHES THE LOG, THROUGH THE WIRING AND NOT THROUGH A CONSTRUCTOR ARGUMENT. + * The renderer takes an optional logger, and a unit test can hand it one; what a unit test cannot prove is + * that the application's own logger ever arrives — WebServiceProvider builds this singleton itself, and for + * one release it passed only the settings object, so the reporting arm would have been dead in every real + * boot while every unit test around it stayed green. The failure below is the one the reviewer reproduced: + * a RuntimeException raised by an extension member's accessor, recorded in no place at all. + */ +it('reports the degraded problem document through the application logger the provider wired', function () { + /** @var WebCapstoneTestCase $this */ + $log = new class extends AbstractLogger + { + /** @var list}> */ + public array $lines = []; + + /** + * @param array $context + */ + public function log(mixed $level, string|Stringable $message, array $context = []): void + { + $this->lines[] = ['message' => (string) $message, 'context' => $context]; + } + }; + + // Swapped BEFORE the request, which is when the renderer singleton is first resolved: the provider's + // closure reads the container's logger at construction time, so this is the application's logger as + // far as it is concerned. + Log::swap($log); + + $response = $this->getJson('/errors/throwing-extension'); + $response->assertStatus(409); + + $degraded = array_values(array_filter( + $log->lines, + static fn (array $line): bool => str_contains($line['message'], 'degraded'), + )); + + $cause = $degraded[0]['context']['exception'] ?? null; + + expect($degraded)->toHaveCount(1) + // The cause, which nothing anywhere recorded before. + ->and($cause)->toBeInstanceOf(Throwable::class) + ->and($cause instanceof Throwable ? $cause->getMessage() : '')->toContain('the accessor could not read the balance') + // And the reference the caller is holding, so the two ends of the failure join up. + ->and($degraded[0]['context']['reference'])->toBe($response->headers->get('X-Correlation-Id')); +}); diff --git a/packages/web/tests/Error/ErrorFrameTest.php b/packages/web/tests/Error/ErrorFrameTest.php new file mode 100644 index 00000000..f453b073 --- /dev/null +++ b/packages/web/tests/Error/ErrorFrameTest.php @@ -0,0 +1,113 @@ +run()', + vendor: true, + ); + + expect($frame->dir())->toBe('vendor/laravel/framework/src/Illuminate/Routing/') + ->and($frame->base())->toBe('Route.php') + ->and($frame->package())->toBe('laravel/framework') + ->and($frame->index)->toBe(0); +}); + +it('has no directory and no package for a bare name or an internal function', function () { + $internal = new ErrorFrame(file: '', shortFile: '[internal function]', line: null, call: 'array_map()', vendor: true); + + expect($internal->dir())->toBe('') + ->and($internal->base())->toBe('[internal function]') + ->and($internal->package())->toBeNull(); +}); + +it('reads the package from the LAST vendor segment, and only from a real one', function () { + $nested = new ErrorFrame( + file: '/srv/vendor/acme/tool/vendor/psr/log/src/LoggerInterface.php', + shortFile: 'vendor/acme/tool/vendor/psr/log/src/LoggerInterface.php', + line: 12, + call: 'Psr\Log\LoggerInterface->error()', + vendor: true, + ); + + // `my-vendor/` is a directory whose name merely ends in the word; it is not a Composer vendor dir, and + // reading `x/y` out of it would label an application frame with a package that does not exist. + $lookalike = new ErrorFrame( + file: '/srv/my-vendor/x/y/Thing.php', + shortFile: 'my-vendor/x/y/Thing.php', + line: 3, + call: 'y()', + vendor: false, + ); + + expect($nested->package())->toBe('psr/log') + ->and($lookalike->package())->toBeNull(); +}); + +it('carries its position in the untrimmed stack, so a trimmed list still reads as a stack', function () { + $frame = new ErrorFrame(file: '/a/b.php', shortFile: 'b.php', line: 1, call: 'b()', vendor: false, index: 37); + + expect($frame->index)->toBe(37); +}); + +it('treats a Windows separator as a separator', function () { + $frame = new ErrorFrame( + file: 'C:\\srv\\vendor\\laravel\\framework\\src\\Route.php', + shortFile: 'vendor\\laravel\\framework\\src\\Route.php', + line: 9, + call: 'run()', + vendor: true, + ); + + expect($frame->base())->toBe('Route.php') + ->and($frame->dir())->toBe('vendor\\laravel\\framework\\src\\'); +}); + +it('splits a call into a qualifier that may be clipped and a function name that never is', function () { + $frame = static fn (string $call): ErrorFrame => new ErrorFrame( + file: '/srv/app/x.php', shortFile: 'x.php', line: 1, call: $call, vendor: false, + ); + + expect($frame('Illuminate\Database\Eloquent\Builder->get()')->callQualifier())->toBe('Illuminate\Database\Eloquent\Builder') + ->and($frame('Illuminate\Database\Eloquent\Builder->get()')->callFunction())->toBe('->get()') + ->and($frame('Illuminate\Routing\Route::make()')->callQualifier())->toBe('Illuminate\Routing\Route') + ->and($frame('Illuminate\Routing\Route::make()')->callFunction())->toBe('::make()') + // A namespaced free function has no class, and its namespace is still the discardable half. + ->and($frame('Illuminate\Support\collect()')->callQualifier())->toBe('Illuminate\Support') + ->and($frame('Illuminate\Support\collect()')->callFunction())->toBe('\collect()'); +}); + +it('leaves a call with no qualifier whole, and reads a closure descriptor as part of the function', function () { + $frame = static fn (string $call): ErrorFrame => new ErrorFrame( + file: '/srv/app/x.php', shortFile: 'x.php', line: 1, call: $call, vendor: false, + ); + + // The synthetic first frame, an internal function, and a bare closure: nothing to clip, so the whole + // token is the function and the qualifier slot is not printed at all. + expect($frame('throw')->callQualifier())->toBe('') + ->and($frame('throw')->callFunction())->toBe('throw') + ->and($frame('array_map()')->callQualifier())->toBe('') + ->and($frame('array_map()')->callFunction())->toBe('array_map()') + ->and($frame('{closure}')->callQualifier())->toBe('') + ->and($frame('{closure}')->callFunction())->toBe('{closure}') + // PHP 8.4 names a closure after the file it was written in, and on Windows that path carries + // separators of its own. The cut is looked for before the first brace so the descriptor survives. + ->and($frame('App\Jobs\Sync::{closure:C:\app\Jobs\Sync.php:31}()')->callQualifier())->toBe('App\Jobs\Sync') + ->and($frame('App\Jobs\Sync::{closure:C:\app\Jobs\Sync.php:31}()')->callFunction())->toBe('::{closure:C:\app\Jobs\Sync.php:31}()'); +}); diff --git a/packages/web/tests/Error/ErrorPageSecurityTest.php b/packages/web/tests/Error/ErrorPageSecurityTest.php new file mode 100644 index 00000000..fd9404e9 --- /dev/null +++ b/packages/web/tests/Error/ErrorPageSecurityTest.php @@ -0,0 +1,206 @@ + ErrorPageSettings::fromConfig( + new Config(new Repository(['firefly' => ['web' => ['error-page' => $errorPage]]])), +); + +it('refuses a javascript: URL from configuration, in every slot that reaches an href', function () use ($settings) { + $hostile = $settings([ + 'home' => 'javascript:alert(document.cookie)', + 'sign-in' => 'JavaScript:alert(1)', + 'support' => "java\tscript:alert(1)", + ]); + + expect($hostile->home)->toBe('') + ->and($hostile->signIn)->toBe('') + ->and($hostile->support)->toBe(''); +}); + +it('refuses every other scheme that is not an ordinary web link', function () use ($settings) { + foreach (['data:text/html;base64,PHN2Zy9vbmxvYWQ9YWxlcnQoMSk+', 'vbscript:msgbox(1)', 'file:///etc/passwd', 'mailto:ops@example.test', '//evil.test/phish', '\\\\evil.test\\share'] as $value) { + expect($settings(['support' => $value])->support)->toBe(''); + } +}); + +/** + * The two ways a value that begins with ONE slash still leaves the origin. + * + * A guard that refuses `//host/…` and stops there is refusing a spelling rather than a behaviour, and both + * of the values it lets through land on a page a confused person is already standing on — which is what + * makes the redirect credible. For a special scheme the URL standard's relative-slash state treats `\` + * exactly like `/`, so a browser resolves `/\evil.test/phish` against this origin as + * `https://evil.test/phish`; and before any parsing happens every ASCII tab, LF and CR is DELETED from the + * input, so `//evil.test` is `//evil.test` by the time the resolver sees it. Each of these begins with + * one slash and not two, and each is the phishing redirect the protocol-relative case is refused for. + */ +it('refuses every value a browser resolves off this origin, whichever separator the authority hides behind', function () use ($settings) { + foreach (['/\\evil.test/phish', '/\\/evil.test', '/\\\\evil.test', "/\t/evil.test/phish", "/\n/evil.test/phish", "/\r/evil.test/phish", "//\tevil.test", 'https:/\\evil.test'] as $value) { + expect($settings(['support' => $value])->support)->toBe(''); + } +}); + +it('keeps an absolute path and an http(s) URL, which is the whole legitimate vocabulary', function () use ($settings) { + $good = $settings([ + 'home' => '/', + 'sign-in' => '/login?next=%2Forders', + 'support' => 'https://support.example.test/tickets/new', + ]); + + expect($good->home)->toBe('/') + ->and($good->signIn)->toBe('/login?next=%2Forders') + ->and($good->support)->toBe('https://support.example.test/tickets/new') + ->and($settings(['support' => 'HTTP://legacy.example.test/help'])->support)->toBe('HTTP://legacy.example.test/help'); +}); + +/** + * Padding at the EDGES is the deployment; padding INSIDE is the attack. + * + * These values arrive from the mechanism the guard's own docblock names — a Helm value, a CI-rendered .env, + * a tenant-provisioning job — and a Helm block scalar and a here-doc-rendered variable both end in a + * newline. Dotenv trims a `.env` LINE; Laravel's `Env` does not touch a real environment variable, so + * "https://support.example.test/tickets/new\n" is exactly what the settings class is handed, and refusing + * it deleted an operator's link with no exception and no log over a character no reader ever sees. The URL + * standard strips leading and trailing C0 controls and space BEFORE it parses, so trimming them decides the + * value exactly as the browser will. An INTERIOR tab, LF or CR is the opposite case: the parser deletes + * those from the middle, which is the whole reason `javascript:` is a javascript: URL, so they are + * still refused. Both halves are asserted together because the guard's first shape refused both, and the + * two can never be conflated again — trimming first gives up nothing, since every hostile value trims into + * another one the guard already refuses. + */ +it('trims the whitespace a deployment adds at the edges, and still refuses it in the middle', function () use ($settings) { + expect($settings(['support' => "https://support.example.test/tickets/new\n"])->support)->toBe('https://support.example.test/tickets/new') + ->and($settings(['home' => " /dashboard\r\n"])->home)->toBe('/dashboard') + ->and($settings(['support' => 'https://support.example.test '])->support)->toBe('https://support.example.test'); + + foreach (["/log\tin", "https://support.example.test/a\nb", "java\tscript:alert(1)", ' //evil.test', "\x00/\\evil.test", "\tjavascript:alert(1)", " \n "] as $value) { + expect($settings(['support' => $value])->support)->toBe(''); + } +}); + +/** + * The fifth configuration-supplied URI, and the first one that is not an `href`. + * + * `firefly.web.problem.type-uri` is the base ProblemType builds RFC 9457's `type` out of, and the RFC's own + * word for that member is "dereferenceable" — it exists so a person can open it. Nothing on the error PAGE + * prints it, which is exactly why it went unguarded while the four beside it did not: the audit that + * produced ErrorPageSettings::url() followed hrefs, and this value leaves by a different door. Every API + * console, IDE HTTP client and documentation viewer that renders a problem document turns `type` into a + * link, so `javascript:` here is the same stored XSS in front of a wider audience. + * + * AND THE SENTINELS ARE COMPARED WITH `===`, which makes the trim load-bearing rather than cosmetic. + * ProblemType::of() asks whether the base IS 'about:blank' and whether it IS ''; Config::string() does not + * trim and Laravel's Env does not touch a real environment variable, so the Helm block scalar and the + * here-doc-rendered `.env` this class's other guard was written for made BOTH answers false and quietly + * moved the deployment from the default into base-URI mode — publishing `" about:blank/resource-not-found"` + * as a URI a console invites a reader to click. + */ +$problemType = static fn (string $value): string => ErrorPageSettings::fromConfig( + new Config(new Repository(['firefly' => ['web' => ['problem' => ['type-uri' => $value]]]])), +)->typeUri; + +it('keeps the two sentinels and an absolute http(s) base, which is the whole legitimate vocabulary', function () use ($problemType) { + expect($problemType('about:blank'))->toBe('about:blank') + ->and($problemType(''))->toBe('') + ->and($problemType('https://api.example.test/problems'))->toBe('https://api.example.test/problems') + ->and($problemType('HTTP://legacy.example.test/p'))->toBe('HTTP://legacy.example.test/p') + // The default is the sentinel, for a settings object read from an empty configuration and for one + // built by hand — the guard runs in the constructor, like the four above it. + ->and(ErrorPageSettings::fromConfig(new Config(new Repository([])))->typeUri)->toBe('about:blank') + ->and((new ErrorPageSettings)->typeUri)->toBe('about:blank'); +}); + +it('trims the padding a deployment adds at the edges, so the RFC sentinel still reads as the sentinel', function () use ($problemType) { + // Each of these used to flip the key out of default mode: ProblemType::of() compares with ===, so a + // padded sentinel matched neither branch and became a base URI with a space or a newline inside it. + expect($problemType(' about:blank'))->toBe('about:blank') + ->and($problemType("about:blank\n"))->toBe('about:blank') + ->and($problemType("\tabout:blank\r\n"))->toBe('about:blank') + ->and($problemType("https://api.example.test/problems\n"))->toBe('https://api.example.test/problems') + ->and($problemType(' https://api.example.test/problems '))->toBe('https://api.example.test/problems') + // Whitespace and nothing else is the value an operator left blank, and '' is what it means: the + // member is omitted, which is the pre-9457 document byte for byte. + ->and($problemType(" \n "))->toBe(''); +}); + +it('refuses a hostile or relative base and falls back to the documented default rather than to silence', function () use ($problemType) { + foreach ([ + 'javascript:alert(document.cookie)', + 'JavaScript:alert(1)', + "java\tscript:alert(1)", + 'data:text/html;base64,PHN2Zy9vbmxvYWQ9YWxlcnQoMSk+', + 'vbscript:msgbox(1)', + 'file:///etc/passwd', + '//evil.test/problems', + '/\\evil.test/problems', + '/problems', + 'problems', + 'about:blank/', + "https://api.example.test/pro\nblems", + "https://api.example.test/pro\tblems", + ] as $value) { + // 'about:blank' and not '': '' is a position an operator takes deliberately, and a typo must not be + // able to take it for them. The answer to a hostile base is to remove the hostility, not the member. + expect($problemType($value))->toBe('about:blank', sprintf('%s reached the published `type`', var_export($value, true))); + } + + expect((new ErrorPageSettings(typeUri: 'javascript:alert(document.cookie)'))->typeUri)->toBe('about:blank') + ->and((new ErrorPageSettings(typeUri: ' about:blank'))->typeUri)->toBe('about:blank'); +}); + +it('defaults home to the site root, offers no sign-in or support link, and renders the action row', function () use ($settings) { + $defaults = $settings([]); + + expect($defaults->home)->toBe('/') + ->and($defaults->signIn)->toBe('') + ->and($defaults->support)->toBe('') + ->and($defaults->actions)->toBeTrue(); +}); + +it('lets an operator switch the action row off entirely', function () use ($settings) { + expect($settings(['actions' => false])->actions)->toBeFalse(); +}); + +/** + * The guard holds for a settings object nobody read out of configuration. + * + * `firefly/security` builds an ErrorPageSettings by hand for the login page, and this package's own render + * tests construct one directly. A guard that ran only in fromConfig() would have covered exactly one call + * site while the class advertised an invariant over all of them; the constructor is the seam every path + * goes through, so this is the assertion that the advertisement is honest. + */ +it('guards a settings object built by hand, not only one read from configuration', function () { + $hostile = new ErrorPageSettings( + home: 'javascript:alert(document.cookie)', + signIn: '/\\evil.test/login', + support: "java\tscript:alert(1)", + ); + + expect($hostile->home)->toBe('') + ->and($hostile->signIn)->toBe('') + ->and($hostile->support)->toBe(''); + + $good = new ErrorPageSettings(home: '/dashboard', support: 'https://support.example.test'); + + expect($good->home)->toBe('/dashboard') + ->and($good->support)->toBe('https://support.example.test') + ->and((new ErrorPageSettings)->home)->toBe('/'); +}); diff --git a/packages/web/tests/Error/ErrorPageTest.php b/packages/web/tests/Error/ErrorPageTest.php index dcda0eda..05935edf 100644 --- a/packages/web/tests/Error/ErrorPageTest.php +++ b/packages/web/tests/Error/ErrorPageTest.php @@ -2,7 +2,10 @@ declare(strict_types=1); +use Firefly\Config\Config; use Firefly\Kernel\Exception\Business\ResourceNotFoundException; +use Firefly\Kernel\Exception\Security\AuthenticationException; +use Firefly\Web\Error\ErrorFrame; use Firefly\Web\Error\ErrorPage; use Firefly\Web\Error\ErrorPageRenderer; use Firefly\Web\Error\ErrorPageSettings; @@ -10,7 +13,19 @@ use Firefly\Web\Error\ProblemMapper; use Firefly\Web\Exception\ProblemDetailsRenderer; use Firefly\Web\Trace\TraceContext; +use Illuminate\Auth\AuthenticationException as LaravelAuthenticationException; +use Illuminate\Config\Repository; +use Illuminate\Http\Exceptions\HttpResponseException; use Illuminate\Http\Request; +use Illuminate\Routing\Exceptions\BackedEnumCaseNotFoundException; +use Illuminate\Translation\ArrayLoader; +use Illuminate\Translation\Translator; +use Illuminate\Validation\ValidationException; +use Illuminate\Validation\Validator; +use Symfony\Component\HttpFoundation\Request as SymfonyRequest; +use Symfony\Component\HttpFoundation\Response as SymfonyResponse; +use Symfony\Component\HttpKernel\Exception\HttpException; +use Symfony\Component\HttpKernel\Exception\MethodNotAllowedHttpException; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; /** @@ -102,7 +117,13 @@ $settings = new ErrorPageSettings(trace: false, hints: false); $html = ErrorPage::render($report(new ResourceNotFoundException('Order 42 does not exist.', 'ORDER_NOT_FOUND'), $settings), $settings); - expect($html)->not->toContain('Order 42 does not exist.') + // The application's OWN sentence is published, and always was — by problem+json, for the same failure, + // written for the caller. The page withholding it was the inconsistency: a person reading the page and + // a client reading the document were told two different things about one error. What stays withheld is + // everything that is not a sentence somebody wrote: the class, the file, the trace, the page's own + // advice about turning the trace on. `firefly.web.error-page.authored-detail => false` restores the + // status-and-code-only page for a deployment that wants it. + expect($html)->toContain('Order 42 does not exist.') ->not->toContain('ResourceNotFoundException') ->not->toContain('Stack trace') // Nor the page's own advice about how to turn the trace on, which names the framework and a config @@ -281,7 +302,10 @@ expect($error->reference)->toBe('ref-1234-abcd') ->and($html)->toContain('ref-1234-abcd') - ->toContain('quote reference ref-1234-abcd if you report it') + // The prose points at the Reference cell instead of repeating the id into it: the id was printed + // TWICE on every production page, and neither copy could be copied. + ->toContain('quote the reference below if you report it') + ->toContain('
    Reference
    ref-1234-abcd
    ') // The reference is the ONLY thing the production 500 adds; the cause stays withheld. ->not->toContain('boom') ->not->toContain('RuntimeException'); @@ -300,7 +324,7 @@ expect($html)->toContain('
    Reference
    ref-5678-efgh
    ') // With the trace on the message is shown, so the reassurance sentence is not — the fact row is // where the reference lives on this variant. - ->not->toContain('quote reference') + ->not->toContain('quote the reference below') ->toContain('boom'); }); @@ -317,8 +341,9 @@ // The fact-row MARKUP, so what is asserted is the table and not a word the page happens to contain. ->and($html)->toContain('
    Reference
    4bf92f3577b34da6a3ce929d0e0e4736
    ') ->toContain('
    Correlation
    corr-42
    ') - // The reference a person is asked to quote is the one a trace search can find. - ->toContain('quote reference 4bf92f3577b34da6a3ce929d0e0e4736 if you report it'); + // The reference a person is asked to quote is the one a trace search can find, and it is named + // once, in the cell that can be selected in a single click. + ->toContain('quote the reference below if you report it'); }); it('shows no Correlation row when the reference already IS the correlation id', function () { @@ -332,8 +357,9 @@ ->and($error->correlationId)->toBe('corr-42') ->and($html)->toContain('
    Reference
    corr-42
    ') ->not->toContain('
    Correlation
    ') - // One id on the page, once: a second row holding the same value teaches a reader they are the same - // thing, which is exactly what the two members exist to keep apart. + // One id on the page, once: the cell, and the copy button's data-ref that reads it. It used to be + // the cell and a second copy spelled into prose, which teaches a reader to retype rather than + // select — and printed the same value in two places with no affordance on either. ->and(substr_count($html, 'corr-42'))->toBe(2); }); @@ -348,3 +374,1075 @@ expect($html)->toContain('
    Reference
    aaaaaaaabbbbbbbbccccccccdddddddd
    ') ->toContain('
    Correlation
    corr-77
    '); }); + +it('shortens every frame against the roots the trace reveals, not only against the base path', function () { + // THE 10,108-PIXEL BUG, at the level that produced it. With a base path that matches nothing — which is + // what a symlinked release, a bind mount and this very harness all look like — the old shorten() left + // every one of 104 rows holding an absolute path, and each of them wrapped onto three lines. + $settings = new ErrorPageSettings(trace: true); + $error = ErrorReport::of( + new RuntimeException('boom'), + Request::create('/x'), + $settings, + '/nowhere-at-all', + 500, + 'Internal Server Error', + '2026-01-01T00:00:00+00:00', + ); + + // The throw site is this file, under the repository the vendor frames reveal. + expect($error->frames[0]->shortFile)->toBe('packages/web/tests/Error/ErrorPageTest.php') + ->and($error->frames[0]->index)->toBe(0); + + $vendor = array_values(array_filter($error->frames, static fn ($f): bool => $f->vendor && $f->file !== '')); + expect($vendor)->not->toBeEmpty(); + + foreach ($vendor as $frame) { + expect($frame->shortFile)->toStartWith('vendor/') + ->and($frame->base())->not->toContain('/'); + + // Dead until shorten() was fixed: package() parses vendor/{a}/{b} out of $shortFile, and every + // $shortFile used to start with /Users/, so it answered null for all 104 frames. `vendor/bin/` is + // Composer's shim directory and not a package directory — the test runner's own `vendor/bin/pest` + // is a frame of this very trace — and answering `bin/pest` for it would name a package that does + // not exist. + if (! str_starts_with($frame->shortFile, 'vendor/bin/')) { + expect($frame->package())->not->toBeNull(); + } + } + + $packages = array_values(array_unique(array_filter(array_map(static fn ($f): ?string => $f->package(), $vendor)))); + + expect($packages)->not->toBeEmpty(); + + foreach ($packages as $package) { + expect($package)->toMatch('#^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$#'); + } +}); + +it('carries the verbs a 405 accepts, which the problem document already had and the page threw away', function () { + $settings = new ErrorPageSettings(trace: false, hints: false); + $e = new MethodNotAllowedHttpException(['POST', 'HEAD'], 'The GET method is not supported for route orders. Supported methods: POST, HEAD.'); + $error = ErrorReport::of($e, Request::create('/orders', 'GET'), $settings, dirname(__DIR__, 4), 405, 'Method Not Allowed', '2026-01-01T00:00:00+00:00'); + + // HEAD is dropped where the sentence is built, not here: Symfony adds it beside every GET and no person + // chooses it. It stays on the Allow header, where the standard wants it. + expect($error->allowed)->toBe(['POST']) + ->and($error->method)->toBe('GET'); +}); + +it('carries the authored sentence for a sub-500 failure, so the page and the document say the same words', function () { + $settings = new ErrorPageSettings(trace: false, hints: false); + $business = new ResourceNotFoundException('Order 42 does not exist.', 'ORDER_NOT_FOUND'); + $error = ErrorReport::of($business, Request::create('/orders/42'), $settings, dirname(__DIR__, 4), 404, 'Not Found', '2026-01-01T00:00:00+00:00'); + + // An abort(404, '…') raises an HttpException, NOT a FireflyException — the taxonomy is not the test of + // whether a sentence was authored, the status and the kind of throwable are. + $aborted = ErrorReport::of(new NotFoundHttpException('No such tenant.'), Request::create('/t/9'), $settings, dirname(__DIR__, 4), 404, 'Not Found', '2026-01-01T00:00:00+00:00'); + + // And a generic throwable's message is an accident — a table name, a bound value, a path on the server. + $accident = ErrorReport::of(new RuntimeException('SQLSTATE[42S02]: no such table'), Request::create('/x'), $settings, dirname(__DIR__, 4), 500, 'Internal Server Error', '2026-01-01T00:00:00+00:00'); + + expect($error->publicDetail)->toBe('Order 42 does not exist.') + ->and($aborted->publicDetail)->toBe('No such tenant.') + ->and($accident->publicDetail)->toBe('') + // The router's own sentence is replaced by the product's, exactly as it is in problem+json. + ->and(ErrorReport::of(new NotFoundHttpException('The route nope could not be found.'), Request::create('/nope'), $settings, dirname(__DIR__, 4), 404, 'Not Found', '2026-01-01T00:00:00+00:00')->publicDetail) + ->toBe(ProblemMapper::NOTHING_HERE); +}); + +it('never publishes the 404 sentences LARAVEL generates, which name a model class and a primary key', function () { + // Handler::prepareException() rewrites a ModelNotFoundException and a BackedEnumCaseNotFoundException + // into `new NotFoundHttpException($e->getMessage(), $e)` before any renderable callback runs, so what + // arrives here is an ordinary 404 carrying the FRAMEWORK's sentence and indistinguishable by class from + // an author's abort(404, '…'). Only its SHAPE tells them apart. The model sentence is spelled as a + // literal because firefly/web does not depend on illuminate/database — on this path the string IS the + // interface — while the enum one is taken from the real class, which illuminate/routing supplies. + $settings = new ErrorPageSettings(trace: false, hints: false); + $detail = static fn (string $message): string => ErrorReport::of( + new NotFoundHttpException($message), + Request::create('/orders/42'), + $settings, + dirname(__DIR__, 4), + 404, + 'Not Found', + '2026-01-01T00:00:00+00:00', + )->publicDetail; + + expect($detail('No query results for model [App\Models\Order] 42'))->toBe(ProblemMapper::NOTHING_HERE) + // With no ids Laravel ends the sentence with a period instead; both spellings name the class. + ->and($detail('No query results for model [App\Models\Order].'))->toBe(ProblemMapper::NOTHING_HERE) + ->and($detail((new BackedEnumCaseNotFoundException('App\Enums\Status', 'pending'))->getMessage())) + ->toBe(ProblemMapper::NOTHING_HERE) + // And an author's own 404 still stands: this is a test of three generated shapes, not a gag on 404s. + ->and($detail('No such tenant.'))->toBe('No such tenant.'); +}); + +it('trims the stack to the configured budget BEFORE markup, and never trims your own frames away', function () { + // The budget is an array_slice in the report, not a CSS trick in the page: a page that renders a hundred + // frames and hides ninety of them has still built, escaped and shipped a hundred frames, and the DOM a + // screen reader walks is still a hundred long. + $settings = new ErrorPageSettings(trace: true, maxFrames: 6); + $error = ErrorReport::of(new RuntimeException('boom'), Request::create('/x'), $settings, dirname(__DIR__, 4), 500, 'Internal Server Error', '2026-01-01T00:00:00+00:00'); + + $appKept = count(array_filter($error->frames, static fn ($f): bool => ! $f->vendor)); + $appTotal = $error->appFrameCount; + + expect($error->frames)->toHaveCount(6) + ->and($error->frameCount)->toBeGreaterThan(6) + // The counts describe the UNTRIMMED stack, so the page can say "6 of 104" honestly. + ->and($appTotal)->toBeGreaterThan(0) + ->and($appKept)->toBe(min($appTotal, 6)) + // Order is the stack's, still: the throw site is first whatever the budget dropped. + ->and($error->frames[0]->call)->toBe('throw') + ->and($error->frames[0]->index)->toBe(0); +}); + +it('clamps an absurd budget rather than trusting it', function () { + $config = static fn (int $max): ErrorPageSettings => ErrorPageSettings::fromConfig( + new Config(new Repository(['firefly' => ['web' => ['error-page' => ['max-frames' => $max]]]])), + ); + + expect($config(0)->maxFrames)->toBe(1) + ->and($config(-7)->maxFrames)->toBe(1) + ->and($config(100000)->maxFrames)->toBe(500) + ->and(ErrorPageSettings::fromConfig(new Config(new Repository))->maxFrames)->toBe(40); +}); + +it('names the UNTRIMMED stack in the trace header, so a budgeted page cannot pass itself off as a whole one', function () { + // The budget and the header are two halves of one promise. Trimming to six frames and then counting the + // six is a label that was honest before `max-frames` existed and stops being honest the moment it does: + // it says "2 of 6 in your code" for a stack of a hundred and marks nothing where ninety-four frames went. + $trimmed = new ErrorPageSettings(trace: true, hints: false, maxFrames: 6); + $error = ErrorReport::of(new RuntimeException('boom'), Request::create('/x'), $trimmed, dirname(__DIR__, 4), 500, 'Internal Server Error', '2026-01-01T00:00:00+00:00'); + $html = ErrorPage::render($error, $trimmed); + + expect($error->frameCount)->toBeGreaterThan(6) + ->and($html)->toContain('6 of '.$error->frameCount.' frames · '.$error->appFrameCount.' in your code') + // One row per BUDGETED frame, still: the header names what was dropped, it does not smuggle it back. + ->and(substr_count($html, '
  • toContain($all->frameCount.' frames · '.$all->appFrameCount.' in your code') + ->not->toContain(' of '.$all->frameCount.' frames'); +}); + +it('answers an RFC 9457 `instance` that is root-relative, including at the site root', function () { + // A relative reference resolves against the document's base URI, so `orders/42` served from /orders/42 + // identifies /orders/orders/42 — the member stops naming the occurrence it exists to name. The site + // root is the case a naive '/'.$path would get wrong in the other direction, answering '//'. + expect(ProblemMapper::instanceFor(Request::create('/orders/42')))->toBe('/orders/42') + ->and(ProblemMapper::instanceFor(Request::create('/api/v1/accounts/42')))->toBe('/api/v1/accounts/42') + ->and(ProblemMapper::instanceFor(Request::create('/')))->toBe('/') + // A query string is not part of the path, and a trailing slash is normalised away by Laravel before + // this ever sees it — both are the framework's answer, pinned here because the member depends on it. + ->and(ProblemMapper::instanceFor(Request::create('/orders/42?include=lines')))->toBe('/orders/42') + ->and(ProblemMapper::instanceFor(Request::create('/orders/')))->toBe('/orders'); + + // AND THE SLASH THIS METHOD ADDS CANNOT OPEN AN AUTHORITY. `\`, tab, LF and CR are the four characters + // a URL parser reads past or deletes on its way to a second separator, so `/\evil.example` would resolve + // to https://evil.example while the unprefixed `\evil.example` it came from resolves against this + // origin. Each is percent-encoded in the METHOD rather than at a call site, because ErrorReport builds + // its copy of the member from this same call and would otherwise need its own guard. Request::create() + // is the one place Symfony refuses a backslash, so the request has to be built the long way — the + // published document is exercised end to end in ProblemDetailsRendererTest. + $raw = Request::createFromBase(new SymfonyRequest([], [], [], [], [], [ + 'REQUEST_URI' => "/\\evil.example/ph\tish", + 'REQUEST_METHOD' => 'GET', + 'HTTP_HOST' => 'app.test', + 'SCRIPT_NAME' => '/index.php', + 'SCRIPT_FILENAME' => '/index.php', + ])); + + expect($raw->path())->toBe("\\evil.example/ph\tish") + ->and(ProblemMapper::instanceFor($raw))->toBe('/%5Cevil.example/ph%09ish') + // And the answer is one this package's own href vocabulary accepts unchanged, which is the same + // question ErrorPage::retry() asks of the address it prints. + ->and(ErrorPageSettings::url(ProblemMapper::instanceFor($raw)))->toBe('/%5Cevil.example/ph%09ish'); +}); + +it('renders one
  • per budgeted frame and no more, so the page cannot be a wall', function () { + $settings = new ErrorPageSettings(trace: true, hints: false, maxFrames: 8); + $error = ErrorReport::of(new RuntimeException('boom'), Request::create('/x'), $settings, dirname(__DIR__, 4), 500, 'Internal Server Error', '2026-01-01T00:00:00+00:00'); + $html = ErrorPage::render($error, $settings); + + $shownVendor = count(array_filter($error->frames, static fn ($f): bool => $f->vendor)); + $allVendor = $error->frameCount - $error->appFrameCount; + + expect(substr_count($html, '
  • ') + substr_count($html, '
  • '))->toBe(8) + // And the header is honest about what it dropped, rather than quietly showing eight of a hundred. + ->toBeLessThan($error->frameCount) + ->and($html)->toContain('8 of '.$error->frameCount.' frames') + ->toContain($error->appFrameCount.' in your code') + // THE DISCLOSURE COUNTS THE SAME UNTRIMMED STACK THE HEADER DOES. Its label is the dependency + // frames in the whole stack, not the handful that survived the budget — `count($vendor)` there + // would read "2 frames in your dependencies" over a trace with fifty of them — and the `.dn` note + // beside it is what makes that honest rather than merely large: it says how many of those fifty + // are actually in the list below. Both halves are asserted, because a label that carries whatever + // number it is given passes a `toContain('frames in your dependencies')` either way. + ->and($allVendor)->toBeGreaterThan($shownVendor) + ->and($html)->toContain(''.$allVendor.' frames in your dependencies') + ->toContain(''.$shownVendor.' shown'); +}); + +it('puts your frames in the list and your dependencies behind one disclosure', function () { + $settings = new ErrorPageSettings(trace: true, hints: false, maxFrames: 60); + $error = ErrorReport::of(new RuntimeException('boom'), Request::create('/x'), $settings, dirname(__DIR__, 4), 500, 'Internal Server Error', '2026-01-01T00:00:00+00:00'); + $html = ErrorPage::render($error, $settings); + + $own = strpos($html, '
  • '); + $deps = strpos($html, '
    '); + + expect($own)->toBeInt() + ->and($deps)->toBeInt() + // Your code first, always: the vendor set opens BELOW it and starts closed. Stated as a boolean + // rather than through toBeLessThan(): strpos() answers int|false, an expectation does not narrow + // the variable it was given, and comparing a possible false at level max is an error, not a style. + ->and($own !== false && $deps !== false && $own < $deps)->toBeTrue() + ->and($html)->toContain(''.($error->frameCount - $error->appFrameCount).' frames in your dependencies') + // And with nothing trimmed the note is not written at all, for the reason the header leaves out + // its "of": "32 shown" beside "32 frames in your dependencies" is a question a reader should not + // have to answer to know they are looking at the whole vendor set. The budget is stated here + // rather than assumed — a Pest stack longer than it would silently turn this into the other case. + ->and($error->frameCount)->toBeLessThanOrEqual(60) + ->and($html)->not->toContain('class="dn"') + ->and(substr_count($html, '
    toBe(1) + // A closed
    is a real control with real keyboard behaviour. The alternative a judge + // measured — a checkbox in one div and a `~` selector reaching for a sibling of its PARENT — matches + // nothing at all, and makes every vendor frame permanently unreachable with scripts on or off. + ->and($html)->not->toContain('~ .tw') + ->not->toContain('type="checkbox"'); +}); + +it('opens exactly one frame, and makes the others exclusive rather than merely closed', function () { + // The throwable is built one call DEEPER than the test closure on purpose. Pest invokes that closure + // from its own vendor code, so a throwable constructed inline has exactly one frame of the + // application's — nothing to be exclusive WITH — and the property under test is precisely what happens + // from the second such frame onward. + $deeper = static fn (): RuntimeException => new RuntimeException('boom'); + + $settings = new ErrorPageSettings(trace: true, hints: false); + $error = ErrorReport::of($deeper(), Request::create('/x'), $settings, dirname(__DIR__, 4), 500, 'Internal Server Error', '2026-01-01T00:00:00+00:00'); + $html = ErrorPage::render($error, $settings); + + $disclosable = count(array_filter($error->frames, static fn ($f): bool => $f->excerpt !== [])); + + expect($disclosable)->toBeGreaterThan(1) + // `name=` is HTML's own exclusive accordion: opening one closes the rest, with no script and no CSS. + ->and(substr_count($html, '
    '))->toBe(1) + ->and(substr_count($html, '
    '))->toBe($disclosable - 1) + ->and(substr_count($html, '
    '))->toBeGreaterThan(0); +}); + +it('draws a frame as one line: a directory that may be clipped and a file name that never is', function () { + $settings = new ErrorPageSettings(trace: true, hints: false); + $error = ErrorReport::of(new RuntimeException('boom'), Request::create('/x'), $settings, dirname(__DIR__, 4), 500, 'Internal Server Error', '2026-01-01T00:00:00+00:00'); + $html = ErrorPage::render($error, $settings); + + expect($html)->toContain('ErrorPageTest.php') + ->toContain('packages/web/tests/Error/') + // The row is a nowrap flex line and the two shrinkable spans are ellipsised. The old rule wrapped + // every row onto three lines, which is what a 10,108-pixel page is made of. + ->toContain('.frames .row,.frames summary{display:flex') + ->toContain('flex-wrap:nowrap') + ->toContain('.frames .base{') + ->and($html)->not->toContain('.frames .where'); +}); + +it('labels a dependency frame with its package, which was dead until the paths were shortened', function () { + $settings = new ErrorPageSettings(trace: true, hints: false, maxFrames: 60); + $error = ErrorReport::of(new RuntimeException('boom'), Request::create('/x'), $settings, dirname(__DIR__, 4), 500, 'Internal Server Error', '2026-01-01T00:00:00+00:00'); + $html = ErrorPage::render($error, $settings); + + expect($html)->toMatch('#[a-z0-9._-]+/[a-z0-9._-]+#'); +}); + +it('gives the method name a span of its own, so the token a vendor frame is known by is never the clipped end', function () { + $settings = new ErrorPageSettings(trace: true, hints: false, maxFrames: 60); + // A Pest run reaches this closure through its own vendor code, so the trace really is a deep vendor + // stack — the shape the row was designed for and the one that exposed the bug. + $error = ErrorReport::of(new RuntimeException('boom'), Request::create('/x'), $settings, dirname(__DIR__, 4), 500, 'Internal Server Error', '2026-01-01T00:00:00+00:00'); + $html = ErrorPage::render($error, $settings); + + $vendor = count(array_filter($error->frames, static fn ($f): bool => $f->vendor)); + + expect($vendor)->toBeGreaterThan(5) + // `.call` used to be one ellipsised span, and a call is `Class->method()`: the clip took the METHOD + // NAME and kept the namespace every Illuminate frame shares. The pair is now the path's pair. + ->and($html)->toMatch('#[^<]+(->|::)[A-Za-z_]#') + // A phone drops the qualifier whole rather than shortening the method name. + ->toContain('.frames .call .cls{display:none}') + // The old single span, with the whole call inside the shrinkable box, must not come back. + ->not->toContain('.frames .call{font-family:var(--mono);font-size:12px;color:var(--ink-3);margin-left:auto;min-width:0;flex:0 1 auto;overflow:hidden') + // The synthetic throw frame has no qualifier at all, so the slot is simply not printed. + ->and($html)->toContain('throw'); +}); + +it('ranks what a narrow row gives up, and takes it inside the row rather than at the panel edge', function () { + $settings = new ErrorPageSettings(trace: true, hints: false, maxFrames: 60); + $error = ErrorReport::of(new RuntimeException('boom'), Request::create('/x'), $settings, dirname(__DIR__, 4), 500, 'Internal Server Error', '2026-01-01T00:00:00+00:00'); + $html = ErrorPage::render($error, $settings); + + // `.fn` WAS `flex:none`, on the theory that a span which cannot shrink keeps its text. It does not: it + // keeps its WIDTH, its own `text-overflow` never gets a box narrower than its glyphs to draw an + // ellipsis in, and the glyphs run out of the row to be cut by `.panel{overflow:hidden}` with nothing + // marking the cut. Measured in Chrome at 375px over a 35-frame Laravel trace, 34 of 35 rows painted + // their method name up to 138px past the panel and `->whereHasMorphRelationship` arrived as `->wh`. + expect($html) + // The rank is a shrink FACTOR, not a refusal to shrink: .dir and .cls at 100, .fn at 1, so flexbox + // spends both discardable spans to nothing before it takes a character off the method name. + ->toContain('.frames .dir{font-family:var(--mono);font-size:12.5px;color:var(--ink-2);min-width:0;flex:0 100 auto;overflow:hidden;text-overflow:ellipsis}') + ->toContain('.frames .call .cls{min-width:0;flex:0 100 auto;overflow:hidden;text-overflow:ellipsis}') + ->toContain('.frames .call .fn{min-width:0;flex:0 1 auto;max-width:24em;overflow:hidden;text-overflow:ellipsis}') + // And the row contains its own overflow, so nothing is ever cut by the panel instead. + ->toContain('display:flex;align-items:baseline;overflow:hidden}') + ->not->toContain('.frames .call .fn{flex:none') + // A PHONE DOES NOT SPEND THE LAST RANK AT ALL. At 375px a row has 295px and a real Laravel file + // name — `AddQueuedCookiesToResponse.php`, which never shortens — is 226 of them, so ranked + // shrinking alone ends with 23 of 35 method names cut and two rendered at zero width. The row wraps + // instead and the call takes a line of its own; the directory goes with `.pkg` and `.cls`, which is + // what holds the wrapped row to two lines. Nothing wraps INSIDE a span — that was the pre-wave rule + // at 87px a row — so the break can only fall between two whole tokens. + ->toContain('.frames .row,.frames summary{flex-wrap:wrap}') + ->toContain('.frames .dir{display:none}') + ->toContain('.frames .call{margin-left:0}') + // With a line to itself the call keeps the 24em guard it has everywhere: the phone's old 14em cap + // was cutting `->sendRequestThroughRouter` for room it no longer needs. + ->not->toContain('max-width:14em') + // The base rule stays nowrap; only the phone block relaxes it, which is what keeps a desktop row + // on one line at every panel width from 540px up (measured: 0 rows past the panel, 35 of 35 on one + // line). THE GEOMETRY ITSELF IS PINNED IN tests/Browser/ErrorPagesDebugTest.php, not here, by + // TRACE_CALLS_INSIDE_PANEL: `assertSee` reads text content, which is identical whether a span is + // drawn whole or clipped in half at the panel's edge, so the phone-width case measures every + // rendered `.fn` right edge against its `.panel` right edge instead. + ->toContain('flex-wrap:nowrap;white-space:nowrap}'); +}); + +it('keeps every text token above 4.5:1, and the focus ring above 3:1, on every ground each is drawn on', function () { + // --ink-3 was #8d95a1 in light (2.80:1 on the page ground, 3.02:1 on a panel) and #6c7883 in dark + // (3.76:1 on the inset panel). It carries the call, the frame index and the panel counts — small text a + // reader is asked to compare, which is the last place to spend contrast. + $html = ErrorPage::render( + ErrorReport::of(new RuntimeException('boom'), Request::create('/x'), new ErrorPageSettings(trace: true), dirname(__DIR__, 4), 500, 'Internal Server Error', '2026-01-01T00:00:00+00:00'), + new ErrorPageSettings(trace: true), + ); + + $ratio = static function (string $a, string $b): float { + $lum = static function (string $hex): float { + $hex = ltrim($hex, '#'); + $channel = static fn (float $c): float => $c <= 0.03928 ? $c / 12.92 : (($c + 0.055) / 1.055) ** 2.4; + + return 0.2126 * $channel((int) hexdec(substr($hex, 0, 2)) / 255) + + 0.7152 * $channel((int) hexdec(substr($hex, 2, 2)) / 255) + + 0.0722 * $channel((int) hexdec(substr($hex, 4, 2)) / 255); + }; + + [$hi, $lo] = $lum($a) >= $lum($b) ? [$lum($a), $lum($b)] : [$lum($b), $lum($a)]; + + return ($hi + 0.05) / ($lo + 0.05); + }; + + expect($html)->toContain('--ink-3:#696f7d') + ->toContain('--ink-3:#828e99') + // Light: page ground, panel, inset panel. + ->and($ratio('#696f7d', '#f7f6f3'))->toBeGreaterThan(4.5) + ->and($ratio('#696f7d', '#ffffff'))->toBeGreaterThan(4.5) + ->and($ratio('#696f7d', '#faf9f6'))->toBeGreaterThan(4.5) + // Dark: the same three. + ->and($ratio('#828e99', '#0f1214'))->toBeGreaterThan(4.5) + ->and($ratio('#828e99', '#15191c'))->toBeGreaterThan(4.5) + ->and($ratio('#828e99', '#181d21'))->toBeGreaterThan(4.5); + + // THE FOCUS RING, which is not text and so is measured against SC 1.4.11's 3:1, not 4.5:1. It is drawn + // on two grounds: a frame's summary on --panel, and the dependency disclosure — the one control a + // keyboard reader MUST operate to reach the frames behind it — on --panel-2. In --brand those measured + // 3.01:1 and 2.86:1, so the ring on the control that matters most was the one below the floor. + expect($html)->toContain('--brand-ink:#a1520a') + ->toContain('.frames summary:focus-visible{outline:2px solid var(--brand-ink)') + ->toContain('.deps>summary:focus-visible{outline:2px solid var(--brand-ink)') + // The copy button's ring is the third one, and it is measured on the same two grounds: the + // button sits on --panel-2 inside a --panel cell, which is exactly where --brand fell short. + ->toContain('.copy:focus-visible{outline:2px solid var(--brand-ink)') + // The action row's ring is the fourth, and it is drawn on the page ground the header sits on — + // #e07a17 measures 2.95:1 there, so the row that gives a keyboard reader the way off this page + // would have shipped the one ring below the floor. + ->toContain('.act:focus-visible{outline:2px solid var(--brand-ink)') + // Pinned as a string so the token cannot silently go back to the shape one that fails. + ->not->toContain('outline:2px solid var(--brand)') + ->and($ratio('#a1520a', '#ffffff'))->toBeGreaterThan(3.0) + ->and($ratio('#a1520a', '#faf9f6'))->toBeGreaterThan(3.0) + // Dark leaves the ring on --brand's own value; both tokens are #ff9d3c there. + ->and($ratio('#ff9d3c', '#15191c'))->toBeGreaterThan(3.0) + ->and($ratio('#ff9d3c', '#181d21'))->toBeGreaterThan(3.0) + // And the value that was failing is recorded as failing, so the swap cannot be undone by accident. + ->and($ratio('#e07a17', '#faf9f6'))->toBeLessThan(3.0); +}); + +it('shows the frames when not one of them is yours, instead of a heading over a closed disclosure', function () { + // THE STACK THIS PINS IS NOT A HYPOTHETICAL. `ErrorFrame::$vendor` is decided by `/vendor/` in the + // path, so a stack has no application frame whenever nothing application-owned is on it: a routing + // miss, a 405, a container or bootstrap throw — everything raised BEFORE application code runs — under + // any deployment whose front controller is itself a dependency, which is Octane, FrankenPHP worker mode + // and Vapor. The split then has nothing to put in its list, and with the disclosure hardcoded closed + // the trace panel came out as a heading over one collapsed row with not a single frame in sight: worse + // than the interleaved trace the split replaced, and a flat contradiction of "nothing is hidden". + // + // WHY THE REPORT IS BUILT BY HAND. This repository cannot produce the shape through ErrorReport::of(): + // a PHP process always has its entry script at the bottom of the stack, and here that script is a test + // file under `packages/` or `tests/` — application code by the same `/vendor/` rule. Measured against + // the wave's own browser harness, even its routing-miss 404 reports `40 of 98 frames · 9 in your code`, + // because the in-process server is driven from `tests/Browser/`. So the renderer is fed the report + // Octane hands it instead, which is the unit that has the decision: the page is a pure function of the + // report, and the report shape is the one the pipeline genuinely builds off the floor of a vendored + // front controller. + $class = new ReflectionClass(ErrorReport::class); + $constructor = $class->getConstructor(); + + expect($constructor)->not->toBeNull(); + + $report = $class->newInstanceWithoutConstructor(); + $constructor?->invokeArgs($report, [ + 'status' => 404, + 'reason' => 'Not Found', + 'code' => 'ROUTE_NOT_FOUND', + 'category' => 'business', + 'severity' => 'warning', + 'method' => 'GET', + 'path' => '/does-not-exist', + 'query' => '', + 'timestamp' => '2026-01-01T00:00:00+00:00', + 'detailed' => true, + 'exceptionClass' => 'Symfony\Component\HttpKernel\Exception\NotFoundHttpException', + 'message' => 'The route does-not-exist could not be found.', + 'location' => 'vendor/laravel/framework/src/Illuminate/Routing/AbstractRouteCollection.php:44', + 'frames' => [ + new ErrorFrame('/srv/vendor/laravel/framework/src/Illuminate/Routing/AbstractRouteCollection.php', 'vendor/laravel/framework/src/Illuminate/Routing/AbstractRouteCollection.php', 44, 'throw', true, [], 0), + new ErrorFrame('/srv/vendor/laravel/framework/src/Illuminate/Routing/Router.php', 'vendor/laravel/framework/src/Illuminate/Routing/Router.php', 731, 'Illuminate\Routing\AbstractRouteCollection->handleMatchedRoute()', true, [], 1), + new ErrorFrame('/srv/vendor/laravel/octane/bin/swoole-server', 'vendor/laravel/octane/bin/swoole-server', 21, 'Laravel\Octane\Worker->handle()', true, [], 2), + ], + 'frameCount' => 3, + 'appFrameCount' => 0, + ]); + + $html = ErrorPage::render($report, new ErrorPageSettings(trace: true, hints: false)); + + expect($html) + // A reader meets three frames, not a closed row: the disclosure is open because it is the panel. + ->toContain('
    ') + ->toContain('3 frames in your dependencies') + ->and(substr_count($html, '
  • '))->toBe(3) + ->and($html)->toContain('AbstractRouteCollection.php') + ->toContain('swoole-server') + // The header still counts the stack, and still says none of it is the application's. + ->toContain('3 frames · 0 in your code') + // And the one place a closed
    and an open one sit differently: the heading above it + // already draws a border-bottom, so the disclosure drops its own border-top when it follows one. + ->toContain('.panel h2+.deps{border-top:0}'); +}); + +it('leaves the dependency disclosure closed whenever there are frames above it to read', function () { + // The other side of the branch, and the one that must not move: when the split has a list, the + // dependency set is the stack the reader came THROUGH and starts collapsed. Driven through + // ErrorReport::of() rather than by hand, because this shape is the one a real run produces. + $settings = new ErrorPageSettings(trace: true, hints: false, maxFrames: 60); + $error = ErrorReport::of(new RuntimeException('boom'), Request::create('/x'), $settings, dirname(__DIR__, 4), 500, 'Internal Server Error', '2026-01-01T00:00:00+00:00'); + $html = ErrorPage::render($error, $settings); + + expect($error->appFrameCount)->toBeGreaterThan(0) + ->and($html)->toContain('
  • ') + ->toContain('
    ') + ->not->toContain('
    '); +}); + +it('draws the fact grid on the panel ground, so a ragged last row is not a dead beige cell', function () { + // The grid is `gap:1px` over a coloured container, so the container shows through between cells — and + // through the WHOLE trailing area whenever the fact count is not a multiple of the (responsive, and + // therefore unknowable) column count. Six facts in four columns is two dead cells, which is what every + // production screenshot shows. The rules are drawn by each cell instead, outset and clipped. + $settings = new ErrorPageSettings(trace: false, hints: false); + $error = ErrorReport::of(new RuntimeException('boom'), Request::create('/x'), $settings, dirname(__DIR__, 4), 500, 'Internal Server Error', '2026-01-01T00:00:00+00:00'); + $html = ErrorPage::render($error, $settings); + + expect($html)->toContain('.facts{display:grid') + ->toContain('background:var(--panel)') + ->toContain('.facts>div{') + ->toContain('box-shadow:-1px -1px 0 var(--line)') + ->and($html)->not->toMatch('#\.facts\{[^}]*background:var\(--line\)#') + ->not->toMatch('#\.facts\{[^}]*gap:1px#'); +}); + +it('prints the reference once, as something a reader can select in one gesture', function () { + $settings = new ErrorPageSettings(trace: false, hints: false); + $request = Request::create('/orders/42', 'GET', server: ['HTTP_X_CORRELATION_ID' => 'ref-once-1234']); + $error = ErrorReport::of(new RuntimeException('boom'), $request, $settings, dirname(__DIR__, 4), 500, 'Internal Server Error', '2026-01-01T00:00:00+00:00'); + $html = ErrorPage::render($error, $settings); + + expect($html)->toContain('
    Reference
    ref-once-1234
    ') + ->toContain('Quote this if you report the problem.') + // The sentence points AT the cell rather than repeating the id into prose. + ->toContain('quote the reference below if you report it') + ->not->toContain('quote reference ref-once-1234') + // `user-select:all` is the affordance that needs nothing: one click takes the whole id, with + // JavaScript off, in a container's minimal browser, in a screenshot tool's headless Chromium. + ->toContain('.fact-ref dd:first-of-type{user-select:all'); +}); + +it('ships the copy button hidden and reveals it from the same script that gives it behaviour', function () { + $settings = new ErrorPageSettings(trace: false, hints: false); + $request = Request::create('/x', 'GET', server: ['HTTP_X_CORRELATION_ID' => 'ref-copy-99']); + $error = ErrorReport::of(new RuntimeException('boom'), $request, $settings, dirname(__DIR__, 4), 500, 'Internal Server Error', '2026-01-01T00:00:00+00:00'); + $html = ErrorPage::render($error, $settings); + + expect($html)->toContain('') + ->toContain('navigator.clipboard') + // A control that does nothing is worse than no control: with scripts off, with a CSP that refuses + // an inline script, or on a plain-http origin where navigator.clipboard is undefined, the button + // stays hidden and the select-all cell is still there. + // + // AND THE CLICK HAS A REJECTION ARM, because those three are the modes the REVEAL can see. + // writeText() rejects with navigator.clipboard present and the guard already passed — an unfocused + // document (an ordinary DOMException, and the common one), a denied `clipboard-write` permission, an + // embedding Permissions-Policy that omits it — and in every one of those the button has already been + // shown. Without this the click does nothing at all, the label stays "Copy", and the quietest page + // in the framework writes an unhandled promise rejection to the console. + ->toContain('.catch(function(){b.textContent="Copy failed"})') + ->and(substr_count($html, '']); + $error = ErrorReport::of(new RuntimeException('boom'), $request, $settings, dirname(__DIR__, 4), 500, 'Internal Server Error', '2026-01-01T00:00:00+00:00'); + $html = ErrorPage::render($error, $settings); + + expect($html)->not->toContain('') + ->toContain('<script>') + ->toContain('data-ref=""><script>'); +}); + +it('gives a 404 the same single reference cell, so one page teaches the other', function () { + // CorrelationIdFilter mints an id when the request carried none, so every rendered page has a reference + // — but only the 5xx sentence names it. The CELL is the constant: whatever the status, the id is in one + // place, selectable, with the same words under it. + $settings = new ErrorPageSettings(trace: false, hints: false); + $request = Request::create('/nope', 'GET', server: ['HTTP_X_CORRELATION_ID' => 'ref-404-aa']); + $html = ErrorPage::render( + ErrorReport::of(new NotFoundHttpException, $request, $settings, dirname(__DIR__, 4), 404, 'Not Found', '2026-01-01T00:00:00+00:00'), + $settings, + ); + + expect($html)->toContain('
    Reference
    ref-404-aa
    ') + ->toContain('Quote this if you report the problem.') + // A 404 is not "something went wrong on our side", so that sentence is not on it. + ->not->toContain('quote the reference below'); +}); + +it('keeps the id shared with the problem document and lets the sentence diverge on purpose', function () { + // The 5xx lede used to be ProblemMapper::OPAQUE_WITH_REFERENCE's wording, id and all, so a ticket read + // the same whichever surface the failure was seen on. It is not any more — and that is a decision, not + // drift: naming the id in prose AND in the cell put one uuid on the page twice with no way to copy + // either. A problem document has no cell to point at, so it keeps its id inline and keeps its wording. + // + // What survives as the cross-surface invariant is the ID, not the SENTENCE, and this pins both halves so + // neither can be re-decided in silence. The page's clause is read off a real render rather than typed, + // so the day the lede changes this test names the sentence that has to change with it. + $settings = new ErrorPageSettings(trace: false, hints: false); + $request = Request::create('/x', 'GET', server: ['HTTP_X_CORRELATION_ID' => 'ref-split-7']); + $error = ErrorReport::of(new RuntimeException('boom'), $request, $settings, dirname(__DIR__, 4), 500, 'Internal Server Error', '2026-01-01T00:00:00+00:00'); + $html = ErrorPage::render($error, $settings); + + if (preg_match('/It has been logged; ([^.<]+)\./', $html, $matched) !== 1) { + throw new RuntimeException('The production 5xx lede no longer carries an "It has been logged; …" clause for this test to read.'); + } + $clause = $matched[1]; + $document = sprintf(ProblemMapper::OPAQUE_WITH_REFERENCE, $error->reference); + + expect($error->reference)->toBe('ref-split-7') + // The id is on both surfaces: in the page's cell, inline in the document. + ->and($html)->toContain('
    Reference
    ref-split-7
    ') + ->and($document)->toContain('ref-split-7') + // The sentence is not. The page's clause points at the cell and names no id; the document's names + // it, and neither is a substring of the other. + ->and($clause)->not->toContain('ref-split-7') + ->and($document)->not->toContain($clause); +}); + +$page = static fn (Throwable $e, ErrorPageSettings $settings, int $status, string $reason, string $method = 'GET', string $path = '/orders/42'): string => ErrorPage::render( + ErrorReport::of($e, Request::create($path, $method), $settings, dirname(__DIR__, 4), $status, $reason, '2026-01-01T00:00:00+00:00'), + $settings, +); + +it('offers a 401 the way back in, and only when one was configured', function () use ($page) { + $configured = new ErrorPageSettings(trace: false, hints: false, home: '/', signIn: '/login'); + $bare = new ErrorPageSettings(trace: false, hints: false, home: '', signIn: ''); + + $html = $page(new AuthenticationException('Authentication is required to access this resource.', 'AUTHENTICATION_FAILED'), $configured, 401, 'Unauthorized'); + + expect($html)->toContain('