-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Prevent concurrent file copying #2841
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import log from "electron-log"; | |
| import { safeJoin } from "./path_utils"; | ||
| import { gitAdd } from "./git_utils"; | ||
| import { isWithinDyadMediaDir } from "./media_path_utils"; | ||
| import { withLock } from "./lock_utils"; | ||
| import { deploySupabaseFunction } from "../../supabase_admin/supabase_management_client"; | ||
| import { | ||
| isServerFunction, | ||
|
|
@@ -31,76 +32,103 @@ export interface CopyFileResult { | |
| export async function executeCopyFile({ | ||
| from, | ||
| to, | ||
| appId, | ||
| appPath, | ||
| supabaseProjectId, | ||
| supabaseOrganizationSlug, | ||
| isSharedModulesChanged, | ||
| }: { | ||
| from: string; | ||
| to: string; | ||
| appId: number; | ||
| appPath: string; | ||
| supabaseProjectId?: string | null; | ||
| supabaseOrganizationSlug?: string | null; | ||
| isSharedModulesChanged?: boolean; | ||
| }): Promise<CopyFileResult> { | ||
| // Resolve the source path: allow both .dyad/media paths and app-relative paths | ||
| let fromFullPath: string; | ||
| if (path.isAbsolute(from)) { | ||
| // Security: only allow absolute paths within the app's .dyad/media directory | ||
| if (!isWithinDyadMediaDir(from, appPath)) { | ||
| throw new Error( | ||
| `Absolute source paths are only allowed within the .dyad/media directory`, | ||
| ); | ||
| return withLock(appId, async () => { | ||
| // Resolve the source path: allow both .dyad/media paths and app-relative paths | ||
| let fromFullPath: string; | ||
| if (path.isAbsolute(from)) { | ||
| // Security: only allow absolute paths within the app's .dyad/media directory | ||
| if (!isWithinDyadMediaDir(from, appPath)) { | ||
| throw new Error( | ||
| `Absolute source paths are only allowed within the .dyad/media directory`, | ||
| ); | ||
| } | ||
| fromFullPath = path.resolve(from); | ||
| } else { | ||
| fromFullPath = safeJoin(appPath, from); | ||
| } | ||
| fromFullPath = path.resolve(from); | ||
| } else { | ||
| fromFullPath = safeJoin(appPath, from); | ||
| } | ||
|
|
||
| const toFullPath = safeJoin(appPath, to); | ||
| const toFullPath = safeJoin(appPath, to); | ||
|
|
||
| if (!fs.existsSync(fromFullPath)) { | ||
| throw new Error(`Source file does not exist: ${from}`); | ||
| } | ||
| if (!fs.existsSync(fromFullPath)) { | ||
| throw new Error(`Source file does not exist: ${from}`); | ||
| } | ||
|
|
||
| // Security: resolve symlinks and re-validate that paths remain within bounds. | ||
| // path.resolve() does not follow symlinks, so an attacker could place a | ||
| // symlink inside the allowed directory that points outside it. | ||
| const realFromPath = fs.realpathSync(fromFullPath); | ||
| const resolvedAppPath = fs.realpathSync(appPath); | ||
| if ( | ||
| path.isAbsolute(from) && | ||
| !isWithinDyadMediaDir(realFromPath, resolvedAppPath) | ||
| ) { | ||
| throw new Error( | ||
| `Source path resolves to a location outside the .dyad/media directory (possible symlink traversal)`, | ||
| ); | ||
| } | ||
| if ( | ||
| !path.isAbsolute(from) && | ||
| !realFromPath.startsWith(resolvedAppPath + path.sep) && | ||
| realFromPath !== resolvedAppPath | ||
| ) { | ||
| throw new Error( | ||
| `Source path resolves to a location outside the app directory (possible symlink traversal)`, | ||
| ); | ||
| } | ||
|
|
||
| // Track if this involves shared modules | ||
| const sharedModuleChanged = isSharedServerModule(to); | ||
| // Track if this involves shared modules | ||
| const sharedModuleChanged = isSharedServerModule(to); | ||
|
|
||
| // Ensure destination directory exists | ||
| const dirPath = path.dirname(toFullPath); | ||
| fs.mkdirSync(dirPath, { recursive: true }); | ||
| // Ensure destination directory exists | ||
| const dirPath = path.dirname(toFullPath); | ||
| fs.mkdirSync(dirPath, { recursive: true }); | ||
|
|
||
| // Copy the file | ||
| fs.copyFileSync(fromFullPath, toFullPath); | ||
| logger.log(`Successfully copied file: ${fromFullPath} -> ${toFullPath}`); | ||
| // Copy the file (do not follow symlinks at destination) | ||
| fs.copyFileSync(fromFullPath, toFullPath); | ||
|
Comment on lines
+100
to
+101
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MEDIUM | security / misleading-comment Misleading comment + missing destination symlink validation The comment The source path now has thorough symlink-traversal validation via 💡 Suggestion: Either (1) add a |
||
| logger.log(`Successfully copied file: ${fromFullPath} -> ${toFullPath}`); | ||
|
|
||
| // Add to git | ||
| await gitAdd({ path: appPath, filepath: to }); | ||
| // Add to git | ||
| await gitAdd({ path: appPath, filepath: to }); | ||
|
|
||
| // Deploy Supabase function if applicable | ||
| const effectiveSharedModulesChanged = | ||
| isSharedModulesChanged || sharedModuleChanged; | ||
| let deployError: unknown; | ||
| if ( | ||
| supabaseProjectId && | ||
| isServerFunction(to) && | ||
| !effectiveSharedModulesChanged | ||
| ) { | ||
| try { | ||
| await deploySupabaseFunction({ | ||
| supabaseProjectId, | ||
| functionName: extractFunctionNameFromPath(to), | ||
| appPath, | ||
| organizationSlug: supabaseOrganizationSlug ?? null, | ||
| }); | ||
| } catch (error) { | ||
| logger.error("Failed to deploy Supabase function after copy:", error); | ||
| deployError = error; | ||
| // Deploy Supabase function if applicable | ||
| const effectiveSharedModulesChanged = | ||
| isSharedModulesChanged || sharedModuleChanged; | ||
| let deployError: unknown; | ||
| if ( | ||
| supabaseProjectId && | ||
| isServerFunction(to) && | ||
| !effectiveSharedModulesChanged | ||
| ) { | ||
| try { | ||
| await deploySupabaseFunction({ | ||
| supabaseProjectId, | ||
| functionName: extractFunctionNameFromPath(to), | ||
| appPath, | ||
| organizationSlug: supabaseOrganizationSlug ?? null, | ||
| }); | ||
| } catch (error) { | ||
| logger.error("Failed to deploy Supabase function after copy:", error); | ||
| deployError = error; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| sharedModuleChanged, | ||
| deployError, | ||
| }; | ||
| return { | ||
| sharedModuleChanged, | ||
| deployError, | ||
| }; | ||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,50 +1,36 @@ | ||
| const locks = new Map<number | string, Promise<void>>(); | ||
|
|
||
| /** | ||
| * Acquires a lock for an app operation | ||
| * @param lockId The app ID to lock | ||
| * @returns An object with release function and promise | ||
| */ | ||
| export function acquireLock(lockId: number | string): { | ||
| release: () => void; | ||
| promise: Promise<void>; | ||
| } { | ||
| let release: () => void = () => {}; | ||
|
|
||
| const promise = new Promise<void>((resolve) => { | ||
| release = () => { | ||
| locks.delete(lockId); | ||
| resolve(); | ||
| }; | ||
| }); | ||
|
|
||
| locks.set(lockId, promise); | ||
| return { release, promise }; | ||
| } | ||
|
|
||
| /** | ||
| * Executes a function with a lock on the lock ID | ||
| * Executes a function with a lock on the lock ID. | ||
| * Uses promise-chaining so that queued operations execute serially, | ||
| * preventing the race where multiple waiters all acquire simultaneously. | ||
| * | ||
| * @param lockId The lock ID to lock | ||
| * @param fn The function to execute with the lock | ||
| * @returns Result of the function | ||
| */ | ||
| export async function withLock<T>( | ||
| export function withLock<T>( | ||
| lockId: number | string, | ||
| fn: () => Promise<T>, | ||
| ): Promise<T> { | ||
| // Wait for any existing operation to complete | ||
| const existingLock = locks.get(lockId); | ||
| if (existingLock) { | ||
| await existingLock; | ||
| } | ||
| const lastOperation = locks.get(lockId) ?? Promise.resolve(); | ||
|
|
||
| let resolve: () => void; | ||
| const newLock = new Promise<void>((r) => { | ||
| resolve = r; | ||
| }); | ||
| locks.set(lockId, newLock); | ||
|
|
||
| // Acquire a new lock | ||
| const { release } = acquireLock(lockId); | ||
| const result = lastOperation.then(async () => { | ||
| try { | ||
| return await fn(); | ||
| } finally { | ||
| resolve(); | ||
| if (locks.get(lockId) === newLock) { | ||
| locks.delete(lockId); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| try { | ||
| const result = await fn(); | ||
| return result; | ||
| } finally { | ||
| release(); | ||
| } | ||
| return result; | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.