fix(marketing): stop mobile nav menu duplicating the desktop top navbar #197
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -523,15 +523,16 @@ export async function findRunOutputDir(workDir: string): Promise<string | null> | |
|
|
||
| async function readTextFileBounded(path: string, maxBytes: number): Promise<string> { | ||
| // The artifact location is selected only from a validated engine output directory. | ||
| // Open first, then fstat the live handle: this avoids the TOCTOU window where an | ||
| // attacker swaps the path for a symlink between a prior lstat and the open. | ||
| // eslint-disable-next-line security/detect-non-literal-fs-filename | ||
| const handle = await open(path, "r") | ||
| try { | ||
| const fileStat = await handle.stat() | ||
| if (!fileStat.isFile()) { | ||
| throw new Error(`Engine artifact is not a regular file: ${path}`) | ||
| } | ||
|
|
||
|
Comment on lines
+526
to
+535
There was a problem hiding this comment. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🌐 Web query:
💡 Result: Yes, Node.js 20 supports the use of fs.constants.O_NOFOLLOW in fs/promises.open [1][2]. This flag is an integer constant that can be combined with other file system flags (using bitwise OR) to specify that the open operation should fail if the path refers to a symbolic link [1][2][3][4]. For cross-platform applications, you should be aware of the following behaviors when using O_NOFOLLOW: 1. POSIX Compliance: On POSIX-compliant systems (such as Linux and macOS), O_NOFOLLOW is supported by the underlying open(2) system call. If the path specified is a symbolic link, the open operation will fail with an error—typically ELOOP [1][2]. This is frequently used to prevent Time-of-Check to Time-of-Use (TOCTOU) security vulnerabilities [1][2]. 2. Windows Platform Differences: The O_NOFOLLOW flag is inherently a POSIX concept. While Node.js exposes the constant on all platforms, its effectiveness and behavior on Windows may vary or be unsupported by the underlying operating system APIs. Applications should be prepared to handle cases where the flag might be ignored or result in unexpected behavior if strict symlink prevention is required on Windows. 3. Error Handling: Because the behavior of O_NOFOLLOW depends on the operating system, cross-platform code should implement robust error handling for the resulting errors (such as ELOOP on POSIX) and not rely solely on the flag to ensure security or consistency across different OS environments [1][2]. If your application must strictly forbid symlinks on all platforms, you should implement an additional verification step (e.g., using fs.promises.lstat to verify the file type before opening) [1][2]. Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "Repository files matching runner.ts:"
fd -a 'runner\.ts$' . | sed 's#^\./##' | rg 'apps/worker/src/engine/runner.ts|runner\.ts$' || true
echo
echo "Line count:"
wc -l apps/worker/src/engine/runner.ts
echo
echo "Relevant runner.ts section:"
sed -n '490,570p' apps/worker/src/engine/runner.ts | cat -n | sed 's/^/runner.ts /'
echo
echo "Imports/top section and run artifact usages:"
sed -n '1,80p' apps/worker/src/engine/runner.ts | cat -n | sed 's/^/runner.ts /'
rg -n "open\\(|handle\\.stat|isFile\\(\\)|run\\.json|vulnerabilities\\.json|artifact" apps/worker/src/engine/runner.tsRepository: ecryptoguru/lyrashield-ai Length of output: 9661 🌐 Web query:
💡 Result: The Citations:
🌐 Web query:
💡 Result: In Node.js (including version 20), fs.promises.lstat behaves correctly by providing information about the symbolic link itself rather than the file or directory it points to [1][2]. When you call fs.promises.lstat(path) on a symbolic link, it returns an fs.Stats object that represents the link [1][3]. Consequently, calling.isSymbolicLink on that returned object will correctly return true [4][5]. In contrast, if you were to use fs.promises.stat on the same path, Node.js would follow the symbolic link and return stats for the target file or directory instead [1][3]. In that case,.isSymbolicLink would return false [6][4]. To verify if a path is a symbolic link, you should always use lstat rather than stat [4][5]. If you need the path of the target that the symbolic link points to, you must use fs.promises.readlink(path) [3]. Summary of behavior: 1. fs.promises.lstat(path): Returns stats for the symlink itself..isSymbolicLink returns true [1][4]. 2. fs.promises.stat(path): Returns stats for the symlink's target..isSymbolicLink returns false [3][6]. 3. fs.promises.readlink(path): Returns the string path of the target the symlink points to [3]. Citations:
🌐 Web query:
💡 Result: The Citations:
Reject symlinked engine artifacts before reading them.
Add a regression test where 🧰 Tools🪛 ast-grep (0.45.0)[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec. (detect-child-process-typescript) 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| const buffer = Buffer.allocUnsafe(maxBytes + 1) | ||
| let offset = 0 | ||
| while (offset <= maxBytes) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,22 +1,47 @@ | ||
| import { lstat, open, rename, realpath } from "node:fs/promises" | ||
| import { randomUUID } from "node:crypto" | ||
| import { dirname, resolve } from "node:path" | ||
|
|
||
| /** | ||
| * Atomically write a file with a temp-and-rename pattern. The temp file is | ||
| * created with O_EXCL so a pre-existing file or symlink cannot be hijacked, | ||
| * fsynced before the rename for durability, and the final path is re-validated | ||
| * with lstat after the rename to ensure it is a regular file and not a dangling | ||
| * or followed symlink. The destination directory is resolved and checked so a | ||
| * parent-path symlink cannot redirect the write outside the intended location. | ||
| */ | ||
| export async function atomicWrite(filePath: string, content: string): Promise<void> { | ||
| const absolutePath = resolve(filePath) | ||
|
|
||
| // Validate the parent directory chain: if any parent is a symlink, the write | ||
| // could land outside the intended target (e.g. /tmp/link -> /etc). Resolve the | ||
| // directory and confirm the real path matches the requested directory. | ||
| const dir = dirname(absolutePath) | ||
| try { | ||
| // eslint-disable-next-line security/detect-non-literal-fs-filename | ||
| const realDir = await realpath(dir) | ||
| if (realDir !== dir) { | ||
| throw new Error(`Refusing to write through a symlinked directory: ${dir}`) | ||
| } | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code !== "ENOENT") throw error | ||
| // Parent does not exist yet (callers mkdir recursively); nothing to validate. | ||
| } | ||
|
Comment on lines
+16
to
+29
There was a problem hiding this comment. 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift 🧩 Analysis chain🌐 Web query:
💡 Result: Node.js 20 does not provide a direct, cross-platform descriptor-relative equivalent to POSIX Citations:
Do not rely on
🤖 Prompt for AI Agents |
||
|
|
||
| const tmp = `${absolutePath}.${randomUUID()}.lyrashield-tmp` | ||
|
|
||
| // O_EXCL: fail if the temp path already exists (including as a symlink). | ||
| // This prevents a symlink attack where an attacker points the temp path at | ||
| // another file and we overwrite the target. fsync before rename so the data is | ||
| // durable on disk before it becomes visible at the final path. | ||
| // eslint-disable-next-line security/detect-non-literal-fs-filename | ||
| const handle = await open(tmp, "wx") | ||
| try { | ||
| await handle.writeFile(content, "utf-8") | ||
| await handle.sync() | ||
| } finally { | ||
| await handle.close() | ||
|
Comment on lines
33
to
+43
There was a problem hiding this comment. 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🌐 Web query:
💡 Result: In Node.js 20, calling FileHandle.sync on a directory handle is not a reliable or universally supported way to synchronize directory metadata (such as a rename) across all platforms [1][2][3]. While FileHandle.sync is designed to flush data for an open file descriptor, its behavior on directories is platform-dependent and often fails or is unsupported [1][2][3]. On many POSIX-compliant systems, you may theoretically open a directory and attempt to fsync it, but this is not consistently exposed or guaranteed to work through the Node.js fs/promises FileHandle interface [2][3]. On Windows, attempting to open a directory for writing or synchronization often results in errors (such as EISDIR or access denied), as NTFS handles directory-level durability differently, often committing rename operations atomically within the file system's own journal [1][2]. Because of this lack of cross-platform consistency, there is no single, portable fallback provided by Node.js for directory fsync [1][4]. The recommended approach for ensuring the durability of a rename operation is as follows: 1. Perform the rename operation (fsPromises.rename), which is generally atomic on most modern file systems [1][3]. 2. Treat directory synchronization as best-effort [1]. Since there is no cross-platform standard, application developers often omit an explicit directory fsync or restrict it to specific environments (like Linux) where they can verify it works for their specific use case [1][4]. 3. If strict durability is required, design your application to handle partial or failed renames by checking for the existence of the destination file upon restart and ensuring that your file-content write/sync process (using FileHandle.sync on the data file itself) is robust [1][4]. Developers should be aware that FileHandle.sync on a file only ensures the file content reaches the storage; it does not guarantee the parent directory entry is committed [1][4][2]. If durability of the rename is critical, verify the specific requirements and behaviors of your target storage environment rather than relying on a generic, cross-platform directory sync method [1]. Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== repo files matching atomic-write =="
fd -a 'atomic-write\.ts$' . || true
echo "== inspect atomic-write file =="
if [ -f packages/cli/src/installers/atomic-write.ts ]; then
wc -l packages/cli/src/installers/atomic-write.ts
sed -n '1,120p' packages/cli/src/installers/atomic-write.ts | cat -n
fi
echo "== related rename/atomic write usages =="
rg -n "atomicWrite|rename\(|FileHandle\.sync|open\(.*wx|max.*rename|syncSyncDir|syncDirectory" packages/cli/src packages -g '*.ts' || true
echo "== Node fs sync directory API probe =="
node - <<'JS'
const fs = require('node:fs/promises');
(async () => {
console.log(process.version);
console.log("exports rename:", typeof fs.rename);
console.log("exists DirHandle?", typeof fs.open);
const cwd = process.cwd();
const handle = await fs.open(cwd, 'r');
console.log("open dir descriptor:", typeof handle.handle, typeof handle.fd, handle.fd);
try {
await handle.sync();
console.log("open dir handle.sync: returned");
} catch (err) {
console.log("open dir handle.sync: error", err.message, err.code, err.errno, err.syscall);
} finally {
await handle.close();
}
})();
JSRepository: ecryptoguru/lyrashield-ai Length of output: 8569 Document async guarantees for The written file contents persist before the rename, but 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| // filePath is the resolved installer target path selected for this workspace. | ||
| // eslint-disable-next-line security/detect-non-literal-fs-filename | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore
body.style.overflowwhen the resize handler closes the dialog.When
closeOnDesktop()callsmenu.close(), the close handler at Lines 138-143 runs. The normal saved value is an empty string. The truthiness check at Line 140 skips restoration, so the page can remain locked withoverflow: hiddenafter resizing from mobile to desktop.Test for an absent dataset value instead of a truthy value, and delete the saved value after restoration.
Proposed fix
🤖 Prompt for AI Agents