Files
PiCloud/security_audit/02_authz_isolation.md
MechaCat02 ec4a2aa24a style: cargo fmt across audit-2026-06-11 tier-3 changes
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-12 18:38:28 +02:00

16 KiB

Authorization & Cross-App Isolation

Audit date: 2026-06-11 Scope: Capability machinery, authz::{can, require, script_gate}, resolve_app, per-handler capability gating, SDK service app_id discipline, dispatcher fan-out, dashboard-supplied app ids, slug-history (redirected) path, public-script anonymous semantics.

Method (what I actually traced)

Two SDK paths traced from Rhai surface to SQL bind:

  1. kv::collection("x").set(k, v) — bridge in crates/executor-core/src/sdk/kv.rs:45 captures Arc<SdkCallCx> set at crates/executor-core/src/engine.rs:218-228 (app_id: req.app_id), where req.app_id = script.app_id from crates/orchestrator-core/src/api.rs:123 (the resolver's row). The KvService::set impl at crates/manager-core/src/kv_service.rs:130-187 calls self.repo.set(cx.app_id, …) which lands at crates/manager-core/src/kv_repo.rs:113-127 with WHERE app_id = $1 AND collection = $2 AND key = $3. No script-controlled app_id enters the call chain.
  2. dead_letters::replay(id) — bridge at crates/executor-core/src/sdk/dead_letters.rs, impl crates/manager-core/src/dead_letter_service.rs:62-94. Capability check (AppDeadLetterManage(cx.app_id)) runs first; the row is loaded by id, then row.app_id != cx.app_id short-circuits to NotFound (line 67-71). Cross-app id probing is timing-flat with intra-app not-found.

Two admin handlers traced from URL → resolve → require → repo:

  1. DELETE /admin/apps/{slug}/triggers/{trigger_id}crates/manager-core/src/triggers_api.rs:659-687. resolve_app (line 664) → require(Capability::AppManageTriggers(app_id)) against the RESOLVED app id (line 669) → load row → reject if trigger.app_id != app_id (line 680). No TOCTOU between resolve and authz; both run on the same app_id value.
  2. POST /admin/apps/{slug}/dead-letters/{id}/replaycrates/manager-core/src/dead_letters_api.rs:175-188. resolve_app → admin-synth SdkCallCx carrying the resolved app_id + principal (line 208-222) → service.replay(&cx, dl_id). Authz lives inside the service (above).

Service-trait sweep: every *Service trait in crates/shared/src/{kv,docs,files,secrets,email,http,queue,pubsub,dead_letters,invoke,events}.rs takes &SdkCallCx and does NOT accept app_id as a side argument. The only methods with an explicit app_id parameter are admin-mediated UsersService::{verify_session_for_realtime, admin_*, list_invitations, revoke_invitation} — called from the manager-core HTTP layer or the realtime authority, never from a script bridge.

High

Queue dispatcher does not assert script.app_id == claimed.app_id

  • Where: crates/manager-core/src/dispatcher.rs:290-340
  • What: dispatch_one_queue claims a queue message scoped to consumer.app_id, resolves script by consumer.script_id (line 290), and builds an ExecRequest with app_id: claimed.app_id (line 334) and script_id: consumer.script_id — but never cross-checks that script.app_id == claimed.app_id. The analogous build_invoke_request does (line 752: if script.app_id != row.app_id { return Err(...) }), and the cross-app invoke guard in invoke_service.rs:83-85 does too. Trigger registration (triggers_api.rs:280-303 validate_trigger_target) blocks same-app divergence at create time, so today this is guarded by a write-path invariant only. If a trigger row is hand-edited, restored from a partial backup, or (future) script-move-between-apps is added, the dispatcher would happily execute a script from app B under app A's SdkCallCx — every kv::*, docs::*, files::*, secrets::*, pubsub::*, email::* SDK call would scope to app A. The script's source belongs to app B but reads/writes app A's data.
  • Impact: Latent cross-app data read/write if any of the above ever holds. Defense-in-depth is missing on the single most security-critical fan-out path. The same gap is absent in the (sibling) build_invoke_request precisely because it was added there explicitly — the queue arm just wasn't audited the same way.
  • Recommendation: Mirror the invoke guard. After self.scripts.get(consumer.script_id) succeeds, check if script.app_id != consumer.app_id { dead_letter the message + tracing::error!("queue trigger script app mismatch"); return; }. Same fix in build_http_request (dispatcher.rs:651-718) — its script.app_id is never compared to row.app_id, only used.

Medium

get_script and list_routes_for_script return 404 before authz — cross-app id enumeration

  • Where: crates/manager-core/src/api.rs:184-197 (get_script), crates/manager-core/src/route_admin.rs:141-158 (list_routes)
  • What: Both handlers load the resource by id and 404 if absent, before the authz check. A Member of app A who guesses a script_id belonging to app B observes 404 for non-existent ids and 403 for ids that exist in app B (authz denies on AppRead(script.app_id)). The discriminator confirms script-id existence across the whole instance. Same gap on the routes-for-script list. Note this contrasts with apps_api::create_script (line 204-213), which intentionally inverts the order to avoid app-existence enumeration ("checking authz first means a Member trying to create against an unknown app gets 403 (no enumeration of app existence)").
  • Impact: Low-grade information leak — a malicious member can map out script-id space across apps. ScriptIds are UUIDv4, so brute-force enumeration is impractical, but ids leak through logs, error messages, and dashboard URLs (a former member with a stale screenshot can re-confirm validity post-revocation).
  • Recommendation: Either (a) collapse NotFound and Forbidden to the same response for these two handlers, or (b) flip the order: do a cheap authz pre-check (you can't, because you need the resource's app_id) — so (a) is the practical fix. Same shape as the membership-on-other-app pattern already in use elsewhere.

Capability check on routes:check / routes:match trusts a body-supplied app_id

  • Where: crates/manager-core/src/route_admin.rs:261-329
  • What: Both handlers pull app_id from the JSON body (CheckRouteRequest / MatchRouteRequest) and authz against it. Because authz blocks a Member of app A from AppRead(B), this is currently safe at runtime. The structural risk: the dashboard sends { app_id: appId, … } from dashboard/src/lib/api.ts:709-718 and any future code path that strips authz (e.g., a "public health-check" wrapper, a new internal caller) would have no second line of defense — the resource (app) isn't path-bound and isn't loaded.
  • Impact: Defense-in-depth gap. Inconsistent with every other recently-refactored handler (queues_api, files_api, secrets_api, topics_api, dead_letters_api, triggers_api) which path-bind the slug and load the app row before authz.
  • Recommendation: Either move to POST /admin/apps/{slug}/routes:check (path-bind the slug, then resolve_app), or load state.apps.get_by_id(input.app_id) and fail closed on None before authz. Both restore the resolve-then-authz invariant.

Cron / trigger fan-out runs forever under a deactivated user's principal — PrincipalResolver rejects but the dispatch arm has no recovery

  • Where: crates/manager-core/src/principal_resolver.rs:42-61, crates/manager-core/src/dispatcher.rs:314-318
  • What: When the registrant is deactivated (is_active = false), PrincipalResolver::resolve returns Inactive(user_id) → the dispatcher surfaces ResolveTrigger(...) and the dispatch loop logs and moves on. The outbox row stays claimed-then-rescheduled forever (or the cron row keeps inserting). There's no "disable the trigger when its owner is gone" path. Per dispatcher.rs:316, the resolver error short-circuits the dispatch but doesn't disable the trigger. This means a deactivated former admin's cron jobs continue to attempt execution indefinitely, building outbox backpressure.
  • Impact: Not a direct authz break (the script never runs because the principal isn't resolvable), but: (a) operational noise; (b) if the registrant is reactivated later with reduced privileges, every backed-up cron tick suddenly fires under their new (possibly weaker) identity at once; (c) if an attacker who briefly held InstanceManageUsers registered crons under a sock-puppet account and was then locked out, the crons remain dormant and re-arm on any future user revival with the same id.
  • Recommendation: On Inactive from the resolver, mark the trigger enabled = false (with an audit note) and dead-letter any in-flight outbox row that targets it. Mirrors the script-missing arm at dispatcher.rs:292-310 which already dead-letters cleanly.

Public/anonymous scripts can call same-app AppDeadLetterManage only because the service explicitly rejects principal: None

  • Where: crates/manager-core/src/dead_letter_service.rs:40-51
  • What: For dead letters specifically, the service handles "anonymous caller" with a hard Err(Forbidden) rather than script_gate's default "skip the check for cx.principal == None." That's the correct intent (managing dead letters is an admin act). The risk is that the convention for every other stateful service (kv, docs, files, secrets, pubsub, email, users) is the OPPOSITE: anonymous public-HTTP scripts skip the capability check entirely (see authz::script_gate at authz.rs:327-342). A public, anonymous-callable HTTP script in app A can write to kv::collection("x"), send email via email::send, publish on pubsub::*, mutate app-users via users::*, etc. — all under "script-as-gate" semantics. This is by design (the author owns the script and decides whether to expose it via a public route), but it means a single misconfigured route exposes powerful capabilities. There's no global toggle to require auth on a public route.
  • Impact: Documented design, but the boundary is one config mistake away from anonymous-internet → email send → SMTP-amplification-of-app-secrets. The "script is the gate" guarantee depends on the script author's discipline.
  • Recommendation: Two improvements — (a) document this loudly in the route-create endpoint response ("this route is anonymous-callable; the script you bound holds capabilities X, Y, Z and will execute them un-gated"); (b) consider a per-route require_principal: bool toggle so app admins can flip a route to "no anonymous calls allowed" without rewriting the script. Defers cleanly to v1.2.

Low

build_http_request does not cross-check script.app_id == row.app_id

  • Where: crates/manager-core/src/dispatcher.rs:651-718
  • What: Same shape as the High above. The HTTP outbox arm uses row.app_id to build the ExecRequest (line 686) and script (resolved by row.script_id at line 660) is trusted to live in the same app. Routes are guaranteed same-app by route_admin.rs:179 (app_id = script.app_id), so today no row violates the invariant. Listing as Low (not High) because the write-path is tighter for HTTP than for queue triggers — there's no validate_trigger_target equivalent because routes carry their own app_id. Still worth the belt-and-suspenders check.
  • Recommendation: One line: if script.app_id != row.app_id { return Err(DispatcherError::ResolveTrigger("http row app mismatch")); } after the script load.

Email-inbound writes outbox with URL app_id, not target.app_id

  • Where: crates/manager-core/src/email_inbound_api.rs:144-189
  • What: After cross-checking target.app_id != app_id at line 145, the outbox insert at line 177 uses the URL-derived app_id. They're equal because of the check, but target.app_id is the canonical source of truth (it survives even if the cross-check were ever relaxed). One-character fix; matters only if the cross-check ever gets refactored.
  • Recommendation: app_id: target.app_id in the NewOutboxRow.

AppLookup.redirected flag is consumed only in apps_api::get_app

  • Where: crates/manager-core/src/app_repo.rs:25-48, crates/manager-core/src/apps_api.rs:213-217
  • What: Every other consumer (triggers_api, topics_api, files_api, secrets_api, queues_api, dead_letters_api, users_admin_api, app_members_api) silently discards the flag via .map(|l| l.app.id) / .map(|l| l.app). That's fine — historical slugs are valid identifiers for the canonical app. No authz check is bypassed; the resolved app.id carries the same authority as the live-slug path. I traced this exhaustively; listing as Info to confirm no privilege escalation via redirect, but moving to Low because the convention is implicit (each consumer must remember to discard, not skip a check). A clearer API might be resolve_app returning (AppId, Option<String> /* redirected_from */) so the call site can never accidentally use the slug.
  • Recommendation: Cosmetic. Consider tightening the type.

RESERVED_MODULE_NAMES is a hard-coded list — drift risk on future SDK module additions

  • Where: crates/manager-core/src/api.rs:261-280
  • What: New SDK modules added to executor-core (queue is the most recent) must be remembered to be added to this list. It's not an authz issue per se; the shadow risk is that a user-supplied module of the same name resolves the user copy. Not load-bearing for cross-app isolation (each app's modules are app-scoped anyway), but worth a comment pointing at crates/executor-core/src/sdk/mod.rs so the next contributor edits both.
  • Recommendation: Centralize the reserved-name list in picloud-shared next to the SDK trait registrations.

Info

SDK trait surface is clean on app_id discipline

  • Every *Service trait method that a Rhai bridge can invoke takes &SdkCallCx and never accepts app_id as a parameter. Verified by grep across crates/shared/src/{kv,docs,files,secrets,email,http,queue,pubsub,dead_letters,invoke,events}.rs. The only app_id-parameter methods are admin-mediated (UsersService::admin_*, verify_session_for_realtime, list_invitations, revoke_invitation) and live under the HTTP admin layer, not the Rhai bridge.

Capability enum is exhaustively matched

  • Capability::app_id() and Capability::required_scope() use full match arms with no default branch, so adding a new variant forces every call site to be updated. Test capability_required_scope_mapping_is_complete at authz.rs:868-887 plus compiler enforcement makes this a self-healing surface.

Bound API keys cannot escape their app

  • binding_allows at authz.rs:449-461 denies instance-scoped capabilities for bound keys outright and requires target_app == bound_app for app-scoped ones. Mint-handler also rejects the combination upfront; the runtime layer is defense-in-depth. Tested at authz.rs:730-767.

Anonymous public scripts are visibly distinguishable in script_gate

  • authz::script_gate at authz.rs:327-342 explicitly distinguishes cx.principal.is_none() ("skip the check, script-as-gate") from authenticated callers. Recorded for clarity; the trade-off discussed in the Medium "Public/anonymous scripts" item above.

default slug has no runtime-privileged behavior

  • Only crates/manager-core/src/app_bootstrap.rs:39 references the slug at all, and only at fresh-install seed time. An attacker creating a new app and then deleting and recreating it cannot influence anything keyed on the slug — every authz check uses app_id (UUID), never the slug.

Out of scope (cross-references)

  • Capability check on the inbound-email HMAC is in agent 3's lane (crypto).
  • Rate limits on the cap-check Argon2 surface — agent 7 (DoS).
  • The dashboard's localStorage echo of the bearer token leaking the principal — agent 6 (network/CSRF) and agent 1 (authn).
  • The principal-cache TTL window letting a freshly-revoked admin still execute one request — agent 1 (authn) F-P-009 follow-up.