Repository navigation
fix(backups): secure pCloud setup and durable conversation exports - #22826
Conversation
…refix result path
…y and model validation
…_region_base_urls
…versation export - Fix ensure_folder sequential parent creation to prevent pCloud API error 2002 - Enforce path traversal rejection (. and ..) in upload_file - Add db.py with authenticated SHA256-CTR token encryption and account isolation - Add main.py FastAPI app with OAuth connect/disconnect and setup webview - Add /conversation webhook with explicit privacy consent gating (summary, transcript, audio) - Add 25 hermetic unit/integration tests running in 0.19s without external dependencies - Add Procfile and railway.toml deployment configurations Failure-Class: none
…and logout revocation - Normalize test response accessors (_get_body_text, _get_redirect_location) and expose Starlette-compatible body bytes and location headers in fallback shims for 100% environment parity - Make _get_encryption_key fail fast with RuntimeError when PCLOUD_TOKEN_ENCRYPTION_KEY is unset - Implement PCloudClient.logout() calling pCloud /logout and invoke it in /disconnect - Add .gitignore for plugins/omi-pcloud-app ignoring data/ token store, pycache, and env files - Pin requirements.txt to exact == dependencies - Add unit tests for logout endpoint and crypto key fail-fast (28 tests total, all passing) Failure-Class: none
…ocation - Normalize FastAPI Query default sentinel objects in auth_pcloud_callback when called directly as a coroutine - Pass error=None explicitly in test_app.py callback tests for 100% test pass rate with installed FastAPI dependencies - Add unit test verifying direct coroutine invocation unwraps Query sentinels safely (29/29 tests passing) Failure-Class: none
… in callback - Drop unused tenacity==8.2.3 from requirements.txt - Remove test-accommodation hasattr(param, 'default') unwrap in auth_pcloud_callback (main.py) to restore pure framework typing - Use TestClient wrapper when available and direct invocation fallback in test_app.py - Replace sentinel-normalization unit test with dedicated test_auth_pcloud_callback_error_param asserting 400 Bad Request on provider errors - Verified 29/29 tests pass hermetically; 100% Black 120 and byte-level diff hygiene compliant Failure-Class: none
…eipts Failure-Class: FC-unsigned-disconnect-deletes-tokens Preserve the #20498 foundation commits. Add authenticated setup/delivery, explicit private export consent, canonical conversation audio reads, encrypted durable retries, truthful revocation, and manifest-backed regression coverage. Local-only verification; keep #20213 open pending live account, runtime binding and rollout proof.
Declare scoped audio route policies and cover completed/partial receipt preservation in the production store and HTTP tests. Failure-Class: FC-unsigned-disconnect-deletes-tokens
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9514483528
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Auto-fix isn't included in your plan ↗ |
|
{"body": "Since my last review: addressed (verified on this head \u2014 fresh launch grants on every client: |
Retain both the plugin tests and nightly CI contract registrations, and both plugin capability and capture-policy route declarations. Installed-service suite: 67 passed. Four isolated backend files: 78 passed. Client refinements remain separate working changes; keep draft pending provider and volume verification.
Failure-Class: FC-unsigned-disconnect-deletes-tokens Fetch fresh grants for both settings and setup steps on every client; preserve signed query bytes, reject failed/stale account reads, and never forward Omi bearer credentials to vendors or log capability-bearing details. Reuse the existing app-detail owner and the shared transport. Verified: 25 focused Flutter tests, 10 native Swift boundary tests, 46 Windows tests, full web check (6 Bun + 876 Vitest), 67 plugin tests, 78 isolated backend tests, plus real browser synthetic HTTP retry. Keep draft for live provider, installed-client, volume, security acceptance and final-head CI. No deployment or customer writes.
Keep the plugin transport link, move its operational detail to the owning contract, and remove a duplicate general documentation reminder already enforced by root AGENTS.md. Lean-guide check passes at 38,988 bytes; no runtime behavior changes.
The global agent-doc reference gate exposed a historical onboarding path that is no longer in main. Verified the source at b0f8455 through GitHub, linked that immutable snapshot and explicitly qualified the old graph measurements. Reference and lean-guide checks pass; no app code or measured values changed.
Non-device verification follow-up — 2026-10-23All checks on current head Fresh local verification:
These are local synthetic tests, not live pCloud OAuth/export or hosted-volume proof. The existing developer console is accessible, but New app explicitly requires provider approval; no approved test OAuth application was available in that view. No application request, new grant, credential creation, live file write or deployment was performed. Still draft: approved minimal-scope test app/callback, live US/EU connect/export/retry/revoke, hosted-volume acceptance and independent security/operator acceptance remain gates. Pairwise integration with #21827 also has conflicts in |
|
Verdict: strong, security-first hardening of the pCloud backup foundation — no blocking correctness issue found in review, but two design decisions need maintainer sign-off before this leaves draft. What I verified in the largest surfaces:
Two points for maintainer sign-off:
Agent-behavior note: this PR edits Still wanted as a reviewer:
Performance: one extra authenticated GET per app-home/setup open on each client — acceptable for one-use grants. Blast radius: legacy unsigned integrations are unchanged by construction (no transport registration → original behavior), pinned by tests. USER IMPACT: none until the pCloud backup app is registered and rolled out; afterwards, opening an app's settings or home always uses a fresh single-use sign-in link instead of a possibly expired or already-used one. Security-sensitive auth/consent design (point 1) needs maintainer sign-off; leaving for human maintainer review under by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9deac7421d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…resh grants - Disconnect rotates a durable per-account authorization epoch, even with no connection. The callback checks the epoch before and after the token exchange under the account lock: a retired attempt never exchanges its code, and one already exchanging revokes its new token instead of storing it. - The Windows/Linux Apps page fetches fresh account-bound app details before launching setup and polling completion, and launches, polls or enables nothing after a failed read, an account change or an unmount. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Pushed
Evidence:
Not verified: a live pCloud OAuth exchange. pCloud hasn't approved the developer app yet, so there are no real client credentials. This PR stays in draft while the release prerequisites in the description are open. |
|
Merged current main ( About the earlier backend CI failure: the first run timed out installing system packages. On the re-run, the backend shards failed on seven pyright "unnecessary isinstance" errors in On the merged head: plugin suite 73 passed, Windows Apps/AppDetailSheet/appInstall 61, Flutter install-flow and setup-authorization 23, and the PR's backend tests (transport, routes, route policy, auto-disable recovery) all passed. |
|
Merged current main ( Why the previous head failed: every test in backend shard 1 passed, but the shard's total time ran 740–748 s, over the unchanged 720 s budget. The cause was the timing, not a test:
I didn't raise the budget. On the merged head: plugin suite 73, the PR's backend tests (transport, routes, route policy, auto-disable recovery) all passed, and the OpenAPI, TS-client and Dart-DTO checks are clean. |
|
Verdict: the security architecture here is sound and unusually well-tested for this size — no blocking code findings from me — but this stays a human-maintainer decision, and one CI wiring gap should be fixed before this leaves draft. What I verified on the current head:
Material notes:
USER IMPACT: when this ships, pCloud backup setup links become single-use and per-account — reopening settings always gets a fresh link — and conversation exports become durable, retryable jobs with transcript/audio consent both defaulting to off. What I'd still want as a reviewer: (1) the pCloud contract check running green on a pushed head, (2) maintainer sign-off on the 7-day audio-token TTL and on merge ordering vs #20498, (3) confirmation that the AGENTS.md Service-Map instruction removal is intentional. Leaving for human maintainer review: security-sensitive new auth surface and audio-read routes need maintainer sign-off, plus a merge-ordering decision against the stacked #20498. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
…Service Map update instruction
|
Pushed
Plugin suite 73 passed on this head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 922986c34d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not get_available_app_by_id(app_id, uid) or not is_user_app_enabled(uid, app_id): | ||
| raise HTTPException(status_code=403, detail='This backup app is no longer enabled') |
There was a problem hiding this comment.
If an app is globally disabled while backups are queued, its existing seven-day grants still pass both HEAD and GET here, allowing further uploads and audio downloads. get_available_app_by_id() checks private-app visibility but returns records with disabled=True, while is_user_app_enabled() only checks the user's Redis installation set; disable_app_in_firestore() does not remove those installations. This bypasses the disabled-app filter used by get_available_apps() for conversation fanout. Check the returned app's disabled state before authorizing pending backup writes or issuing audio URLs.
Useful? React with 👍 / 👎.
| : step?.url | ||
| ? urlWithUidParam(step.url, uid) | ||
| : null; | ||
| if (url) tab.location.href = url; |
There was a problem hiding this comment.
For an unregistered app, a developer can supply a javascript: home URL: ExternalIntegration.app_home_url in backend/models/app.py is an unrestricted string, and the create/update handlers do not validate its scheme. Assigning that URL here executes developer-controlled JavaScript in the reserved blank tab, which inherits Omi's origin; setting opener = null does not sandbox its storage access. The previous React-rendered href passed through React 19's JavaScript-URL sanitizer, whereas this imperative assignment bypasses it. Parse and allowlist safe navigation protocols before assigning either home or setup URLs.
Useful? React with 👍 / 👎.
| const uid = getCacheUid() | ||
| try { | ||
| const res = await omiApi.get<App>(`/v1/apps/${app.id}`) | ||
| if (!mounted.current || getCacheUid() !== uid) return |
There was a problem hiding this comment.
In the Windows/Linux development renderer, main.tsx wraps the app in StrictMode, which runs an extra effect setup/cleanup cycle. This component's existing cleanup sets mounted.current = false, but its effect setup never restores it to true. Consequently, this newly added guard silently rejects every Open App and setup-step launch in development, even after a successful details fetch. Restore the ref in effect setup and cover these launchers with a StrictMode regression test.
Useful? React with 👍 / 👎.
| @app.get('/setup-completed') | ||
| def setup_completed(uid: str, omi_token: str): | ||
| claims = verify(omi_token, 'status') |
There was a problem hiding this comment.
For a registered plugin using the documented /v1/oauth/authorize flow, the remaining caller in backend/routers/oauth.py still reads undecorated database metadata. Its /v1/oauth/token handler sends only uid to the completion endpoint (lines 213–226), so this newly required omi_token produces 422 and the OAuth exchange reports 503 even when setup is complete. Already-enabled users skip that probe but receive an undecorated home URL (line 270), which likewise cannot establish a pCloud session. Mint status and launch grants in this OAuth handler too, and preserve their query parameters in backend/templates/oauth_authenticate.html, which currently appends a literal ?uid=.
AGENTS.md reference: AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
Summary
Refs #20213. Builds on #20498 while preserving its contributor commits; do not merge the unsafe foundation first.
This PR completes the account/consent/durable-delivery boundary rather than adding another UID-only observer or uncorrelated audio buffer. Maintainer authorization now covers merging verified source and the repository's normal automatic development-deployment/app-release pipelines. No manual deployment, pCloud credential provisioning, provider activation, remote-data mutation or issue closure is requested.
Changes
Architecture and rollout constraints
The standalone service requires one replica on an explicitly configured persistent volume. It does not silently fall back from Redis to JSON or deploy onto ephemeral Cloud Run disk. No existing Redis/file data is read, deleted or automatically migrated. Any existing installation must be inventoried and migrated explicitly before rollout.
The shared plugin SDK remains models-only. Transport and job architecture/checklist are documented in
backend/docs/plugin-transport.mdandplugins/omi-pcloud-app/README.md; capture/playback ownership is recorded in the pipeline guide. User-facing setup/status remains the existing mobile/desktop integration flow.OMI_PLUGIN_TRANSPORT_JSONand the matching standalone configuration are intentionally unprovisioned. Reviewed runtime secret bindings on every finalization host, a pCloud developer app/test account, minimal provider permissions/callbacks, persistent-volume backup/restore, real connected US/EU export/retry/revocation and separately authorized rollout remain release gates. Live provider configuration has not been verified; local fixtures are not provider credentials. Code readiness and activation are separate. The new registered pCloud transport stays inactive until provisioned; activation remains blocked on the release gates above. Existing-client fresh-grant and credential-isolation fixes do affect later app builds, so the whole PR is not described as inactive. Keep #20213 open until they are verified.Line-Count-Exception: backend/utils/apps.py | 1716 -> 1731 | Re-enable validation owns the probe list, so the registered-transport branch (trusted-origin check plus non-mutating health probe instead of an unsigned delivery) belongs inside validate_app_endpoints_for_reenable rather than a parallel validator.
UI evidence
UI-Evidence: none
Both app-home and setup-step actions fetch fresh launch grants. Flutter
app_detail.dart, macOSAppsPage.swift, WindowsAppDetailSheet.tsxand webAppDetail.tsxretain the existing layout. Web uses a same-styled button, reserves a tab synchronously, then navigates it to the fresh URL. A failed refresh closes the reserved tab and shows a retryable inline error. Local browser evidence below uses synthetic loopback grants, not a connected provider.Review follow-up
Second review follow-up (2026-10-23)
Apps.tsxbeginSetupFlowbuilt setup and completion URLs from the undecorated catalog item. It now fetches this account's app details first, keeps signed query bytes viaurlWithUidParam, and launches, polls or enables nothing after a failed read, an account change or an unmount. The Install button stays busy during the read, so a repeated attempt gets a new grant.137509030a: plugin suite 73 passed (real FastAPI routes and SQLite, synthetic provider I/O, network denied); Windows Apps/AppDetailSheet/appInstall 61 passed, typecheck clean; isolated loopback service smoke (boot and restart) passed. No live pCloud OAuth: the developer console is signed in, but app creation awaits pCloud approval, so client credentials and a real token exchange remain unverified.Earlier verification (before the 2026-10-23 update)
bash scripts/run-plugin-pcloud-tests.sh: 67 real installed-service/provider/storage/signing contract tests passed with live provider requests blocked, including completed/partial receipt preservation on disconnect.Local source verification is on
bbace15c92, based on mainc84b9138f4c46b76728465656e4653308c64a289. Main advanced ten non-overlapping commits during this work; final GitHub merge-tree checks remain a gate before any merge.Expected consent behavior comes from #20213. Region routing, OAuth token behavior and revocation are defined by pCloud authorization, OAuth tokens and logout. Existing mock metadata/response conventions are not treated as authority over those contracts.
Failure-Class: FC-unsigned-disconnect-deletes-tokens
Line-Count-Exception: desktop/macos/Desktop/Sources/MainWindow/Pages/AppsPage.swift | 3748 -> 3759 | Fresh home and setup-step launches and page/account retirement stay in the existing detail-sheet owner; AppSetupURL owns fresh-read fencing and signed-query preservation.
Line-Count-Exception: backend/routers/apps.py | 2673 -> 2696 | Existing marketplace integration entry points need setup-capability decoration and one GET/HEAD handoff; signing, authorization and audio logic are extracted into focused utility modules, with route-composition tests.
Why not a new one-off guard: this PR provides the account-bound transport primitive and durable state owner, then exercises production HTTP/state transitions; the existing unsigned-disconnect class already records this trust-boundary failure. The new manifest entry runs that behavioral suite rather than scraping source for a preferred implementation.
UI-Evidence: none — Python-rendered standalone setup is outside released-client UI paths; local synthetic screenshots and browser observations are recorded in this task, not advertised as deployed/customer proof.
2026-10-23 maintainer review and merge gates
Ready for code review at Aryan's request; the gates below block merge, not review. The receipt/folder/retry-history fixes from the prior review are present. This pass resolves the current-main manifest conflicts, refreshes both home and setup-step grants on every client, preserves signed query parameters on Windows, rejects stale/failed launch reads, and removes mobile vendor completion bearer forwarding, signed-URL logging and app-detail body logging. The existing catalog owner also rejects retired account/ABA detail reads. Existing legacy apps without a completion endpoint keep optional setup. Local evidence: 67 installed plugin/provider/store tests, 78 isolated backend tests, 25 Flutter launch/authorization/privacy tests, 46 Windows renderer/install tests, 10 native Swift URL/launch-boundary tests, final Flutter analyzer, and the complete web check (6 Bun smoke + 876 Vitest tests) pass. The full macOS debug build also passed in 619.16 seconds with two jobs; this is not macOS 14 runtime acceptance or the full desktop test suite. A real browser with the production AppDetail and synthetic loopback HTTP grants accepted distinct grants on repeated setup clicks, closed a reserved tab after a forced 503, and recovered on retry. These are synthetic provider boundaries, not live pCloud OAuth/US/EU exports or installed-client acceptance. The source fix is c43b217; documentation-only follow-ups 7f34a3c and 9deac74 repair the backend instruction budget and pin a removed historical page reference to its measured source snapshot. At that historical checkpoint those items remained open. The current code-readiness and separate activation decisions are recorded in the 2026-10-25 update below. The mobile vendor isolation intentionally overlaps #21827; retain that boundary when integrating the two PRs. No deployment or credentials provisioned.
Maintainer readiness and scope update (2026-10-25)
922986c34d2f8e86595c48c48e7616d2c5fb7e46; no new blocking code defect identified in the current follow-up. Fresh localbash scripts/run-plugin-pcloud-tests.sh: 73 passed with synthetic provider IO and network denied. The changed transport implementation is unchanged from the previously reviewed head; prior valid browser, client-launch, storage, account/epoch and revocation regression evidence is reused, not presented as a fresh live-provider run.OMI_PLUGIN_TRANSPORT_JSONand the standalone provider configuration unprovisioned. Before pCloud activation, verify approved company/provider ownership, minimal grants and exact callbacks, real US/EU export/retry/revocation, and persistent-volume backup/restore. Keep feat(backups): add pCloud and extensible cloud-backup destinations #20213 open; a source merge is not customer recovery or proof of live-provider operation.