Repository navigation
TEMPORARY: demonstrate CI fails on a failing vitest assertion (do not merge) - #98
Closed
MaximumTrainer wants to merge 9 commits into
Closed
MaximumTrainer wants to merge 9 commits into
MaximumTrainer wants to merge 9 commits into
Conversation
vitest.config.ts excluded 'playwright/**', but the Playwright specs live in e2e/ (playwright.config.ts testDir). Vitest therefore collected all six e2e specs and every one failed on test.describe(), so `npm test` exited 1 on a clean checkout while the 108 real unit tests passed. - Scope vitest to src/** and exclude e2e/** so the two runners own disjoint directories. - Delete src/frontend/playwright/, a byte-identical copy of e2e/onboarding-journeys.spec.ts sitting outside Playwright's testDir, so no runner ever executed it. - Run `npm test` in the frontend-ci job. The job previously ran lint and build only, which is why the red suite went unnoticed. - Ignore playwright-report/ and test-results/. Verified: npm test 14 files / 108 tests green, exit 0; a deliberately failing assertion exits 1; npx playwright test --list still finds 26 tests in 6 files. Closes #92 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Metadata advertised KeyDescriptor use="encryption" but the callback never configured a decryption key, so an IdP that honoured the advertised key produced assertions the ACS could not unwrap - every such login failed as a generic saml_invalid_assertion. - Register the SP certificate in Saml2Configuration.DecryptionCertificates so EncryptedAssertion responses decrypt. No new configuration is needed: the certificate published in metadata is the one used to decrypt. - Add Authentication:Saml:RequireEncryptedAssertion. When true, an unencrypted assertion is rejected with saml_assertion_not_encrypted. - Detect an expired SP or IdP certificate and redirect with saml_certificate_expired. Login checks the SP certificate it signs with; the callback also checks the IdP certificate it verifies against. - Log decryption failures distinctly from signature failures, so an operator can tell a key mismatch from a tampered assertion behind the deliberately generic browser-facing error. This makes every code in the frontend LOGIN_ERROR_MESSAGES table reachable; saml_certificate_expired and saml_assertion_not_encrypted were previously dead entries. Tests: 7 added covering the encrypted happy path, encryption to an unknown key, both RequireEncryptedAssertion settings, and expired SP/IdP certificates. SamlAuthControllerTests 19/19, suite 245/245. Closes #94 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An SSE stream is pinned to the instance that accepted it, but InMemorySessionEventEmitter was the only ISessionEventEmitter and kept per-session channels in process memory. Above one replica an event raised on instance A never reached a stream held by instance B: no error, no closed stream, the applicant just stops receiving progress. Both CD workflows deploy to platforms that scale out by default. - Add ISessionEventTransport, a port for fanning an event out to every instance, with RabbitMqSessionEventTransport as the adapter (fanout exchange, per-instance exclusive auto-delete queue, transient messages). - Add DistributedSessionEventEmitter, which broadcasts over the transport and delivers to local channels on receipt. It subscribes during startup as an IHostedService, before the first request is served. - Extract the channel mechanics into SessionEventChannels so capacity bounds, terminal-event completion and stale-channel cleanup are the same code for both emitters rather than two implementations to keep in step. - Select by SessionEvents:Transport, mirroring how IEventBus is selected. The RabbitMQ URI falls back to the event bus URI, so a deployment already running RabbitMQ needs only the one setting. In-memory stays the default for development, tests and single-replica deployments. - Warn at startup when a non-Development environment uses the in-memory emitter, since the failure it risks is otherwise silent. Tests: 16 added, including two emitter instances sharing one transport where the event is raised on A and asserted on B's stream, terminal events completing streams on every instance, and capacity/cleanup behaviour asserted against both implementations from one theory. Suite 259/259. Closes #93 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…token
VITE_API_KEY was inlined into the published bundle by Vite, and
ApiKeyAuthenticationHandler maps any valid key to a full Operator
principal. Every visitor to the public onboarding page could read the key
out of the JavaScript and then list all sessions, read every submission's
PII, rewrite flow definitions, or register a webhook to exfiltrate future
events. SessionOwnershipHandler short-circuits on IsInRole(Operator), so
per-session checks were no defence either.
Backend:
- POST /api/workflow/sessions/start is now anonymous - it is the entry
point of a public journey, so there is no credential to present yet. It
keeps the existing session-start rate limit and returns an applicant
token scoped to the session it just created.
- Add ApplicantSessionTokenService and an ApplicantSession scheme. The
token is an HMAC-signed JWT naming one session and carrying only the
Applicant role. The policy scheme routes bearer tokens by issuer, so
applicant tokens and external IdP tokens coexist.
- SessionOwnershipHandler accepts a token whose sessionId claim matches
the session, which does not depend on a customer profile existing.
- Require Authentication:ApplicantToken:SigningKey outside Development,
mirroring JwtAuthorityValidator; Development uses an ephemeral key.
- Enforce ownership on GET sessions/{id}/events, which had none: any
Applicant or Operator could stream any session. EventSource cannot set
headers, so that endpoint also accepts the token as an access_token
query parameter, checked identically.
- Pin Microsoft.IdentityModel.JsonWebTokens to match the Tokens version
ITfoxtec pulls; the mismatched pair threw MissingMethodException when
reading a JWT.
Frontend:
- Applicant requests carry the session token; operator surfaces go through
operatorFetch, which sends the AdminSession cookie and no API key.
- Drop the apiKey prop from every component and VITE_API_KEY from source
and .env.example.
Contracts: credentials are no longer recorded in the Pact interactions -
the consumer's token means nothing to the provider - and the provider
verifier injects a real token minted for the seeded session.
Verified: a production build with VITE_API_KEY set contains neither the
key nor any X-Api-Key header. Backend 272/272, frontend 117/117, mocked
e2e journeys 3/3. 13 integration tests cover the token being accepted for
its own session and rejected for another session, another session's event
stream, and every operator endpoint.
Closes #90
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
App.tsx guarded only /admin/journey-builder. The root route - the page an applicant loads to complete onboarding - rendered flow authoring, version history, analytics, the session list (including applicants' submitted fields) and webhook deliveries below the step renderer, with no auth gate. It also offered journey and persona pickers that let an applicant switch themselves onto any seeded flow. - Add src/routing/routes.ts as the single route -> required-role table. Its default is operator-only: an unmatched /admin path is guarded rather than falling through to the applicant page, so a new admin surface cannot be added without a guard, which is how this arose. - Add AdminConsolePage at /admin holding the moved operator sections, and reduce the applicant route to ApplicantJourneyPage: the step renderer, progress, and completion. - Applicants arrive on a link naming their journey (/?flowId=...) instead of choosing from a picker of every seeded flow. - Check identity only on routes that need it, so the public page makes no /api/auth/me call at all. - Derive the auth state from the path it was fetched for, so navigating between admin routes re-checks rather than reusing the previous answer. - Polyfill ResizeObserver and DOMMatrixReadOnly in test-setup; jsdom has neither and React Flow calls both on mount. Tests: routes.ts has its own unit tests including the unmatched-admin-path case. App.routing.test.tsx asserts the unauthenticated root renders no operator heading, no journey or persona selector, and issues no request to any operator endpoint. Playwright covers the redirect from /admin and from an unrouted admin path, and that the public page stays clean. Frontend 139/139, typecheck and lint clean, 16 mocked e2e passing. The 13 remaining e2e are the journey-a/b/c API specs, which need a backend on :5072 that CI provides. Closes #91 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ents
The analytics pipeline was complete except for its ends: the only
IAnalyticsProvider wrote to the logger, and browser-side events had
nowhere to go - there was no ingestion endpoint at all. Per-step timing,
drop-off points and every client signal were unreadable after the fact.
Backend:
- Add the AnalyticsEvents table, DatabaseAnalyticsProvider and an
IAnalyticsEventStore port for reading a trail back and pruning it.
Selected by Analytics:DatabaseProvider:Enabled, like IEventBus and
IVirusScanService; disabling it leaves the console provider working.
- Add POST /api/analytics/events. Every event in a batch must name the
session the caller's applicant token authorises or the whole batch is
refused, so a client cannot write events against another journey. The
server stamps source=client rather than trusting the caller, and the
endpoint is rate limited and capped at 100 events per batch.
- Add GET /api/analytics/sessions/{id}/events, the ordered operator-only
trail.
- Add GET /api/analytics/flows/{flowId}. The operator analytics view has
been calling this all along and getting a 404; the existing stats live
at /api/workflow/flows/{id}/stats in a different shape.
- Prune events after Analytics:RetentionDays, mirroring the document
cleanup service. Retention is measured from write time, not the
client-reported timestamp a caller controls.
Frontend:
- Add an HTTP analytics sink that batches by size and interval, flushes on
pagehide so the tail of a journey is not lost when the tab closes, and
drops a failed batch instead of retrying. Analytics must never be able
to stall an application form, and an unbounded retry queue against a
broken endpoint would.
Tests: 11 backend ingestion tests including cross-session rejection and
that a mixed batch is refused whole; 4 retention tests including that a
backdated client timestamp cannot evict a trail early; 11 sink tests
covering batching, auth header, and failure isolation. Backend 287/287,
frontend 150/150, 16 mocked e2e, lint and build clean.
Closes #95
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Quality checks lived only in CI, so a broken build or a leaked credential
was found after push at the earliest. These catch the cheap cases locally.
Hooks live in .githooks/ and are enabled per clone through core.hooksPath,
so a change to them is reviewed like any other change - unlike scripts
copied into .git/hooks. `npm install` in src/frontend runs the installer,
and scripts/install-git-hooks.{sh,ps1} do it explicitly.
- pre-commit: scans the staged diff for PEM private keys, VITE_* secrets
and non-placeholder credentials in configuration, then lints or builds
only the areas the commit touches. The VITE_* rule encodes the #90
lesson: Vite inlines those into the browser bundle as literal strings.
- commit-msg: requires a Conventional Commits subject, the format this
history already follows and what a release tool needs to derive a
version from the log.
- pre-push: runs the backend and frontend suites and refuses a direct push
to main.
Scope is deliberate. Only suites that need no external service run in the
hooks; Pact provider verification and Playwright need PostgreSQL and a
browser and stay in CI. A hook that passes only on a correctly provisioned
machine is one people learn to bypass, and a bypassed hook enforces
nothing. Every hook honours SKIP_HOOKS=1, which is preferable to
--no-verify because it is visible in the command that used it.
Also corrects getting-started.md, which still told developers to put an
Operator-equivalent API key in .env.local - the practice the pre-commit
secret scan now blocks - and documents the current routes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An audit of #90, #92, #93 and #95 against their acceptance criteria found four claims that were implemented but never actually asserted. - #95 required the analytics ingest endpoint to be rate limited. The policy was wired but nothing proved it, and no test in the repository exercised a real limiter: the Testing environment swaps every one for a no-op, so the existing RateLimitingTests only assert that no-op. Give TestWebAppFactory an environment parameter and assert a genuine 429 plus its Retry-After header. - #93 required a startup warning when a non-Development environment runs the single-instance emitter. Only its registration was covered. The failure it guards is silent at runtime, so the warning is the operator's only signal: assert it fires for Production and Staging, names the environment and the fix, and stays quiet for Development and Testing. - #90 required operator views to depend on the AdminSession cookie rather than an API key. Cover the case directly: every operator endpoint rejects a request carrying no credential, and still serves one that does. Backend suite 287 -> 300. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
Author
|
Demonstration complete. |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Throwaway draft opened only to satisfy the last acceptance criterion of #92:
Contains one deliberately failing test. Do not merge. This PR is closed and the branch deleted as soon as the
Frontend — lint, test & buildcheck reports failure.