Skip to content

Backlog: publish/autosave race, /admin/config truthfulness, OIDC proxy, AI fail-closed - #294

Merged
richardmhope merged 1 commit into
mainfrom
claude/backlog-task-selection-svmnci
Aug 2, 2026
Merged

Backlog: publish/autosave race, /admin/config truthfulness, OIDC proxy, AI fail-closed#294
richardmhope merged 1 commit into
mainfrom
claude/backlog-task-selection-svmnci

Conversation

@richardmhope

@richardmhope richardmhope commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

Five more issues from the Fable review backlog, weighted toward the core invariants: one
that can permanently destroy an analyst's work, two on a page whose entire job is telling
operators the truth, one that breaks a documented invariant, and one fail-closed gap.

Rebased. This PR originally carried a second commit with the four P1 findings
(#268/#269/#270/#271) plus #276. That work was merged to main separately as #293, so the
branch has been rebased onto the new main and now contains only the five issues below.
The remaining diff is byte-identical to what it was before the rebase.

Closes #273
Closes #274
Closes #275
Closes #277
Closes #278

Changes

  • Report publish/transition races the 1.2s autosave debounce, can freeze stale content into the immutable snapshot #278 (bug) — the lifecycle transition buttons were plain form POSTs that navigated
    immediately, dropping a pending 1.2 s autosave debounce. Type a fix to a key judgement,
    click "Publish & disseminate" within ~1.2 s, and the PublicationSnapshot freezes the
    pre-edit text — permanently, because published output is immutable and report_save's
    not-published guard blocks any correction. The race predates UX review P1+P2: cycle-ordered nav, one save model, feed delivery reasons #267, but removing the manual
    "Save draft" button removed the flush users had. Transitions now await saveNow() before
    submitting (the pattern the AI panel already used) and refuse to move the report forward if
    that flush fails, naming a save conflict specifically. Skipped when canEdit is false, so a
    reviewer approving someone else's report doesn't attempt an author-only save.
  • /admin/config leaks credential-bearing URLs (inline proxy creds, Slack webhook URL) despite 'never their value' promise #273 (security)/admin/config promises "secrets are shown only as a set / not-set
    status — never their value", but inline proxy userinfo (http://user:pass@proxy:3128, the
    standard form, which services/proxy.py passes through untouched) and Slack incoming-webhook
    URLs (bearer-equivalent by design) rendered in full — twice each, since data-search carries
    a second copy of every value. Userinfo is now scrubbed from every value and the webhook
    URL reduced to its origin. The root cause was allow-by-default, so that is inverted too: a
    URL/DSN-shaped field renders in full only once listed in PLAINTEXT_URL_FIELDS, and the
    guard test — extended over the DB settings rows, which previously had no equivalent — fails
    on any field in none of the three classification sets. A future sentry_dsn is redacted
    and flagged.
  • Effective-config MISP/Webhook/SSO tiles still have the #264 stored-vs-resolved bug #274 (bug) — the MISP and Webhook capability tiles hard-coded ok: True off the stored
    enabled flag, so an enabled MISP with no URL (or no env API key, which is read only at send
    time) showed a green pill on the page an operator opens to debug, while the /admin hub
    correctly said NOT CONFIGURED. Both now use the hub's off → not-configured → enabled
    tri-state. The SSO readiness check gained the missing provider-locator test via a new
    oidc_settings.validate_provider mirroring ai_settings.validate_selection: Auth0 with a
    client id and secret but no domain yields https:///.well-known/… and fails at the first
    login, so it can no longer read green on either surface.
  • OIDC outbound HTTP (discovery/JWKS/token) bypasses the global proxy #277 (bug) — OIDC discovery, JWKS and the token exchange bypassed the global proxy,
    contradicting the documented "every outbound HTTP call honours the global proxy" invariant.
    In the egress-restricted deployment that feature exists for, SSO simply timed out.
    proxy.resolve is now applied per provider at registration and threaded through Authlib's
    client_kwargs, which is exactly where httpx takes proxy/trust_env. The registry cache
    is versioned on the proxy row as well as the OIDC row, so an /admin/proxy change reaches
    SSO without an SSO edit.
  • AI resolve() overlays max_tlp/timeout unvalidated -> 500 instead of failing closed #275 (bug)resolve() overlays the settings row with model_copy(update=...), which
    skips pydantic validation, but validate_selection never checked max_tlp or timeout. A
    row carrying max_tlp="PURPLE" therefore stayed "valid", and TLP(settings.ai_max_tlp)
    raised inside should_send_report — outside every fail-soft try — 500ing every AI
    endpoint. Both fields are validated now, so a bad row disables the backend, which is what
    the resolver's docstring already promised.

Testing

  • uv run pytest passes — 665 passed, 16 skipped (re-run after the rebase)
  • Static gates pass — ruff, bandit, mypy, vulture, djlint all clean
  • Added/updated tests (a bug fix includes a regression test)

New coverage: the transition flush ordering and the 409-on-stale contract; proxy-password and
Slack-path redaction end to end through the template including the data-search attribute,
the deny-by-default URL classifier, and the extended URL-shaped audit over both Settings and
the DB rows; MISP/Webhook/SSO tile tri-states; OIDC proxy kwargs asserted through Authlib's
own extract_client_kwargs (with the NO_PROXY bypass, SYSTEM mode, and cache invalidation on
a proxy change); and bogus max_tlp/timeout resolving to a disabled backend with a
fail-soft assist response.

Two gates could not be run in the dev sandbox and rely on CI: biome lint (standalone
binary, not installed there) and the assets drift job (scripts/vendor_assets.py needs
network). Only design-system CSS classes were added — no new Tailwind utilities — so asset
drift is not expected.

One pre-existing test had its premise changed, not just its expectation, so it is called
out rather than buried in the diff: test_sso_tile_warns_when_a_provider_has_no_client_secret
asserted a green pill for Entra configured with a client id and secret but no tenant id.
That passed only because the locator went unchecked — it was asserting the #274 bug. The
fixture now sets a tenant id so the green case is genuinely usable, and the missing-locator
state gets its own test.

Checklist


Note for whoever merges: #293 landed the first batch on main but did not close its
issues — #268, #269, #270, #271 and #276 are still open despite being fixed on main.
They can be closed as completed independently of this PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LQpYwRbt1YhpTXPb46MR5v

@richardmhope
richardmhope requested a review from a team as a code owner August 2, 2026 20:22
…y, AI fail-closed

Five more from the Fable review backlog, weighted toward the core
invariants: one that can destroy an analyst's work, two on a page whose
whole job is telling operators the truth, one that breaks a documented
invariant, and one fail-closed gap.

**#278 — transition races the autosave debounce.** The lifecycle buttons
were plain form POSTs that navigated immediately, dropping a pending 1.2s
debounce. Type a fix to a key judgement, click "Publish & disseminate"
within ~1.2s, and the snapshot freezes the PRE-EDIT text — permanently,
because published output is immutable and `report_save`'s not-published
guard blocks any correction. The race predates #267, but removing the
manual "Save draft" button removed the flush users had. Transitions now
`await saveNow()` before submitting (the pattern the AI panel already
used) and refuse to move the report forward if that flush fails, naming a
conflict specifically. Skipped when `canEdit` is false, so a reviewer
approving someone else's report doesn't attempt an author-only save.

**#273 — /admin/config printed credentials carried inside a URL
(security).** The page promises "secrets are shown only as set/not-set —
never their value", but inline proxy userinfo (`http://user:pass@proxy`,
the standard form, which `services/proxy.py` passes through untouched) and
Slack incoming-webhook URLs (bearer-equivalent by design) rendered in
full — twice each, since `data-search` carries a second copy. Userinfo is
now scrubbed from *every* value; the webhook URL is reduced to its origin.
Root cause was allow-by-default, so that is inverted too: a URL/DSN-shaped
field renders in full only once listed in `PLAINTEXT_URL_FIELDS`, and a
guard test — extended over the DB settings rows, which previously had no
equivalent — fails on any that is in none of the three sets. A future
`sentry_dsn` is redacted *and* flagged.

**#274 — /admin/config tiles reported stored flags, not resolved state.**
MISP and Webhook hard-coded `ok: True` off the stored `enabled` flag, so
an enabled MISP with no URL (or no env API key, read only at send time)
showed green on the page an operator opens *to debug* while the /admin hub
correctly said NOT CONFIGURED. Both now use the hub's off →
not-configured → enabled tri-state. The SSO readiness check gained the
missing provider-locator test via a new `oidc_settings.validate_provider`
mirroring `ai_settings.validate_selection`: Auth0 with a client id and
secret but no domain yields `https:///.well-known/…` and fails at the
first login, so it can no longer read green on either surface.

**#277 — OIDC egress bypassed the global proxy.** Discovery, JWKS and the
token exchange went direct, contradicting "every outbound HTTP call
honours the global proxy" — in the egress-restricted deployment that
feature exists for, SSO just timed out. `proxy.resolve` is now applied per
provider at registration and threaded through Authlib's `client_kwargs`,
which is exactly where httpx takes `proxy`/`trust_env`. The registry cache
is versioned on the proxy row as well as the OIDC row, so a /admin/proxy
change reaches SSO without an SSO edit.

**#275 — AI resolve() 500'd instead of failing closed.** `resolve` overlays
the row with `model_copy(update=...)`, which skips pydantic validation, but
`validate_selection` never checked `max_tlp` or `timeout`. A row carrying
`max_tlp="PURPLE"` therefore stayed "valid", and `TLP(settings.ai_max_tlp)`
raised inside `should_send_report` — outside every fail-soft try — 500ing
every AI endpoint. Both fields are validated now, so a bad row disables the
backend, which is what the resolver's docstring promises.

Tests: the 409-on-stale contract for transitions plus the client-side flush
ordering; proxy-password and Slack-path redaction end to end through the
template including `data-search`, the deny-by-default classifier, and the
extended URL-shaped audit; MISP/Webhook/SSO tile tri-states; OIDC proxy
kwargs asserted through Authlib's own `extract_client_kwargs` (with the
NO_PROXY bypass, SYSTEM mode, and cache invalidation on a proxy change);
and bogus `max_tlp`/`timeout` resolving to a disabled backend with a
fail-soft assist response.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LQpYwRbt1YhpTXPb46MR5v
@richardmhope
richardmhope force-pushed the claude/backlog-task-selection-svmnci branch from 930c6f0 to 1d48dbc Compare August 2, 2026 20:36
@richardmhope richardmhope changed the title Backlog: close ten Fable-review findings (4 P1 + 6 medium) Backlog: publish/autosave race, /admin/config truthfulness, OIDC proxy, AI fail-closed Aug 2, 2026
@richardmhope
richardmhope merged commit 8e90a07 into main Aug 2, 2026
7 checks passed
@richardmhope
richardmhope deleted the claude/backlog-task-selection-svmnci branch August 2, 2026 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment