Repository navigation
fix(main): harden cloud owner password recovery trust and email sync - #1330
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughCloud recovery now checks the trusted server URL and active owner email. Owner email changes and backup restoration synchronize the registered email. Cloud configuration routes restrict selected fields to owners. ChangesCloud recovery and owner identity
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 6 files. (2 skipped: 2… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly summarizes the main changes: hardening cloud owner password recovery trust and synchronizing owner email data. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description follows the repository template and is substantive. It states the intent, lists the two verified findings (F1 recovery trust bypass and F2 owner email desync), gives a clear change lis… Full details: Description check
-
Explanation
The description follows the repository template and is substantive. It states the intent, lists the two verified findings (F1 recovery trust bypass and F2 owner email desync), gives a clear change list, and reports the focused test suites and six live scenarios with evidence. Several template sections are missing or incomplete. The 'Related work' section has no issue or discussion link. The 'Scope' section is absent. The 'Compatibility & data safety' section is not filled in, so database impact, migration, user-visible behavior, and offline/network impact are unstated. The 'Cross-cutting surfaces' table is blank. The 'Developer documentation' checkboxes are unchecked, although the summary says documentation was updated. The contributor checklist is unchecked. Commands run and results are given as free text under Testing, not under the template's Verification heading. CI is still pending. The core content is clear, so the description is mostly complete.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 6 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…:120509151887`, alert `js/missing-rate-limiting` on `tests/cloud-password-recovery.test.ts:80-90`) by removing the ad-hoc inline Express `jwt.verify` middleware in `tests/cloud-password-recovery.test.ts` and reusing `createApp` from `tests/helpers/test-setup.ts` to mount `/api/auth`, `/api/settings`, and `/api/staff`. Verified with: - `node tests/run-electron-node-test.cjs tests/cloud-password-recovery.test.ts` (passed) - `npm run lint` (passed) - `npm run build` (passed) - `npm run docs:check` (passed) - `git diff --check` (passed)
Intent
Verify these release-audit findings in the owner email password recovery feature (merged in #1310) and, if true, fix them:
cloudSync.isTrustedRecoveryServerUrl()checkscloud_server_urlfrom SQLite, but the recovery calls (requestOwnerPasswordRecovery,getOwnerPasswordRecoveryStatus,redeemOwnerPasswordRecoveryviasignedFetch) send requests to the in-memorythis.settings.server_url, which can differ.POST /api/settings/cloud/registerwritescloud_server_urlto SQLite before itscloud_deletion_blocked409 return and beforecloudSync.register()reloads settings, so SQLite and memory can disagree. Also, amanager(who hascloud.manageby default) can overwritecloud_api_key/cloud_store_idthroughPUT /api/settings/cloud, which would let recovery run under credentials the manager controls and reset the owner's password.PUT /api/staff/:id) or restoring/importing a backup leavessettings.email(what recovery uses) out of sync withusers.email, so recovery fails with 404 after approval; on a stranded store (no active administrator) the complete step matches withoutrole = 'owner'and can promote a non-owner staff member who holds the old email.What Changed
CloudSyncServiceto the trusted FloAdmin server URL verified against both persisted SQLite and active runtime settings, defercloud_server_urlpersistence and reload inPOST /api/settings/cloud/registeruntil pre-registration checks pass, and restrictcloud_server_url, unmaskedcloud_api_key, andcloud_store_idupdates on/api/settings/cloudand/api/settings/cloud/registerto theownerrole.settings.emailwhen an active owner's email or role changes viaPUT /api/staff/:idorPOST /api/staff/:id/deactivate(refreshing the cloud registration profile when updated), and preserve and resyncsettings.emailagainst active owner accounts during backup restore and import inmergeRestoreProtectedSettings.settings.emailto match an activeowneraccount incloudSync.isEmailRecoveryAvailable(),POST /api/auth/recover-password/cloud/request, andPOST /api/auth/recover-password/cloud/completeso FloAdmin email recovery never matches or promotes non-owner accounts on stranded stores, with regression tests and updated documentation.Risk Assessment
✅ Low: All recovery trust-boundary, owner-only cloud credential/endpoint mutation, owner email synchronization, backup/import email reconciliation, and non-owner role-promotion guards are enforced consistently across SQLite and in-memory CloudSync state and covered by behavioral integration tests.
Testing
Ran focused backend test suites (
test:cloud-password-recovery,test:cloud-account-status,test:staff-authz, andtest:issue-127-password-recovery) and stood up the compiled FloCafe Express server (startServer()) with an isolated SQLite database and disposable HTTP servers to drive all 6 happy-path and adversarial scenarios live over HTTP, recording the full verification report in~/.no-mistakes/evidence/01M5MN7S76WYVMZ8Z58VS5EJ4X/live-cloud-recovery-hardening-verification.json(no visual screenshot artifact was captured because this change only modifies backend API routes, SQLite restore helpers, and documentation with no UI surface changes); all checks and live scenarios passed.Evidence: Live HTTP & SQLite cloud recovery hardening verification report
{ "scenario1_memory_sqlite_divergence": { "sqliteServerUrl": "https://blue.flopos.com/", "inMemoryServerUrl": "http://127.0.0.1:62582", "setupStatusCloudRecoveryAvailable": false, "httpRequestRecoveryStatus": 409, "directRequestPasswordResetError": { "name": "CloudRequestError", "message": "Password recovery requires the official FloAdmin server", "status": 403 }, "untrustedServerHits": 0 }, "scenario2_blocked_register_no_sqlite_write": { "registerHttpStatus": 409, "storedCloudServerUrlAfter409": "https://blue.flopos.com/", "inMemoryTrustedAfter409": true }, "scenario3_manager_vs_owner_cloud_settings": { "managerPutApiKeyStatus": 403, "managerPutStoreIdStatus": 403, "managerPutServerUrlStatus": 403, "managerPostRegisterUrlStatus": 403, "managerToggleFlagsStatus": 200, "ownerPutCredentialsStatus": 200, "persistedApiKey": "live-owner-api-key", "persistedStoreId": "live-owner-store-id" }, "scenario4_staff_owner_email_sync_and_recovery": { "putStaffStatus": 200, "syncedSettingsEmail": "updated-owner@example.com", "cloudRecoveryAvailable": true, "recoverRequestStatus": 202, "recoverPollStatus": 200, "recoverCompleteStatus": 200, "loginWithNewPasswordStatus": 200 }, "scenario5_backup_restore_and_json_import_email_sync": { "sqliteRestoreSuccess": true, "emailAfterSqliteRestore": "updated-owner@example.com", "jsonImportStatus": 200, "emailAfterJsonImport": "updated-owner@example.com", "cloudRecoveryAvailableAfterRestoreAndImport": true }, "scenario6_stranded_store_non_owner_guard": { "createCashierStatus": 201, "setupStatusCloudRecoveryAvailable": false, "recoverRequestStatus": 409, "recoverCompletePrecheckStatus": 409, "recoverCompleteInnerLookupStatus": 404, "redeemCallsDuringStrandedAttempt": 0, "cashierRoleAfterAttempt": "cashier" } }Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
npm run test:cloud-password-recoverynpm run test:cloud-account-statusnpm run test:staff-authznpm run test:issue-127-password-recoverynpm run build && ELECTRON_RUN_AS_NODE=1 ./node_modules/.bin/electron <live-server-scenario-driver>drivingPOST /api/auth/setup/initialize,GET /api/auth/setup/status,POST /api/settings/cloud/register,PUT /api/settings/cloud,POST /api/staff,PUT /api/staff/:id,POST /api/auth/recover-password/cloud/request,GET /api/auth/recover-password/cloud/status,POST /api/auth/recover-password/cloud/complete,POST /api/auth/login,GET /api/db/export, andPOST /api/db/importagainst a live running server on port 3189✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Summary by CodeRabbit
Security
Bug Fixes