Repository navigation
Security review fixes, signing-key revocation, auth_time, GHCR retention - #261
Merged
Merged
Conversation
With MODGUD_TEST_POSTGRES_CONNECTION_STRING set, the shared fixture runs against a fresh, exclusively created modgud_review_* database on 127.0.0.1:5433 (user modgud_dev) instead of starting a Testcontainers instance. Any other host, port, user or an existing database (or derived realm database) is refused before a host starts. Without the variable nothing changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The deletion APIs are deliberately reachable below the portal's MFA minimum, but the
profile page's background reads (profile request status, external links, header
inbox) got an area-MFA 403 and the shared HTTP client navigated away to step-up,
taking the allowed deletion page with it.
- HttpClient.get accepts { redirectOnStepUp: false } per request; the request still
rejects, and 401 and fresh-account-proof redirects are unchanged.
- useEntityService gets redirectOnLoadStepUp; the inbox store opts out.
- The profile view loads its optional data that way; explicit actions keep the default.
- Node unit tests for the HTTP client, an isolated Playwright config/spec that needs no
backend, middleware whitelist and sign-in scenario coverage.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e only Creating a service account with an initial credential enriched the same DTO that was then dispatched to read subscribers and the cluster relay, so the one-time secret could reach anyone subscribed with service-account:read. The create endpoint now dispatches an explicitly projected, secret-free DTO and returns a separate response object carrying the credential. The frontend keeps the one-time credential in the creator's response, never in the shared entity store. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The erased/masked marker was stored before event masking, archiving and audit cleanup ran separately. A failure after that left the erasure looking done, so the normal pending retries no longer picked it up. An erasure is now claimed first (start time, original actor and reason, the live and historically referenced link ids) and IsDataMasked is set only by the final commit. If a step fails, the claim keeps its inventory and attribution, and a fresh retry resumes it. UserDeletionState uses optimistic concurrency, so a stale sweep or cancel cannot overwrite the winning decision. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…cursor reset The resource-server SDK kept its session denylist in memory and, after a restart or a feed reset, took over only the cursor; sessions ended before that were accepted again. Ending a session now also writes a small SessionRevocation record in the same Marten session (including the orphan sweep). The change feed serves this bounded recovery history, and the SDK rebuilds its denylist from it on start and after a reset. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A DPoP-bound management token sent with the DPoP scheme skipped the local proof-of-possession validation, and the management guard did not compare a proof either: a stolen bound token was usable without its private key. Only on endpoints marked RequiresManagementPermission, an OpenIddict validation handler extracts the DPoP scheme; after token validation and the live permission checks, a proof validator checks the proof against the token binding before the consumer runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… stale list On reconnect useEntityService fired `void loadAll()`; a refused reload became an unhandled rejection and the old snapshot stayed marked as loaded. A failed full read now clears the list, resets allLoaded and sets loadFailed; the reconnect callback handles the rejection. A revision guard keeps an older read from overwriting a newer snapshot or FullSync. Inbox and the user, service-account and position lists show a generic warning until a full read succeeds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Pin the packages flagged by the NuGet audit: MessagePack 2.5.303 and System.Security.Cryptography.Pkcs, System.Security.Cryptography.Xml and Microsoft.AspNetCore.SignalR.Common 10.0.13, referenced directly where the transitive graph needs them (no global transitive pinning). Compatibility tests cover the patched assemblies and the data-protection keyring persistence. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The ACS read the SAML response without enabling XML signature validation and consumed the pending AuthnRequest first. SamlLoginFlow now unbinds with Saml2PostBinding.Unbind, which validates against the configured certificates and algorithms and requires at least one valid response or assertion signature, before the request is consumed. SamlAcsReplayEndToEndTests cover the signature and replay counter-cases. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An opt-in startup filter (enabled by a directory variable) correlates a failing test request with the server-side exception type without recording request values, messages or bodies. Diagnostic tests for the sporadic API 500 hypotheses (OTP failure paths and overlapping rate-limit SQL) stay as regression probes; the original cause stays open. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The applications, OAuth client, OAuth scope and invite-code stores bypass useEntityService and had the same unhandled reconnect rejection. Each now tracks loadFailed, invalidates its list on a failed full read, ignores stale completions and shows the generic warning in its list views. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… catalog The invite-code endpoints require invite-code:read and invite-code:write, but the system app's permission catalog had neither, so only realm admins (via the bypass) could use them and ordinary roles could not be granted them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
EnrichWithHttpRequest runs before the application middleware, so reading TenantContext.Current there threw before RealmMiddleware had opened the scope. The realm tag is now set once the realm is resolved. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Document the development preflight: use only the existing postgres-dev on 127.0.0.1:5433 with a private, work-owned database and an unused loopback API port, verify the resolved startup configuration first, and never reset or reuse the shared modgud/postgres services or their connection strings. The first admin is created with the recovery CLI. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Router and main layout used their own, incomplete permission lists: users with only service-account, position, invite-code or scheduled-job read rights saw the sidebar item but the router refused them, and /admin always redirected to /admin/users. A shared admin-navigation definition now drives the sidebar, the router guard, the top-level entry and a permission-aware /admin landing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The login page handles MFA failures locally (retry, switch method, back to login), but the shared HTTP client also redirected every 401 to bare /login, dropping the MFA state and the pending OAuth/profile/admin continuation. A 401 from exactly the TOTP login, email-MFA login and email-MFA code request POSTs now stays local; every other 401 keeps the normal redirect. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
After the target policy was raised to multi-factor, a refresh family for a user without a second factor evaluated as satisfied/setup-pending without a deadline and kept getting tokens. The refresh now refuses that case instead of starting or bypassing the setup grace; a grace that has already started stays in effect, but a refresh does not extend it and is refused once it has ended. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…st is under Confirming an account change with a passkey used the realm's primary domain as RP even when the user's own credential belongs to an app RP that the profile host sits under, so the confirmation could never succeed. The server now selects, for the account owner only, an eligible RP of their own credentials for this host (realm first, then the most specific), pins it in the ceremony and records a fresh passkey proof. There is still no free, client-chosen RP. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LiveFixBrowserTests runs the built SPA in Chromium against a real loopback Kestrel host, cookie authentication and SignalR on a fresh postgres-dev database. The specs cover the service-account secret disclosure, the admin entry without grants, inbox reconnect with profile deletion, the MFA retry continuation and the standalone store reloads. It only runs when explicitly enabled; see the frontend README. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Pins the investigation result: after a PrimaryDomain switch the passkey proof and RP pinning are intact; which of an old domain cookie and a new host cookie wins is a transport matter. HTTP cases control the cookie order explicitly, and an isolated Chromium probe records the order the browser actually sends. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ial id once Two overlapping finishes of one passkey enrollment ceremony both deleted it (Marten does not version-check deletes) and each stored a credential. With the same key that left two entries: deleting one in the account page still let the key sign in. Two separate ceremonies of the same key (two tabs) could also both pass the read-then-insert uniqueness check. - Web register and native enroll consume the ceremony with a version-checked ConsumedAt store (optimistic concurrency on PasskeyEnrollCeremony); the loser gets a 400. Both finishes refuse a consumed ceremony, which now stays until expiry. - Unique index on the stored credential id per realm database. Marten's own (Unique)Index casts a byte[] member to jsonb and would fail every insert with 22P02, so it is a raw index on (data ->> 'CredentialId'); a violation of exactly that index answers 400. - Web register fails closed like native enroll: a refused attestation (wrong origin/challenge, forged, already registered) or a malformed body is a 400, not a 500. Native enroll also rejects a malformed body with a 400. Verified against a fresh database: concurrency tests hold both requests at the consume or the insert; the new cases fail without the fix and pass with it. A read-only check of production found no duplicate credential ids. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The request-log enricher copied the whole query string into the event, and the Serilog OTLP sink exports every property as a log attribute. Magic-login, reset-password, verify-email, bootstrap and install links carry one-time secrets in the query, and the collector's redaction cannot recognise an opaque token. - The QueryString property keeps parameter names and replaces every value with [redacted]; a segment without '=' or with an unusual name is replaced whole. - The request log records the user id instead of the user name, which the collector's user-name rules did not cover in this event. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The approval POST to /connect/verify binds the signed-in session to a device code by user_code alone, and the CSRF defence only covers /api/* (where it also lets same-site requests through). A page on a sibling subdomain gets the session cookie sent along, SameSite or not, and could approve the attacker's own device code. The POST now requires Sec-Fetch-Site: same-origin, or, without it, an Origin (or Referer) that is exactly this request's origin. A caller without browser fetch metadata is unchanged; it carries no victim's ambient cookie. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…l identity Finishing an external sign-in while signed in links the external identity to the current account. That path persisted the link without any account proof or notice, so a taken-over session alone could bind the attacker's own IdP identity to the victim's account: a sign-in path that outlives the session, and, with an MFA amr from the IdP, a fresh external factor for account changes. Adding a link is now an account change like adding a passkey. The OIDC finish and SAML ACS callers ask the account-change guard about the signed-in session; the processor refuses a new link without that verdict (fail-closed default) with Idp.LinkRequiresRecentProof, and an allowed link mails the account-change notice. Signing in through an already linked identity is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… defaults A self-registration joined its configured default groups directly, without the guard that protects every other membership change of a group that confers realm:admin. A delegated realm-settings writer without realm:admin could name such a group as a default, register a fresh account and hold realm:admin through it. The pipeline now skips (and logs) any default group from which realm:admin is reachable, directly or through an ancestor, whoever configured the defaults (realm or app settings). Ordinary default groups still apply. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The native urn:cocoar:magic grant checked IsConsumed but then consumed the challenge with a Delete, which Marten does not version-check. Two overlapping redemptions of one link both deleted it and each minted tokens. It now consumes with a version-checked ConsumedAt store like the web redemption; the loser gets invalid_grant. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The login throttle only peeked at the failure bucket before the password check and charged a failure afterwards. Overlapping requests all read "free": 30 simultaneous wrong passwords were all checked against a limit of five. The check now takes the slot atomically (HitAsync). A wrong password keeps it; a success, a second-factor step or an administrative lock gives it back through the new IRateLimitStore.ReleaseAsync (fixed window: one hit less in the same window; token bucket: one token more, capped). Recording a failure no longer charges the bucket a second time. Still only failures count, now also under concurrency. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… is applied FD-01: the ExpectedVersion check compared against the loaded document only and RealmDraft had no optimistic concurrency, so two overlapping saves of one version both passed and the last one silently won. RealmDraft now uses Marten's version check; a lost race on any draft mutation is a 409 Draft.ConcurrentChange. FD-02: an apply deleted the consumed draft after the live commit without any version check, so an edit saved meanwhile vanished unapplied. The apply now claims the draft (ApplyingSince, version-checked) before the live write; edits, staging, rebase and delete are refused with 409 Draft.Applying while it runs. A failed apply releases the claim at once; an abandoned claim expires after ten minutes. Once the live state is committed the caller always gets the result, one-time secrets included; a failed cleanup is logged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With MODGUD_TEST_POSTGRES_CONNECTION_STRING set (the opt-in the shared fixture already has), the cold-start fixture starts no Testcontainers instance. Every host (shared, isolated, uninitialized) boots against a new database on 127.0.0.1:5433 whose name is derived from the supplied modgud_review_* name and verified not to exist yet, together with every database derived from it; the host creates it itself, as in a real cold boot. The derived names never start with "<name>_", so the shared fixture's freshness check stays intact in a mixed run. The databases are left on postgres-dev for diagnosis; only a container the fixture started is removed. Without the variable nothing changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Local integration runs leave one or more modgud_review_* databases per run on the dedicated postgres-dev container (the fixtures keep them for diagnosis). The script lists them and drops them with --apply; dry run by default. It refuses unless the container is postgres-dev, running, published exclusively on 127.0.0.1:5433 and on the modgud-postgres-dev-data volume; it only touches ^modgud_review_[a-z0-9_-]+$ names and skips databases with open connections instead of disconnecting them. Kept out of scripts/, whose scripts never write to the database directly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The discovery document added registration_endpoint and client_id_metadata_document_supported from the realm switches only, while /connect/register and the CIMD resolver decide by the effective (App ⊕ realm) setting of the App whose domain the request came in on. Realm off / App on hid a working mechanism; realm on / App off advertised one that is then refused. Both discovery handlers now read the App's Enabled override for the request's Application domain and fall back to the realm switch otherwise. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
prompt=login forced the login challenge, but the return URL repeated the unchanged query, so the re-entry after a successful sign-in challenged again and a relying party using prompt=login never received a code. The first pass now appends a data-protected, time-limited marker (the time of the prompt) to the return URL. The re-entry is satisfied when the session proved a sign-in factor at or after that time; the session that existed before the prompt is not, so merely visiting the login page does not satisfy it. Nothing in the original request is rewritten, so a pushed authorization request works the same. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The pending AuthnRequest was single-use but not bound to a browser. An attacker could start a login in their own browser, obtain the IdP's signed Response for their own identity and submit it from the victim's browser within the 15-minute window: login CSRF / session swapping, the victim working in the attacker's account. The login start now sets a Modgud.Saml.Binding cookie (random value, reused when the browser already holds one so several tabs work; HttpOnly, SameSite=None, Secure on HTTPS, path /saml/, 15 minutes) and the pending request records its SHA-256. After the replay/expiry/provider checks the ACS compares it in constant time and refuses a mismatch as saml-browser-mismatch without consuming the request. Requests recorded before this change carry no hash and are still accepted until they expire. Because the ACS POST is cross-site, SAML sign-in now requires HTTPS; the SAML admin docs say so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
For an IdP whose metadata offers only HTTP-POST single sign-on, the metadata parser fell back to that POST URL, but the login always used the redirect binding: the AuthnRequest reached a POST-only endpoint as a GET query, which such an IdP refuses. When the IdP offers no redirect endpoint, the login now answers with ITfoxtec's auto-submitting POST form, under its own narrow CSP (nothing loads, the inline submit may run, form-action only to the IdP's SSO origin, no framing) and no-store. HTTP-Redirect stays the default whenever the IdP offers it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two first uses of a realm's SAML SP certificate (metadata, a signed AuthnRequest) could each generate a key pair and Store it, last write wins, and each hand out its own: the loser's certificate could already sit in published SP metadata or have signed a request the realm then no longer backs. The first-use write is now an Insert. When a concurrent first use stored its certificate first, the save fails, the pending changes are dropped and the stored winner (read through a fresh query session) is returned. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The "IdP Entity ID" field ("set explicitly only if the metadata omits it or you
need to override") was persisted but never read: metadata without an entityID was
rejected outright, and the accepted issuer always came from the metadata.
The override now applies when the metadata is parsed (fetched by URL or pasted
XML, at registration and on refresh), so it is the IdP entity ID everywhere,
including the accepted issuer; without it the metadata's entityID is used as
before.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
RefreshMetadataAsync read the cached provider entry, awaited the metadata fetch and then wrote its updated copy back unconditionally. A provider disabled, deleted or re-registered while the fetch ran was resurrected with its old configuration and certificates until the next registration event. The refresh now writes back with a compare-and-swap against the entry it read (ConcurrentDictionary.TryUpdate); if the registration changed meanwhile, the refreshed copy is discarded and the newer state stays. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…th its configuration The readiness gate ran only on the disabled → enabled transition, so an update of an already active OIDC provider could persist an unusable configuration (an empty ClientId) while staying enabled. The scheme manager then returned before touching the existing registration, so the OLD options kept answering logins, the materializer counted the new state as processed, and after a restart the same document produced no registration at all. An update that would make a usable, enabled provider unusable is now refused with the same readiness errors as enabling; an already incomplete provider stays editable. And a configuration the manager cannot register (unknown flavor, endpoint derivation failure, empty ClientId) now drops the previous registration. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Login provider writes had no conflict protection. An update appended a full-state event without an expected stream version, so an update that loaded before another committed could reset it (trust and policy fields included); overlapping appends surfaced as an unhandled EventStreamUnexpectedMaxEventIdException (a 500). Create and the realm seeder checked slug and display name by query only, so two overlapping creates both passed. Active (not deleted) providers now have unique indexes on slug and display name. The update appends its events at exactly the stream version it loaded. Index and version conflicts come back as SlugTaken, DisplayNameTaken or the new ConcurrentChange conflict instead of an unhandled error, and the seeder treats a concurrent seeding of the Internal provider as done. Production was checked read-only for existing duplicates first (none). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Provider IDs are only unique inside a realm's database (a manifest import
may pin one), but OIDC schemes and the SAML provider cache are process-wide
and were keyed by the ID alone: one realm registering, disabling or deleting
a provider with the same ID replaced or removed another realm's
registration. Scheme names are now Oidc_{realm}_{id}, the SAML cache is
keyed by (realm, id), and unregister, lookup and metadata refresh take the
realm.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
RawClaimsRetentionDays was offered, stored and projected but read by nothing: the last raw-claims snapshot was delivered and kept on the link, and in every login event of its stream, however old it was. Expired snapshots are now withheld by the admin views and the script editor sample, and a new realm job (raw-claims-retention) clears the link snapshot through an expiry event and nulls only the RawClaims field of expired login events. Email, display name and script output stay. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
EventSourcedUserStore.UpdateAsync stored the whole ApplicationUser it was given and patched password hash, security stamp and factor fields from it. An update that had loaded the user before a deactivation, password change or factor change put the earlier state back when it saved. The store now keeps a snapshot of each user object it hands out and writes only the fields that changed since: document fields as patches, security fields only when they changed. Users it did not load keep the former whole write. SetSecurityStampAsync no longer stores the whole object. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Since the login throttle reserves its slot before the password is checked (B8-AJ-01), a parallel burst gets only as many attempts to the password check as the untrusted pool allows; the rest are refused without a password verdict. The lockout concurrency test still expected every attempt in the failure counter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
BoundTo decides in which apps a group's roles take effect. Binding a dormant group that holds a realm:admin role to Modgud made its existing members realm administrators, and unbinding it took that away, but the conferral guard only covered newly added roles and membership changes. A delegated group writer could flip realm:admin for a group's members, their own included, through BoundTo alone. A BoundTo change on a group that confers realm:admin (directly or through an ancestor) now needs a realm administrator; anyone else gets 403 Group.RealmAdminActivationForbidden. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The "only a realm:admin may confer realm:admin" rule lived in the group commands, but three user routes changed group membership with user:write alone: adding a user to a group, removing them, and creating a user with initial GroupIds. A delegated user administrator could put themselves or a freshly created account into an admin group, or remove administrators from it. The routes now answer 403 Group.RealmAdminMembershipForbidden for a caller who is not a realm administrator when a target group confers realm:admin directly or through an ancestor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…figuration Up to JsEval 4.1 every JsEngine was built from, and first modified, the one Jint Options held by the singleton JsEngineOptions. Engines built at the same time could corrupt its constraint list, after which every new engine in the process failed with a NullReferenceException in Jint.Engine.ResetConstraints, and every script-backed endpoint answered 500 until restart. The execution timeout and statement limit were also one instance for all engines, so any evaluation restarted them for every running script. JsEval 5.0.1 gives each engine its own Jint Options. The script-engine registration moves into AddModgudScriptEngine so a unit test pins both behaviours on Modgud's own configuration. Jint 4.15.3 (JsEval 5.0) turns CLR arrays into read-only live views; the documented script operations keep working, only Array.isArray changes, which the script docs now mention. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A rotation only retired the previous signing key: it stayed in the verification set and the JWKS for the 30-day overlap, and nothing could take a key out of trust earlier, yet the docs advised rotating on a suspected compromise. Whoever held the old private key could keep signing accepted tokens for 30 days. A rotation can now revoke the previous key (?revokePrevious=true, `recover rotate-signing-key --revoke-previous`), and every retired key still in its overlap can be revoked (POST signing-keys/revoke-retired, `recover revoke-retired-signing-keys`). A revoked key leaves the verification set and the JWKS at once, every token it signed stops working, each revocation is a required security-log event, and the janitor deletes revoked keys on its next run. The Signing keys tab offers both as confirmed danger actions; the docs describe the compromise path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The observability activity and error feeds checked observability:read once, at subscribe time, and then pushed every later item until the client disconnected: a user whose permission was withdrawn while the session stayed alive kept receiving activity and errors. These feeds are high-rate and fed from synchronous buffer handlers, so instead of a check per item the gate re-checks every 15 seconds; once the permission is gone (or the lookup fails) it stops forwarding, detaches from the buffer and completes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
User ID tokens carried no auth_time; only staffing tokens set it. OpenID Connect Core makes it REQUIRED when the RP sent max_age, and without it an RP cannot check for itself that prompt=login led to a fresh sign-in. The claim is now set where acr/amr are, from the latest proven sign-in factor, for ID and access tokens. The factors ride codes and refresh tokens, so a refresh keeps the original time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The staffing-sweep job is registered per realm and counted in the scheduled-jobs overview, but had no section of its own, and the positions page called it a system job. Add the section (default cron, parameters, what it does, failure behaviour) and fix the positions page to call it a realm job with a link to it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GHCR lists every pushed manifest as a package version. A release is a multi-arch image index whose per-platform images and attestation manifests are untagged versions, and whose cosign signatures are versions tagged sha256-<digest>.sig. The retention job pruned untagged versions after 7 days and every other tag after 30, so each retained tag (0.20.1, latest, the betas) still existed but pointed at deleted manifests: pulling it fails with "manifest unknown". The retention decision now lives in .github/scripts/ghcr-retention.cjs: a kept image protects the manifests its index lists and the signatures and referrer tags that name them; the three newest images are always kept; a kept image whose manifest cannot be read stops the run before anything is deleted. The script is unit-tested in a new CI job. Images already pruned cannot be restored; the next release publishes a complete one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The CI npm audit now fails on advisories published after the last develop build: vite <=8.1.5 (launch-editor command handling, server.fs.allow bypass; dev server only) and dompurify <3.4.17 via monaco-editor (SAFE_FOR_TEMPLATES and IN_PLACE bypasses). Raise vite to ^8.1.6 (resolves 8.3.6) and the dompurify override to >=3.4.17. pnpm audit --prod is clean; type-check, build, unit and isolated browser tests pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DependencyPatchCompatibilityTests required exactly 10.0.13 for the Pkcs/Xml/SignalR assemblies; CI installs the latest 10.0.x runtime (10.0.14), whose shared-framework assembly wins, so the check failed although a patched assembly was loaded. It now requires at least the patched version. The browser and live specs served dist files with statSync then readFileSync, which CodeQL flags as a file-system race (js/file-system-race). They now read once and fall back on the error, as before for a missing file or a directory. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
HasOnlyEncryptedAssertions was inserted between CheckRequiredSignatures and its doc comment, so it carried two summaries and the check none. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The TestServer client receives the response before the hosting layer stops the request activity, and stopping the host does not wait for it. RealmTraceEnrichmentTests asserted right after reading the response, so the first (cold) case could find no completed span yet; it failed once in CI. The tests now wait up to 5 s for the expected spans. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merged
15 of 18 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.
Motivation
Fixes from a security and correctness review of the whole IdP: 70 focused commits, each with a regression test written red first, plus one CI fix found while preparing this release.
GHCR retention broke every published image. A release is a multi-arch index whose platform images and attestations are untagged GHCR versions.
cd-ghcr-retention.ymlpruned untagged versions after 7 days, so every tag (0.20.1,latest, the betas) still exists but answersmanifest unknownon pull. The retention now keeps the manifests and cosign signatures of every kept image, always keeps the three newest images, and fails closed when it cannot read a kept image's manifest. A dry run against the live package deletes nothing. Images already pruned cannot be restored; the next release publishes a complete one.Highlights of the review fixes:
?revokePrevious=true,POST signing-keys/revoke-retired, recovery CLI); previously a rotated key stayed trusted for the 30-day overlap.auth_timein user ID tokens; scopes revalidated on code/refresh/device exchange; grants revoked when a client is deleted or disabled;prompt=logincompletes after a fresh sign-in; DPoP proof required for DPoP-bound management tokens; device approval only from the IdP origin; magic link redeemed once.realm:admin; one permission list for sidebar, router and admin entry.clientDataJSON; login attempts reserved before the password check.Surface touched
Modgud.Authentication/)Modgud.Authorization/)/connect/*, OAuth aggregates)Modgud.Api/Features/Admin/)src/frontend-vue/)Modgud.Authentication/Cli/)docs/)Tests
.github/scriptsretention tests 7/7, now a CI job)Rolling update
ExternalIdentityRawClaimsExpiredEventwithout an upcaster (read byExternalIdentityLinkProjection); new unique index on passkey credential ids per realm database (it fails to build if a realm already holds a duplicate credential id); optimistic concurrency on passkey ceremony and account-deletion documents.dotnet testis green locally — the full suite was not run in one go locally; CI runs it.Docs
docs/updated where user-visible behavior changeddocs/roadmap.mdupdated if a roadmap item movedBreaking changes
invalid_grantwhen the MFA setup grace has not started and the target now requiresmulti.Notes
IRealmKeyStoregainedRotateAsync(slug, revokePrevious, ct)andRevokeRetiredKeysAsync. Revocation reaches other API instances within ~60 s, and resource servers trust a revoked key until their own JWKS refresh.🤖 Generated with Claude Code
This pull request was created by CodeAlta, an AI coding agent.