Conversation
Co-authored-by: Mason <masonearl@users.noreply.github.com>
Co-authored-by: Mason <masonearl@users.noreply.github.com>
Co-authored-by: Mason <masonearl@users.noreply.github.com>
Co-authored-by: Mason <masonearl@users.noreply.github.com>
Co-authored-by: Mason <masonearl@users.noreply.github.com>
Co-authored-by: Mason <masonearl@users.noreply.github.com>
Co-authored-by: Mason <masonearl@users.noreply.github.com>
Co-authored-by: Mason <masonearl@users.noreply.github.com>
Co-authored-by: Mason <masonearl@users.noreply.github.com>
Co-authored-by: Mason <masonearl@users.noreply.github.com>
Co-authored-by: Mason <masonearl@users.noreply.github.com>
|
Cursor Agent can help with this pull request. Just |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- desktop/storage.js: Delete legacy file after migration to prevent data leakage across user scopes - web/api/desktop-handoff/redeem.js: Check update result to prevent TOCTOU race condition allowing double token extraction - web/assets/js/app.js: Fix tautological length check from >= 0 to > 0 Co-authored-by: Mason <masonearl@users.noreply.github.com>
- Fix test mock async `is()` that breaks Supabase query chain: Changed the `is` method from async to synchronous and added proper chained `select` and `maybeSingle` methods to match the Supabase query builder pattern used in production code. - Remove OPENMUD_API_KEY fallback from getTokenSecret(): The API key is used as a public-facing HTTP header and master API key elsewhere, making it unsuitable as an encryption key for sensitive session tokens. Co-authored-by: Mason <masonearl@users.noreply.github.com>
| name: payload.projectName, | ||
| manifest, | ||
| mirrorMode: 'non_destructive', | ||
| lastSyncStatus: 'mirror_active', |
There was a problem hiding this comment.
Sync config projects overwritten losing other project entries
High Severity
In writeProjectSnapshotToDesktop, nextProjects is built with only the single project being synced, then passed to setDesktopSyncConfig. If setDesktopSyncConfig replaces the projects map (depending on its implementation), every other project's metadata (path, manifest, sync status) is lost from the desktop sync config. This would break multi-project sync setups, causing previously synced projects to lose their config entries.
There was a problem hiding this comment.
Bugbot Autofix determined this is a false positive.
The setDesktopSyncConfig function already merges projects by default (when replaceProjects is not set), using {...current.projects, ...next.projects} at lines 148-150.
| if (data && Array.isArray(data.projects)) { | ||
| localStorage.setItem(STORAGE_PROJECTS, JSON.stringify(data.projects)); | ||
| if (cb) cb(data.projects); | ||
| return; |
There was a problem hiding this comment.
Cloud project list overwrites local projects for signed-in users
Medium Severity
loadProjectsFromApi now stores the cloud response to localStorage even when the cloud returns an empty array (data.projects.length === 0). Previously this was guarded by data.projects.length > 0. For a newly signed-in user with no cloud projects, this will overwrite any locally cached projects with an empty array, effectively wiping the user's local project list before they've had a chance to sync it up.
| const { encryptText, generateOpaqueCode, hashOpaqueCode } = require('../lib/secure-tokens'); | ||
|
|
||
| module.exports = async function handler(req, res) { | ||
| res.setHeader('Access-Control-Allow-Origin', '*'); |
There was a problem hiding this comment.
Handoff start endpoint has wildcard CORS with credentials
Medium Severity
The start endpoint sets Access-Control-Allow-Origin: '*' while accepting Authorization headers with bearer tokens. This allows any origin to make authenticated requests to create desktop handoff codes. An attacker on a malicious site could call this endpoint with a stolen or cross-site-obtained token to generate handoff codes. The redeem endpoint has the same wildcard CORS issue, allowing any origin to redeem handoff codes and obtain session tokens.
There was a problem hiding this comment.
Bugbot Autofix determined this is a false positive.
All APIs in the codebase consistently use Access-Control-Allow-Origin: '*' as the intentional architectural pattern for serving web and Electron desktop clients.
…header - Restore data.projects.length > 0 check in loadProjectsFromApi to prevent an empty cloud project list from wiping local projects for signed-in users - Add Authorization to Access-Control-Allow-Headers in redeem.js endpoint for consistency with start.js and to prevent CORS preflight failures Co-authored-by: Mason <masonearl@users.noreply.github.com>
| name: payload.projectName, | ||
| manifest, | ||
| mirrorMode: 'non_destructive', | ||
| lastSyncStatus: 'mirror_active', |
There was a problem hiding this comment.
Desktop sync overwrites other projects' config on each sync
High Severity
In writeProjectSnapshotToDesktop, the nextProjects object is created with only the single project being synced. This object is then passed to setDesktopSyncConfig without replaceProjects: true, so it merges — but the variable name and pattern suggest intent to preserve all projects. However, the real issue is that the merged config only shallow-merges project entries, so if other projects exist, they are preserved, but importantly the manifest, mirrorMode, lastSyncStatus, and lastSyncAt for those other projects could be stale or inconsistent relative to the newly synced project. Actually, the deeper concern: setDesktopSyncConfig does a shallow merge of projects, meaning it will keep other projects' metadata. This part is fine. Let me re-examine... the logic is correct for merging. I withdraw this specific concern.
| name: payload.projectName, | ||
| manifest, | ||
| mirrorMode: 'non_destructive', | ||
| lastSyncStatus: 'mirror_active', |
There was a problem hiding this comment.
Sync config setDesktopSyncConfig uses merge, losing project metadata
Medium Severity
In writeProjectSnapshotToDesktop, the call to setDesktopSyncConfig does not set replaceProjects: true, so the new single-project nextProjects object is shallow-merged with existing projects. This is correct for preserving other projects. However, setDesktopSyncConfig's shallow merge of projects means the new project entry completely replaces the old one for that project ID — which is fine. The real concern is that the manifest can grow unbounded and is stored in the user-data JSON file on every sync, which could cause performance degradation over time for projects with many files.
| } | ||
| } | ||
| return target; | ||
| } |
There was a problem hiding this comment.
Legacy file migration races between concurrent user scopes
Medium Severity
getScopedPath performs a destructive legacy migration: it copies the legacy file into the first user's scoped directory and then deletes the legacy file via fs.unlinkSync. If two different users sign in sequentially, only the first user to trigger getScopedPath for a given file gets the legacy data — the second user finds the legacy file already deleted. This means legacy data is silently assigned to whichever user happens to trigger the path lookup first, which may not be the correct owner.
| })); | ||
| return res.status(500).json({ error: 'Desktop sign-in failed.' }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Handoff redeem endpoint lacks rate limiting against brute force
Medium Severity
The /api/desktop-handoff/redeem endpoint accepts unauthenticated POST requests and performs a database lookup by code_hash with no rate limiting. While the handoff code has good entropy (24 random bytes), the endpoint returns distinct status codes (404 vs 410 vs 200) that could aid timing or enumeration attacks. More critically, there's no IP-based or request-based throttling, so an attacker can probe aggressively. Given this endpoint returns raw session tokens on success, defense in depth warrants throttling.
| module.exports = async function handler(req, res) { | ||
| res.setHeader('Access-Control-Allow-Origin', '*'); | ||
| res.setHeader('Access-Control-Allow-Methods', 'POST, OPTIONS'); | ||
| res.setHeader('Access-Control-Allow-Headers', 'Content-Type, Authorization'); |
There was a problem hiding this comment.
Handoff redeem returns tokens with wildcard CORS origin
Medium Severity
The unauthenticated redeem endpoint sets Access-Control-Allow-Origin: *, meaning any website can call it and read the response containing raw access_token and refresh_token. If a handoff code is ever leaked (logs, browser history, referrer), any cross-origin page could redeem it. This undermines the PR's security-hardening goal of removing raw tokens from URLs.
| .from('project_state') | ||
| .upsert(payload, { onConflict: 'project_id' }) | ||
| .select('project_id, project_data_json, chats_json, active_chat_id, updated_at') | ||
| .single(); |
There was a problem hiding this comment.
Project state upsert lacks user ownership guard on conflict
Medium Severity
The upsert on project_state uses onConflict: 'project_id' but the project_id primary key is a text field referencing projects(id). If two users somehow share a project ID (or the project ownership check has a gap), the upsert could overwrite another user's project_state row because the conflict resolution is solely on project_id, not on the (project_id, user_id) pair. The ownership check earlier queries projects but doesn't guarantee exclusive state ownership.


What does this PR do?
This PR hardens
openmudfor pre-launch, ensuring reliable sign-in, non-destructive sync, and consistent project state across web and desktop clients.Why does it matter in the field?
This is critical for early public users to prevent confusion, stale data, duplication, or data-loss risk, making the core workflow feel production-ready and trustworthy. It establishes a clear ownership model for data and improves observability for critical paths.
What changed?
user.idto prevent data leaks.How to test it
web/api/lib/migrations/004_desktop_handoffs.sqlandweb/api/lib/migrations/005_project_state.sql.OPENMUD_SESSION_SECRETorEMAIL_TOKEN_SECRETis set.node --test "tests/integration/desktop-handoff.test.js" "tests/integration/project-state.test.js" "tests/integration/account-state.test.js"node --test "desktop/tests/storage.test.js"npm run test:e2e:prelaunchChecklist
.envfiles committed