- Shipped
- September 23, 2026 at 8:54 AM UTC
- Author
- Kamo
- Commit
- da4e9f3
Findings 2-6 from the phone-system audit, fixed together because several share files. 2. HIGH — VoipInstanceController had no right check on any mutating endpoint **************** any signed-in member of the org, not just its VOIP admins, could create, delete or repoint a phone server's credentials. Now requires MANAGE_VOIP_SETTINGS, the same right Settings -> Features -> Phone itself is gated on. Also: platformUrl/baseUrl were unvalidated, so a member could point RingCentral's token mint (which sends the org's real clientId/clientSecret + JWT) or a FreePBX GraphQL client at a host of their choosing and capture credentials, or reach the pod network (Redis/MinIO/Yugabyte all answer unauthenticated, no egress NetworkPolicy). Fixed with PhoneServerUrlGuard: RingCentral is now restricted to its own two hosts (production/sandbox); FreePBX gets the general public-only SSRF check built on kamo-shared-library's PublicHostGuard (the same primitive SecurityService's SafeSiteFetcher and aiservice's OutboundUrlGuard use). Both RingCentralJwtTokenService's and FreePBXTokenCache's token caches were keyed without the host, so a live Bearer token minted against the real host would keep being sent to a new one after a platformUrl/baseUrl edit -- both cache keys now include it. mergeConfig no longer lets a masked "***" secret ride along to a newly-set host (must be re-entered), never merges KamoPBX's server-assigned accountId/realm from a client, and Test Connection / manual sync no longer echo a raw exception message (which could carry the destination host or a response fragment back to the caller). Telnyx's apiKey added to SECRET_FIELDS. 3. HIGH — MemberVoipConfigController's PUT only checked that the target member was in the caller's org. Any signed-in member could reassign a COLLEAGUE's **************** and SipController.getSipCredentials hands back whatever extension is currently assigned -- a same-org account-takeover primitive, not merely an IDOR. Now requires MANAGE_EXTENSIONS or MANAGE_VOIP_SETTINGS unconditionally, matching the settings UI's own comment that extension assignment is admin-managed and never self-service. instanceId is now also checked against the caller's org -- previously a phone server from a different organization could be named and, if one of its extensions happened to be unassigned, claimed. 4. HIGH — no right checks anywhere on: **************** (create/update/delete/ test/test-send/send), VoipDevicesController, VoipUsersController, VoipExtensionsController, VoipOrgAggregateController, OrgPhoneNumberController, MemberPhoneNumberController. Most now require the right the corresponding kamo-internal screen itself is gated on (MANAGE_VOIP_SETTINGS for phone-server inventory/numbers screens, **************** for extension assignment). VoipOrgAggregateController's /org/voicemails additionally required VIEW_VOICEMAIL specifically (matching VoipVoicemailController, not the org-aggregate default) and gained the same PHI transcript-disclosure audit call. BulkTextInstanceController's /send sent arbitrary SMS with no outbound consent/suppression gate -- it now refuses (409) a number whose latest TCPA_SMS ConsentRecord is REVOKED, reading the WORM ledger SmsKeywordService already writes on every inbound STOP/START, tenant-scoped so a STOP to a different org never blocks this one. MemberPhoneNumberController is the one exception to "admin right required, period": unlike VOIP extension/instance assignment (finding 3, no self-service path at all by explicit product decision -- the settings UI's own comment says so), the settings/member Phone tab's MemberTextNumbersCard offers every viewer full self-service controls (assign/remove/make-primary) over their OWN numbers with no admin-only gate of its own -- that page is reachable on ACCESS_VOIP alone, per its own comment: "ACCESS_* covers the user managing their own settings; MANAGE_* covers admins configuring on a member's behalf." So this controller got a self-or-admin rule instead (mirrors **************** existing shape): a member manages their own numbers with no special right; acting on a colleague's still requires MANAGE_EXTENSIONS or MANAGE_VOIP_SETTINGS. An admin-only rule here would have 403'd every ACCESS_VOIP-only member out of a card that works today. 5. MEDIUM — **************** the unique index on ORG_PHONE_NUMBER is (org_id, phone_canon), not (phone_canon) alone (deliberately, so a ported number's history can exist under two organizations over time) -- but nothing stopped a SECOND org from creating its own row for a number a FIRST org already actively held, since get()/findByNumber() are org-scoped and simply would not find the other org's row. save() now refuses to create a new row when another org already has an active claim on the same number; discovery (which asks the provider itself, real evidence of ownership) is untouched. MemberPhoneNumberService's assign/unassign/ setPrimary/forMember validated numberId against the org but never memberId at all -- combined with assign()'s write-through to MemberVoipConfig (looked up by member id alone), a caller in one org could repoint a REAL MEMBER OF A DIFFERENT ORG's outbound caller id at their own phone infrastructure. requireMemberInOrg is the fix. 6. LOW (perf) — InstanceSyncService re-saved every cached extension/user/device/voicemail row on every sweep with a bumped dateUpdated, even when the provider reported nothing different -- ~23k avoidable UPDATEs/day. Each of the four sync methods now compares every field before writing it and only saves when something actually changed. Tests: PhoneServerUrlGuardTest, **************** **************** RingCentralJwtTokenServiceTest (new case), FreePBXTokenCacheTest, **************** **************** **************** **************** **************** **************** (self-service allowed, cross-member requires the admin right, both rights accepted), **************** Rights-only additions to **************** were verified by code review and full-suite compilation rather than a dedicated test per controller -- the pattern is identical and already covered by **************** and **************** Every guard above was mutation-checked (reverted locally, confirmed the matching test goes red, restored). Three pre-existing VoipInstanceController tests **************** VoipInstanceJustCallIdTest, **************** predate finding 2 and built their session with no rights list at all; they now include MANAGE_VOIP_SETTINGS so they still exercise the behavior they were written for (provisioning retry, JustCall id adoption, JWT-shape validation) rather than tripping the new right check first. **************** FreePBX case also swapped a non-resolving "pbx.example.com" placeholder for a literal IP, since PhoneServerUrlGuard's baseUrl check now performs a real DNS lookup. Report for the coordinator: no schema changes, no config changes, no gateway changes needed for these six findings (only finding 1's report has an operational follow-up). Confirmed against apiservice: it forwards /api/voip/** wholesale, so findings 2, 3, 5 and 6 need nothing there. BulkTextInstanceController (finding 4) sits at /api/bulktext/instances/**, which apiservice deliberately does NOT wildcard (only /api/bulktext/inbound/** is public, guarding the internal X-Internal-Auth /api/bulktext/send from ever being reachable from the internet) -- but it never needed to: kamo-internal's own server reaches it directly through **************** which proxies to VOIPSERVICE_URL, bypassing the public gateway entirely. Nothing to change on either side.