Comfy-Org/desktop

Fork 0

mirror of https://github.com/Comfy-Org/desktop.git synced 2026-11-04 23:47:43 +00:00

[GH-ISSUE #1404] OneDrive detection and default path handling misses common variants #401

New issue

Closed

opened 2026-10-31 22:12:32 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-31 22:12:32 +00:00

Owner

Originally created by @benceruleanlu on GitHub (Nov 19, 2025).
Original GitHub issue: https://github.com/Comfy-Org/desktop/issues/1404

Summary

Path validation and default install path selection contain special handling for OneDrive, but the current implementation only covers a narrow set of cases:

buildRestrictedPaths() only uses process.env.OneDrive.
GET_SYSTEM_PATHS strips a hardcoded "OneDrive\\" substring from the documents path.

This can miss other common OneDrive variants and folders, leading to:

Default paths still landing on OneDrive in some setups.
Install locations under OneDrive being allowed until the runtime restriction logic happens to catch them (or not at all if the env var doesn’t line up).

Current behavior

In src/handlers/pathHandlers.ts:

// buildRestrictedPaths
if (process.platform === 'win32') {
  const localAppData = process.env.LOCALAPPDATA;
  // ...
  // 5. OneDrive
  if (process.env.OneDrive) {
    addRestrictedPath('oneDrive', process.env.OneDrive);
  }
}

and in GET_SYSTEM_PATHS:

ipcMain.handle(IPC_CHANNELS.GET_SYSTEM_PATHS, (): SystemPaths => {
  let documentsPath = app.getPath('documents');

  // Remove OneDrive from documents path if present
  if (process.platform === 'win32') {
    documentsPath = documentsPath.replace(/OneDrive\\/, '');
    return {
      appData: app.getPath('appData'),
      appPath: app.getAppPath(),
      defaultInstallPath: path.join(documentsPath, 'ComfyUI'),
    };
  }

  return {
    appData: app.getPath('appData'),
    appPath: app.getAppPath(),
    defaultInstallPath: path.join(documentsPath, 'ComfyUI'),
  };
});

Issues / edge cases

OneDrive variants not covered
- Windows often exposes other env vars like OneDriveCommercial, OneDriveConsumer, or path shapes such as C:\Users\<user>\OneDrive - <Tenant>\Documents.
- These won’t match process.env.OneDrive exactly or the simple /OneDrive\\/ regex in documentsPath.replace, so:
  - The default install path can still end up under a OneDrive-backed Documents folder.
  - evaluatePathRestrictions may not mark the path as isOneDrive when the env var doesn’t align with the actual path.
Default path sanitization is brittle
- documentsPath.replace(/OneDrive\\/, '') assumes a specific segment name (OneDrive\) and doesn’t consider spaces or suffixes.
- For paths like C:\Users\User\OneDrive - Work\Documents, the OneDrive segment is not removed and the default still lives on OneDrive.

Expected behavior

Any path under OneDrive, regardless of whether it’s OneDrive, OneDrive - Personal, OneDrive - <Tenant>, or similar, should be treated as isOneDrive and blocked for installation (as the UI text already suggests).
The default install path returned by GET_SYSTEM_PATHS on Windows should avoid landing under any OneDrive location when possible.

Possible improvements

More robust OneDrive root detection
- Consider checking multiple environment variables (OneDrive, OneDriveCommercial, OneDriveConsumer, etc.) and adding each to buildRestrictedPaths.
- Alternatively or additionally, detect OneDrive by path heuristics (e.g. segments starting with "OneDrive") when normalizing Windows paths.
Safer default documents path
- Instead of a string replace, parse the path into segments and strip the OneDrive* segment plus any following tenant suffix (e.g. "OneDrive - Work").
- Fall back to something like C:\Users\<user>\Documents when the current documents path appears to be under OneDrive.
Tests for OneDrive variants
- Extend tests/unit/handlers/pathHandler.test.ts with cases for:
  - documents paths like C:\Users\Test\OneDrive - Tenant\Documents.
  - Env vars OneDriveCommercial / OneDriveConsumer with corresponding paths.
- Assert that:
  - GET_SYSTEM_PATHS returns a default install path outside of any OneDrive subtree.
  - VALIDATE_INSTALL_PATH returns isOneDrive: true and isValid: false for these scenarios.

Impact

Users on common OneDrive configurations (especially business / tenant setups) may still get default install locations on OneDrive, contrary to the warning copy.
Install paths under OneDrive may not be consistently rejected, leading to potential performance or reliability problems the app is explicitly trying to avoid.

Suggested work

Decide on the canonical OneDrive detection strategy (env-based, heuristic, or both).
Update buildRestrictedPaths and GET_SYSTEM_PATHS to use that strategy.
Add focused unit tests covering the OneDrive variants above to prevent regressions.

┆Issue is synchronized with this Notion page by Unito

Originally created by @benceruleanlu on GitHub (Nov 19, 2025). Original GitHub issue: https://github.com/Comfy-Org/desktop/issues/1404 ### Summary Path validation and default install path selection contain special handling for OneDrive, but the current implementation only covers a narrow set of cases: - `buildRestrictedPaths()` only uses `process.env.OneDrive`. - `GET_SYSTEM_PATHS` strips a hardcoded `"OneDrive\\"` substring from the documents path. This can miss other common OneDrive variants and folders, leading to: - Default paths still landing on OneDrive in some setups. - Install locations under OneDrive being allowed until the runtime restriction logic happens to catch them (or not at all if the env var doesn’t line up). ### Current behavior In `src/handlers/pathHandlers.ts`: ```ts // buildRestrictedPaths if (process.platform === 'win32') { const localAppData = process.env.LOCALAPPDATA; // ... // 5. OneDrive if (process.env.OneDrive) { addRestrictedPath('oneDrive', process.env.OneDrive); } } ``` and in `GET_SYSTEM_PATHS`: ```ts ipcMain.handle(IPC_CHANNELS.GET_SYSTEM_PATHS, (): SystemPaths => { let documentsPath = app.getPath('documents'); // Remove OneDrive from documents path if present if (process.platform === 'win32') { documentsPath = documentsPath.replace(/OneDrive\\/, ''); return { appData: app.getPath('appData'), appPath: app.getAppPath(), defaultInstallPath: path.join(documentsPath, 'ComfyUI'), }; } return { appData: app.getPath('appData'), appPath: app.getAppPath(), defaultInstallPath: path.join(documentsPath, 'ComfyUI'), }; }); ``` ### Issues / edge cases 1. **OneDrive variants not covered** - Windows often exposes other env vars like `OneDriveCommercial`, `OneDriveConsumer`, or path shapes such as `C:\Users\<user>\OneDrive - <Tenant>\Documents`. - These won’t match `process.env.OneDrive` exactly or the simple `/OneDrive\\/` regex in `documentsPath.replace`, so: - The default install path can still end up under a OneDrive-backed Documents folder. - `evaluatePathRestrictions` may not mark the path as `isOneDrive` when the env var doesn’t align with the actual path. 2. **Default path sanitization is brittle** - `documentsPath.replace(/OneDrive\\/, '')` assumes a specific segment name (`OneDrive\`) and doesn’t consider spaces or suffixes. - For paths like `C:\Users\User\OneDrive - Work\Documents`, the OneDrive segment is not removed and the default still lives on OneDrive. ### Expected behavior - Any path under OneDrive, regardless of whether it’s `OneDrive`, `OneDrive - Personal`, `OneDrive - <Tenant>`, or similar, should be treated as `isOneDrive` and blocked for installation (as the UI text already suggests). - The default install path returned by `GET_SYSTEM_PATHS` on Windows should avoid landing under any OneDrive location when possible. ### Possible improvements 1. **More robust OneDrive root detection** - Consider checking multiple environment variables (`OneDrive`, `OneDriveCommercial`, `OneDriveConsumer`, etc.) and adding each to `buildRestrictedPaths`. - Alternatively or additionally, detect OneDrive by path heuristics (e.g. segments starting with `"OneDrive"`) when normalizing Windows paths. 2. **Safer default documents path** - Instead of a string replace, parse the path into segments and strip the `OneDrive*` segment plus any following tenant suffix (e.g. `"OneDrive - Work"`). - Fall back to something like `C:\Users\<user>\Documents` when the current `documents` path appears to be under OneDrive. 3. **Tests for OneDrive variants** - Extend `tests/unit/handlers/pathHandler.test.ts` with cases for: - `documents` paths like `C:\Users\Test\OneDrive - Tenant\Documents`. - Env vars `OneDriveCommercial` / `OneDriveConsumer` with corresponding paths. - Assert that: - `GET_SYSTEM_PATHS` returns a default install path outside of any OneDrive subtree. - `VALIDATE_INSTALL_PATH` returns `isOneDrive: true` and `isValid: false` for these scenarios. ### Impact - Users on common OneDrive configurations (especially business / tenant setups) may still get default install locations on OneDrive, contrary to the warning copy. - Install paths under OneDrive may not be consistently rejected, leading to potential performance or reliability problems the app is explicitly trying to avoid. ### Suggested work - Decide on the canonical OneDrive detection strategy (env-based, heuristic, or both). - Update `buildRestrictedPaths` and `GET_SYSTEM_PATHS` to use that strategy. - Add focused unit tests covering the OneDrive variants above to prevent regressions. ┆Issue is synchronized with this [Notion page](https://www.notion.so/Issue-1404-OneDrive-detection-and-default-path-handling-misses-common-variants-2b06d73d365081c0b092d185aee5892b) by [Unito](https://www.unito.io)

bsakaguchi

2026-10-31 22:12:32 +00:00

closed this issue
added the
bug

area: desktop-app

component: installer

platform: windows
labels

bsakaguchi commented

2026-10-31 22:12:33 +00:00

Author

Owner

@dosubot[bot] commented on GitHub (Feb 18, 2026):

Hi, @benceruleanlu. I'm Dosu, and I'm helping the desktop team manage their backlog and am marking this issue as stale.

Issue Summary:

You reported that the OneDrive detection and default install path logic is too narrow.
Current logic only recognizes limited environment variables and path patterns.
This causes issues with default install paths under OneDrive folders, especially for variants like OneDriveCommercial or tenant-specific names.
The problem leads to improper restriction application.
No further activity or comments have been made on this issue.

Next Steps:

Please let me know if this issue is still relevant with the latest version of the desktop app by commenting here.
If I don’t hear back within 7 days, this issue will be automatically closed.

Thanks for your understanding and contribution!

@dosubot[bot] commented on GitHub (Feb 18, 2026): Hi, @benceruleanlu. I'm [Dosu](https://dosu.dev), and I'm helping the desktop team manage their backlog and am marking this issue as stale. **Issue Summary:** - You reported that the OneDrive detection and default install path logic is too narrow. - Current logic only recognizes limited environment variables and path patterns. - This causes issues with default install paths under OneDrive folders, especially for variants like OneDriveCommercial or tenant-specific names. - The problem leads to improper restriction application. - No further activity or comments have been made on this issue. **Next Steps:** - Please let me know if this issue is still relevant with the latest version of the desktop app by commenting here. - If I don’t hear back within 7 days, this issue will be automatically closed. Thanks for your understanding and contribution!

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Comfy-Org/desktop#401

No description provided.

Rows
Columns