Fix/audit remediation critical - #124
Merged
Merged
Conversation
#102 — Workspace admins held user.create/update/delete, but those endpoints mutate the global BackendUser. A workspace admin could replace the credentials of, erase, or export a member who also belongs to other workspaces — a cross-tenant account takeover. Global identity administration now requires platform scope; workspace administration moves to a dedicated membership.* permission set whose endpoints confine a workspace-bound caller to its own workspace. #103 — ApiKeyAuthenticationHandler accepted any non-empty key as platform super-admin whenever RequireApiKeyAuthentication was false. Credential validity now depends on the presented key alone; RequireApiKeyAuthentication is a startup policy switch only. Bootstrap keys additionally honour an optional expiry so the break-glass credential can retire itself, and the constant-time comparison runs over fixed-length digests so key length no longer leaks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SZ7bVnUeBsMfELhMDFizEo
#106 — The rate limiter read the raw first X-Forwarded-For value, so rotating the header handed out a fresh login bucket per request. It now partitions on Connection.RemoteIpAddress only. X-Forwarded-For rewrites that address solely through UseForwardedHeaders, and only once KnownProxies/KnownNetworks name a trusted peer — with empty trust lists ASP.NET would accept the header from any client, so the header stays unprocessed and startup logs why. #107 — CallBusinessEvent serialises the remote telephone number as remoteParty, which neither the Communication manifest nor the core baseline declared, so webhooks shipped it in the clear at IncludeSensitiveData=false. The manifest now declares the field, and a test dispatches a real CallBusinessEvent through the production dispatcher and minimizer. The manifest documentation showed field names the code never emits; it now mirrors the shipped file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SZ7bVnUeBsMfELhMDFizEo
#109 — The admin extension proxy checked plugin availability only against the token-bound workspace. A platform operator has none and selects the target with ?workspaceKey=, which reached Communication's scope resolution ungated: the plugin could be operated for a workspace where it is unentitled, inactive or unhealthy. The host now resolves the effective workspace itself — bound workspace first, a query selection only for platform operators — and gates availability against it, matching the MCP path. Routes declare their scope: HostAdminApiRouteScope defaults to Workspace, so a route without a resolvable workspace is rejected with 400 rather than passing through, and a genuinely global route (plugin status) opts out explicitly. Communication's scope helpers no longer read the query, which is what made the bypass reachable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SZ7bVnUeBsMfELhMDFizEo
#105 — JWTs carried role and permission claims for an hour with no jti, no security stamp and no server-side record, so password changes, role removal, deletion and logout left an issued token fully usable. Every session now carries a jti and the account's security stamp. Rotating the stamp — password change, deactivation, deletion, RBAC assignment or grant change — revokes all of that account's sessions; logout records the jti in a durable revocation list and kills exactly one. The request-path check reads a size-capped 15-second cache that is dropped on rotation, so revocation is immediate rather than eventual. Tokens without a stamp (external OIDC, named integrations) stay with their own issuer. #104 — One BackendPasswordPolicy now governs the bootstrap seed, the demo admin, operator-created accounts and later credential changes; a violating seed is skipped instead of creating a weak super-admin, which is why the default demo password changed. Ten consecutive failures lock an account for fifteen minutes; a success clears the counter. Accounts can be disabled without deletion, which keeps their data and memberships while stopping authentication and live sessions, and both directions are audited. For MFA the host takes the documented external-identity path: RequireExternalIdentityForOperators refuses the local password login for platform operators and fails startup without an OIDC authority, rather than pretending to a second factor Callora does not issue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SZ7bVnUeBsMfELhMDFizEo
#108 (partial) — Media and signalling channels reassembled fragments into an unbounded MemoryStream, so one peer could grow host memory simply by never setting EndOfMessage. Reassembly now runs through a shared reader with a hard byte cap and an idle timeout: an oversized message closes the socket with MessageTooBig instead of allocating, and an abandoned socket is torn down. Audio payloads are checked before decoding and must match the negotiated frame size exactly — AudioFormat now derives BytesPerFrame, which turns the negotiated format into an enforceable constraint. The paced sender is bounded by total bytes as well as frame count, because many large frames stayed under a count-only cap. Connect tokens are no longer stored in the clear: the session row keeps a SHA-256 lookup key, so a leaked row hands out no working ticket. Activation additionally rejects future-dated rows, which satisfied the old lower-bound TTL check forever, and an hourly job purges spent and expired sessions. Still open on this issue: per-IP/token connection limits and an explicit origin policy for browser-facing endpoints, both of which belong at the host WebSocket layer rather than in the plugin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SZ7bVnUeBsMfELhMDFizEo
#110 — Enabled accounts were provisioned once at plugin startup and the CRUD handlers only wrote rows. A created or enabled account therefore did not register until the next restart, a disabled or deleted one kept its registration and could still take calls, and credential or endpoint changes never reconnected — while the API reported success either way. ISipAccountRuntimeReconciler is now the single path from a persisted account to a live channel, used by startup and by every mutation, so the two cannot drift. It is state-based and idempotent: an unchanged account is a no-op, a changed configuration fingerprint tears the old registration down before connecting the new one, and a disabled account is deprovisioned. Per-account locking serializes concurrent mutations. A runtime failure is no longer invisible: the account's status is written back as failed with its reason and the caller gets 502 carrying both, so the persisted state and the response agree with what the runtime did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SZ7bVnUeBsMfELhMDFizEo
An earlier commit on this branch swept custom/ wholesale and picked up the archived Communication plugin's dependency tree. The .gitignore pattern only covered custom/static-plugins/*/node_modules, not the archive's nested one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SZ7bVnUeBsMfELhMDFizEo
…nect #111 — The admin UI and API advertised digest, IP-authenticated and mutual-TLS accounts, while SdkSipAccountFactory throws for everything but digest. Such an account was accepted, persisted, then silently skipped at provisioning, leaving the UI on "Connecting" with no explanation. Verified against CalloraVoipSdk 4.7.3 that neither can be implemented here: SipLineChannel has no path around RegisterAsync and clamps the expiry to Math.Max(1, …), so a registration-less trunk is impossible; TlsConfiguration hangs off VoipOptions rather than SipAccount and SipTlsCertificateProvider holds one file-loaded certificate, so per-account client certificates are impossible. Both gaps are now tracked upstream as callora-voip-sdk#104 and #183. SipAuthMethodSupport is the single boundary the UI, the API and the provisioner share. Create and update refuse an unsupported method with 422 and a reason that names the upstream gap; the admin form offers only what the backend accepts; the reconciler fails such an account before touching the connector, so accounts predating the guard surface that reason on startup instead of staying invisible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SZ7bVnUeBsMfELhMDFizEo
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.
1. Why is this change necessary?
The audit tracker #123 blocks any production release on two critical findings, then lists a phase of security-boundary work behind them. Both criticals were exploitable as written.
#102 granted
user.create,user.updateanduser.deleteto the workspaceadminrole inWorkspaceRolePermissions.UserEndpointschecked only that the target user belonged to the caller's workspace, but the operations behind that check mutate the globalBackendUser. A workspace administrator could therefore replace the email, display name and password of any member, erase that member's account together with every workspace membership and global RBAC assignment, and export their memberships across all workspaces. For a user who belongs to two workspaces, that is a cross-tenant account takeover. If a platform operator also holds a workspace membership, it is a privilege escalation.#103 decided bootstrap API-key validity with
EnableBootstrapApiKeys && (!RequireApiKeyAuthentication || IsKnownBootstrapKey(providedKey)). WithRequireApiKeyAuthenticationset tofalse, any non-empty value inX-Callora-Api-Keyauthenticated as platform super-admin. A switch named "require authentication" was deciding whether a presented credential is valid at all.The phase-1 findings share a shape. Each one advertises a guarantee the code does not keep: rate limits partitioned on an attacker-controlled header (#106), a webhook payload carrying a telephone number that no sensitive-field registry knew about (#107), a plugin availability gate that a platform operator walked around with a query parameter (#109), JWTs that outlived password changes and role removals by up to an hour (#105), a password policy that applied only to the bootstrap seed (#104), and WebSocket channels that reassembled fragments into an unbounded buffer (#108).
The two Communication findings are the same problem one layer down. SIP account CRUD wrote rows and reported success while the running voice provider was never reconfigured (#110), and the API offered three SIP authentication methods while the SDK adapter throws for two of them (#111).
2. What does this change do, exactly?
Global identity and workspace membership become separate concerns (#102). Workspace roles keep
user.read, because the read endpoints are already filtered to the caller's workspace, and lose everyuser.*write key. Membership administration moves tomembership.read/update/deleteon/api/workspaces/{key}/members, whereWorkspaceScopeEvaluator.HasWorkspaceAccessconfines a workspace-bound caller to its own workspace and answers404for any other. Credential changes, account erasure and the data-subject export additionally require platform scope, so a stray permission claim on a workspace token is not enough.API-key validity depends on the presented key alone (#103). The bootstrap branch now requires
IsKnownBootstrapKeyunconditionally.RequireApiKeyAuthenticationis documented and implemented as a startup policy switch that refuses to boot with bootstrap keys enabled but unconfigured.BootstrapApiKeysExpireAtUtclets onboarding hand out a break-glass credential that retires itself, and the constant-time comparison runs over SHA-256 digests so key length no longer leaks through the length check.Rate-limit identity comes from the connection, not the header (#106).
BackendRateLimiting.ResolveClientKeyreadsConnection.RemoteIpAddressonly.X-Forwarded-Forreaches that address exclusively throughUseForwardedHeaders, andBackendForwardedHeaders.Buildadds theXForwardedForflag only onceKnownProxiesorKnownNetworksnames a trusted peer. With empty trust lists ASP.NET applies forwarded headers from any peer, so the header stays unprocessed and startup logs why per-client limits degraded to per-proxy.Sessions become revocable (#105). Every issued token carries a
jtiand the account's security stamp.BackendSessionValidatorruns inOnTokenValidatedand compares that stamp against the stored one, so a password change, deactivation, deletion or RBAC change invalidates outstanding tokens instead of waiting out their hour. Logout records thejtiinbackend_revoked_sessions, which is durable on purpose because an in-memory list would resurrect logged-out tokens on restart. Account state is cached for fifteen seconds behind a hard entry cap and dropped bySessionStateInvalidatingUserStorethe moment a stamp rotates, so revocation is immediate rather than eventual. Tokens without a stamp claim (external OIDC, named integrations) belong to their own issuer and are left alone.One password policy, lockout, and deactivation (#104).
BackendPasswordPolicygoverns the bootstrap seed, the demo admin, operator-created accounts and later changes alike. A seed that violates it is skipped with a warning rather than creating the one weak super-admin in the system, which is why the default demo password changed. Ten consecutive failures lock an account for fifteen minutes and a success clears the counter.PUT /api/users/{id}/activationdisables an account without deleting it, keeping its data, memberships and audit trail while stopping authentication and live sessions, and both directions are audited. Callora issues no second factor of its own, soRequireExternalIdentityForOperatorstakes the documented external-identity path instead: it refuses the local password login for platform operators and fails startup without an OIDC authority.Call events stop leaking the remote number (#107). The Communication manifest declares
remotePartyas sensitive, which is the nameCallBusinessEvent.ToEventData()actually emits. The manifest documentation showedphoneNumber,callerNumberandcalleeNumber, none of which exist in the schema and none of which would have masked anything.The plugin availability gate reads the effective workspace (#109).
PluginAdminWorkspaceResolverresolves the token-bound workspace first and falls back to?workspaceKey=only for platform operators, matching what the MCP path already did.HostAdminApiRouteScopedefaults toWorkspace, so a route without a resolvable workspace is rejected with400rather than passing through ungated, and a genuinely global route such as plugin status opts out explicitly. Communication's scope helpers no longer read the query themselves, which is what made the bypass reachable.WebSocket memory is bounded and media tickets are hashed (#108).
BoundedWebSocketReadercaps reassembly by byte count and tears down an idle socket, closing withMessageTooBiginstead of allocating. Audio payloads are checked before decoding and must match the negotiated frame size exactly, whichAudioFormat.BytesPerFramenow derives.PacedAudioSenderis bounded by total bytes as well as frame count, because many large frames stayed under a count-only cap.MediaStreamSessionkeeps a SHA-256 lookup key instead of the connect token, rejects future-dated rows that satisfied the old lower-bound TTL check forever, and an hourly job purges spent sessions.SIP mutations reconcile the runtime (#110).
ISipAccountRuntimeReconcileris the single path from a persisted account to a live channel, used by startup and by every mutation, so the two cannot drift. It is state-based and idempotent: an unchanged configuration fingerprint is a no-op, a changed one tears the old registration down before connecting the new one, and a disabled account is deprovisioned. Per-account locking serializes concurrent mutations. A runtime failure is written back onto the account as a failed status with its reason and returned as502with the account payload, so the row and the response agree with what the runtime did.Unsupported SIP authentication is refused rather than advertised (#111).
SipAuthMethodSupportis the boundary the admin form, the API and the reconciler share. Create and update answer422with a reason that names the upstream gap, the form offers only digest, and the reconciler fails such an account before touching the connector so accounts predating the guard surface that reason on startup.I disassembled
CalloraVoipSdk.Core4.7.3 before choosing refusal over implementation.SipLineChannelhas no path aroundRegisterAsyncand clamps the registration expiry toMath.Max(1, expiry), so a registration-less IP-authenticated trunk cannot be expressed.TlsConfigurationhangs offVoipOptionsrather thanSipAccount, andSipTlsCertificateProviderholds a single file-loaded certificate, so per-account client certificates cannot be expressed either. Both gaps are now tracked upstream.Scope
This covers the release gate and phase 1 of #123, plus the first two phase-2 findings. Phases 2 through 4 (#112 through #122) are not in this PR. Two items on #108 also remain open, both of which belong at the host WebSocket layer rather than in the plugin: per-IP and per-token connection limits, and an explicit origin policy for browser-facing endpoints.
3. Describe each step to reproduce the issue or behaviour.
#102, cross-tenant takeover. On
main, create a user who is a member ofworkspace-aandworkspace-b. Sign in as an administrator ofworkspace-aonly. SendPUT /api/users/{sharedUser}with a new email and password. The request succeeds and the victim's global credentials are replaced, so their access toworkspace-bis now under the attacker's control.DELETE /api/users/{sharedUser}likewise erases both memberships. On this branch both answer403, andWorkspaceAdminIdentityBoundaryTestscovers the victim in two workspaces, the operator who is also a workspace member, and the export.#103, unknown API key accepted. On
main, setBackendHost__EnableBootstrapApiKeys=trueandBackendHost__RequireApiKeyAuthentication=false, then call any control-plane endpoint withX-Callora-Api-Key: anything-at-all. The request is authenticated as platform super-admin. On this branch it answers401, andApiKeyAuthenticationHandlerTestswalks the full on/off matrix of both flags.#105, stolen token after a password change. Sign in and keep the access token. Change that account's password through
PUT /api/users/{id}. CallGET /api/auth/mewith the original token. Onmainit succeeds for the rest of the token's hour. On this branch it answers401. The same holds for deactivation, deletion and an RBAC role change, and logging out one session leaves the account's other sessions working.#110, disabled account keeps taking calls. Configure an enabled SIP account and let it register. Call
POST /api/.../sip-accounts/{id}/disable. Onmainthe row flips to disabled while the registration stays live until the next restart. On this branch the channel is deregistered before the response returns.#111, unsupported authentication. Create a SIP account with
authMethod: "IpAuthenticated"or"MutualTls". Onmainit is accepted with201and then silently skipped at provisioning, leaving the admin UI onConnectingwith no explanation. On this branch it answers422naming the upstream issue, and nothing is persisted.4. Please link to the relevant issues (if any).
fixes #102
fixes #103
fixes #104
fixes #105
fixes #106
fixes #107
fixes #109
fixes #110
fixes #111
related: #108 (per-IP connection limits and the origin policy are still open)
related: #123 (audit tracker)
downstream: BechsteinDigital/callora-voip-sdk#104
downstream: BechsteinDigital/callora-voip-sdk#183
Additional Changes
An early commit on this branch staged
custom/wholesale and picked up the archived Communication plugin's dependency tree, because the ignore pattern coveredcustom/static-plugins/*/node_modules/but not the archive's nested one. The files are untracked again and the pattern now covers both the archive and the plugins'app/*/node_modules. The blobs still sit in the intermediate commits, so squash-merging keeps them out ofmain.