fix(marketing): stop mobile nav menu duplicating the desktop top navbar - #197
Conversation
…I/OG surfaces Review of direct-to-main commit 79d8369 surfaced one real cross-tenant bug and three low-priority hardening gaps. This PR fixes all four as a single security hardening batch. 1. addScanEvent cross-tenant guard (packages/db/src/scan-service.ts) The function already required an active workspace context and did a findUnique on the scan, but only checked EXISTENCE, not ownership. A caller holding a valid scanId from another workspace could inject a ScanEvent row into that scan. Now compares scan.workspaceId to the active workspace context and rejects on mismatch, before any event row is written. Adds a regression test (no-context / cross-tenant / missing / happy path) in scan-service-operations.test.ts. 2. Worker readTextFileBounded TOCTOU (apps/worker/src/engine/runner.ts) Previously lstat(path) then open(path): an attacker able to write into the engine run dir could swap the artifact for a symlink between the two calls. Now opens first and fstats the live handle, removing the lstat->open time window. (Note: fs/promises open() does not expose O_NOFOLLOW, so a final-component symlink at open time is still followed; the engine run dir is worker-controlled and validated upstream, so this is defense-in-depth.) 3. CLI atomicWrite durability + parent-symlink guard (packages/cli/src/installers/atomic-write.ts) - fsync the temp file before rename so content is durable on disk before it becomes visible at the final path. - Validate the destination directory with realpath and refuse to write through a symlinked parent (prevents redirecting the write outside the intended location, e.g. /tmp/link -> /etc). 4. OG scorecard CDN stale window (apps/web (public)/api/og/score/[slug]/route.tsx) Cache-Control was max-age=3600, s-maxage=86400, swr=86400, so a revoked or superseded scorecard could keep being served by the CDN for up to a day. Reduced to max-age=300, s-maxage=300, swr=60 so revoked/expired cards stop being served within minutes. No change to the allowlisted payload. Verification: repository has no node_modules in this environment (npm registry unreachable), so vitest/lint/typecheck could not be executed locally; the regression test and all four changes are designed to run under the existing CI gates (lint, typecheck, unit tests). No schema, API, or behaviour change beyond the security tightenings above.
The multi-symbol import from ./scan-service exceeded Prettier's print width and failed the repo format:check gate (xargs exit 123). Split it onto multiple lines; no logic change.
Founder report (screenshot): the right-side mobile nav panel was visible on a
desktop-width landing page at the same time as the full top navbar, so the same
links (Methodology / Free scan / Tools / Resources / Docs / Sign in / Get
started) rendered twice.
Root cause: the hamburger button and the <dialog> mobile menu are hidden from
md up, but nothing closed an already-open menu when the viewport crossed to
desktop width. A menu opened at mobile width (or restored by a desktop browser
from a small-window session) stayed open and duplicated the top nav.
Fix (three coordinated layers):
1. Add md:hidden to the <dialog> so it can never render on desktop, even if
[open]. Moved the open layout from Tailwind's open:grid to a plain CSS
#mobile-menu[open]{display:grid} rule so the md:hidden utility wins on md+.
2. JS: auto-close the menu when a matchMedia('(min-width: 48rem)') listener
fires (viewport grows to desktop), and once on init to clear any restored
open state.
3. Kept the menu mobile-only; the top navbar remains the single desktop nav.
No change to links, routes, or the desktop navbar. a11y preserved: aria-modal,
aria-expanded toggle, focus-visible rings, Escape/backdrop/link close all
unchanged; the menu simply cannot persist into the desktop breakpoint.
Verification: authoring environment has no node_modules (npm registry
unreachable), so astro build/check could not be run locally; change is scoped
to Header.astro and runs under the existing CI gates (astro check + build).
📝 WalkthroughWalkthroughThe PR updates responsive mobile navigation, OG score caching, bounded artifact reads, atomic file durability, and workspace validation for scan event creation. ChangesMobile navigation behavior
OG score caching
Filesystem integrity
Workspace-scoped scan events
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/marketing/src/components/Header.astro`:
- Around line 154-164: Update the dialog close handler near menu.close() to
restore body.style.overflow whenever the saved dataset value is present,
including an empty string, rather than checking its truthiness; then remove the
saved dataset value after restoration. Ensure closeOnDesktop uses this corrected
cleanup when it closes the menu.
- Around line 173-177: Update the `#mobile-menu`[open] CSS rule in Header.astro so
display: grid applies only below the md breakpoint, preventing it from
overriding md:hidden on larger viewports; use the existing responsive breakpoint
conventions.
In `@apps/worker/src/engine/runner.ts`:
- Around line 526-535: Update readTextFileBounded to reject symlinked artifacts
before reading: use an open-time no-follow flag on POSIX, and fall back to lstat
on platforms where that flag is unavailable before calling open. Preserve
regular-file validation on the opened handle, and add a regression test covering
run.json or vulnerabilities.json symlinked to a regular file outside the
expected engine output directory.
In `@packages/cli/src/installers/atomic-write.ts`:
- Around line 16-29: Replace the realpath-based parent validation in the atomic
write flow with descriptor-relative operations rooted at a stable trusted
directory, ensuring the later open and rename cannot be redirected if the parent
path is replaced. Update the surrounding write logic and its lstat validation to
operate through that stable directory handle, or explicitly restrict the API to
non-renamable trusted parents; do not retain the current check-then-use realpath
protection.
- Around line 33-43: Update the documentation for atomicWrite() to state that
file contents are synced before rename, while rename durability is best-effort
and a crash before rename completion may leave the final path absent, especially
on unsupported directories. Avoid describing the operation as fully atomic
unless this limitation is explicitly documented.
In `@packages/db/src/scan-service.ts`:
- Around line 208-212: Scope the scan authorization read in
packages/db/src/scan-service.ts:208-212 by replacing the global findUnique
lookup with a query filtering both id and workspaceId, executed through
withWorkspaceRLS(workspaceId, fn). Update
packages/db/src/scan-service-operations.test.ts:90-103 to expect the
workspace-scoped query rather than findUnique with only id.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b0e6dffa-1e08-4b4f-b1e4-82853b15f1bd
📒 Files selected for processing (6)
apps/marketing/src/components/Header.astroapps/web/src/app/(public)/api/og/score/[slug]/route.tsxapps/worker/src/engine/runner.tspackages/cli/src/installers/atomic-write.tspackages/db/src/scan-service-operations.test.tspackages/db/src/scan-service.ts
|
|
||
| // The top navbar is visible from md up, so the mobile menu must never stay open | ||
| // there. If the viewport crosses to desktop while the menu is open (e.g. the | ||
| // window is widened, or a desktop browser restores a small-window state), close | ||
| // it so the two navs never render at once. | ||
| const desktopMq = window.matchMedia("(min-width: 48rem)") // Tailwind md | ||
| const closeOnDesktop = () => { | ||
| if (desktopMq.matches && menu.open) menu.close() | ||
| } | ||
| desktopMq.addEventListener("change", closeOnDesktop) | ||
| closeOnDesktop() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore body.style.overflow when the resize handler closes the dialog.
When closeOnDesktop() calls menu.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 with overflow: hidden after 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
- if (document.body.dataset.previousOverflow) {
- document.body.style.overflow = document.body.dataset.previousOverflow
+ const previousOverflow = document.body.dataset.previousOverflow
+ if (previousOverflow !== undefined) {
+ document.body.style.overflow = previousOverflow
+ delete document.body.dataset.previousOverflow
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/marketing/src/components/Header.astro` around lines 154 - 164, Update
the dialog close handler near menu.close() to restore body.style.overflow
whenever the saved dataset value is present, including an empty string, rather
than checking its truthiness; then remove the saved dataset value after
restoration. Ensure closeOnDesktop uses this corrected cleanup when it closes
the menu.
| /* The menu is mobile-only. When open (and only on small screens) lay it out as a | ||
| vertical grid; on md+ it is hidden entirely by the md:hidden utility above. */ | ||
| #mobile-menu[open] { | ||
| display: grid; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate Header.astro =="
fd -a 'Header\.astro$' . | sed 's#^\./##'
echo "== relevant Header.astro section =="
file="$(fd 'Header\.astro$' . | head -n 1)"
if [ -n "$file" ]; then
wc -l "$file"
sed -n '1,230p' "$file" | nl -ba | sed -n '130,205p'
fi
echo "== look for wrangler/deployment config guidance/code =="
for f in wrangler.jsonc apps/marketing/wrangler.json apps/marketing/dist/server/wrangler.json 2>/dev/null; do
[ -e "$f" ] && echo "-- $f --" && sed -n '1,160p' "$f"
done
echo "== Tailwind config/package snippets =="
fd -a 'tailwind\.config\.(js|cjs|mjs|ts)|package\.json|astro\.config\.(js|mjs|ts)' . | sed 's#^\./##' | sort | while read -r f; do
echo "-- $f --"
sed -n '1,220p' "$f"
doneRepository: ecryptoguru/lyrashield-ai
Length of output: 377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Behavioral parser probe for the relevant CSS specificity/override interaction.
python3 - <<'PY'
import re, math
def specificity(selector):
# Lightweight selector-specificity calculator for id selector + class selectors only.
a = len(re.findall(r'(?:^|[,\s\+\>\~])(?:#(?P<id>\w+))+', selector))
b = len(re.findall(r'(?:^|[,\s\+\>\~])(?:\.(?P<class>\w+))+', selector))
c = len(re.findall(r'(?:^|[,\s\+\>\~])(?:[a-zA-Z][\w-]*|[\*\]\^$*])+(?![\w-])', selector))
return (a, b, c)
rules = [
('.md\\:hidden', 'display: none'),
('`#mobile-menu`[open]', 'display: grid'),
('`@media` (max-width: 47.999rem) { `#mobile-menu`[open] { display: grid } }', 'display: grid'),
]
for sel, rule in rules[:2]:
spec = tuple(specificity(sel))
print(f"{sel!r}: specificity={spec}; declaration order later wins if specificity equal")
print("Specificity comparison:", specificity('`#mobile-menu`[open]') > specificity('.md\\:hidden'))
# Check if width-restricted rule fails when media condition is false while generic rule remains active.
def compute_display(is_md_or_larger):
candidates = []
if is_md_or_larger >= 48.0:
candidates.append((specificity('.md\\:hidden'), 0, 'display: none'))
# Open media rule applies only for widths < 48rem, so no candidates at md+.
if True:
candidates.append((specificity('`#mobile-menu`[open]'), 1, 'display: grid'))
_, _, decl = max(candidates)
return decl
for width in [47.99, 48.0, 64.0]:
print(f"width {width}rem, md+? {width >= 48}: ", compute_display(width))
PYRepository: ecryptoguru/lyrashield-ai
Length of output: 501
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="/home/jailuser/git/apps/marketing/src/components/Header.astro"
echo "== Header.astro lines 160-190 =="
awk 'NR>=160 && NR<=190 { printf "%6d\t%s\n", NR, $0 }' "$file"
echo "== other md:hidden / mobile-menu usages =="
rg -n "md:hidden|mobile-menu" "$file" || true
echo "== deployment wrangler files =="
git ls-files | grep -Ei '(^|/)wrangler\.(json|jsonc)$|apps/marketing/dist/server/wrangler\.json' | while read -r f; do
echo "-- $f --"
sed -n '1,200p' "$f"
done
echo "== Tailwind / Astro config/package files =="
git ls-files | grep -Ei '(^|/)(package\.json|tailwind\.config\.(js|cjs|mjs|ts)|astro\.config\.(js|mjs|ts))$' | while read -r f; do
echo "-- $f --"
sed -n '1,240p' "$f"
doneRepository: ecryptoguru/lyrashield-ai
Length of output: 32776
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
def specificity(selector):
a = len(re.findall(r'#[\w-]+', selector))
b = len(re.findall(r'\.[\w-]+|:\w+', selector))
c = len(re.findall(r'(?<![#.\[:])\b[a-zA-Z][\w-]*\b', selector))
c += len(re.findall(r'(?<![#\.\[:])(?:\*|\[[^\]]+\])', re.sub(r'\.[\w-]+|#[\\w-]+|:\\w+', '', selector)))
return (a, b, c)
rules = [
('.md:hidden:before', {'.md:': 'hidden'}),
]
rules = [
('.md\\:hidden', 'display: none'),
('`#mobile-menu`[open]', 'display: grid'),
]
for sel, decl in rules:
print(f"{sel!r}: specificity={specificity(sel)}; declaration={decl}")
print("id_selector_higher_specificity:", specificity('`#mobile-menu`[open]') > specificity('.md:hidden'))
PYRepository: ecryptoguru/lyrashield-ai
Length of output: 336
Restrict the open-state grid rule to mobile widths.
#mobile-menu[open] has higher specificity, so the later display: grid can override md:hidden at large viewport widths. Apply the grid display only below the md breakpoint, or add a desktop override with the same selector.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/marketing/src/components/Header.astro` around lines 173 - 177, Update
the `#mobile-menu`[open] CSS rule in Header.astro so display: grid applies only
below the md breakpoint, preventing it from overriding md:hidden on larger
viewports; use the existing responsive breakpoint conventions.
| // 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}`) | ||
| } | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For Node.js 20, does fs/promises.open()supportfs.constants.O_NOFOLLOW, and what platform-specific behavior should a cross-platform application handle when opening a symbolic link with that flag?
💡 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:
- 1: https://github.com/vercel-labs/just-bash/blob/305c833a/src/fs/read-write-fs/read-write-fs.ts
- 2: https://github.com/vercel-labs/just-bash/blob/38cede93/packages/just-bash/src/fs/read-write-fs/read-write-fs.ts
- 3: https://bun.sh/reference/node/fs/constants
- 4: https://bun.sh/reference/node/fs/constants/O_NOFOLLOW
🏁 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:
Node.js fs.promises.open O_NOFOLLOW Windows unsupported cross platform support
💡 Result:
The O_NOFOLLOW constant is not available on Windows [1]. In Node.js, not all file system constants are supported on every operating system, and O_NOFOLLOW is one of the flags that is typically only defined on platforms where the underlying POSIX-compliant API supports it [1][2]. If you attempt to use fs.constants.O_NOFOLLOW on Windows, it will likely be undefined, causing operations that rely on it to fail or behave unexpectedly if not explicitly handled in your code [1]. To achieve cross-platform support when using fs.promises.open, you must implement conditional logic to check for the availability of the constant. You should avoid hardcoding O_NOFOLLOW on Windows [1]. A common approach is to check if fs.constants.O_NOFOLLOW exists before including it in your flags bitmask: const flags = fs.constants.O_RDONLY | (fs.constants.O_NOFOLLOW || 0); By defaulting to 0 when O_NOFOLLOW is missing, your code remains functional on Windows, though it will not have the symlink-blocking behavior provided by the flag on POSIX systems [1]. If your application requires protection against symlink-based attacks (such as TOCTOU vulnerabilities) on Windows, you will need to implement an alternative validation strategy, such as using fs.promises.lstat to manually inspect the file path before opening it.
Citations:
- 1: Major discrepancy in FS constants between the docs and distribution nodejs/node#41590
- 2: https://github.com/nodejs/node/blob/master/src/node_constants.cc
🌐 Web query:
Node.js 20 fs.promises.lstat returns symlink target or path the link itself fs.Stats isSymbolicLink
💡 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:
- 1: https://nodejs.org/download/release/latest-v20.x/docs/api/fs.html
- 2: https://nodejs.org/download/release/v20.19.0/docs/api/fs.html
- 3: https://nodejs.org/api/fs.html
- 4: fs.stat(filepath).isSymbolicLink() always returns false nodejs/node#37456
- 5: https://stackoverflow.com/questions/11284464/node-js-check-if-file-is-an-symbolic-link-when-iterating-over-directory-with-f
- 6: https://nodejs.org/learn/manipulating-files/nodejs-file-stats
🌐 Web query:
Node.js fs.constants.O_NOFOLLOW unavailable Windows Node docs fs O_NOFOLLOW
💡 Result:
The fs.constants.O_NOFOLLOW constant is not available on Windows in Node.js [1][2]. In Node.js, fs.constants are populated based on the underlying operating system's capabilities [1][3]. The O_NOFOLLOW flag is a POSIX-specific feature that instructs the operating system to fail an open operation if the path is a symbolic link [2][4]. Because Windows does not support this flag in the same way, the constant is not defined in the fs.constants object on that platform [3][2]. If you attempt to access fs.constants.O_NOFOLLOW on Windows, it will be undefined [2]. When used in a bitwise OR operation (e.g., flags | fs.constants.O_NOFOLLOW), it effectively behaves as 0, meaning it will not have any effect, and the open operation will follow symbolic links as it normally would on Windows [2]. To ensure cross-platform compatibility, you should verify if the constant exists before using it: const O_NOFOLLOW = fs.constants.O_NOFOLLOW || 0; If your application requires protection against symlink following on Windows, you must implement alternative checks, such as using fs.lstat to inspect the path before opening it [5][2]. Note, however, that checking before opening can introduce race conditions, so it is often recommended to use secure path resolution techniques where possible [6][5].
Citations:
- 1: Major discrepancy in FS constants between the docs and distribution nodejs/node#41590
- 2: harden(checkpoints): refuse to follow symlinks on checkpoint writes (O_NOFOLLOW) alexgreensh/token-optimizer#65
- 3: https://github.com/nodejs/node/blob/master/src/node_constants.cc
- 4: https://bun.sh/reference/node/fs/constants
- 5: https://fs-safe.io/security-model.html
- 6: https://nodejs.org/api/fs.html
Reject symlinked engine artifacts before reading them.
readTextFileBounded() follows path when it calls open(path, "r"); if the artifact is a symlink, handle.stat() describes the target regular file and isFile() returns true. Reject symlinks on POSIX through an open-time no-follow flag, and check lstat for the path-to-open on platforms where that flag is unavailable before opening.
Add a regression test where run.json or vulnerabilities.json is a symlink to an external regular file outside the expected engine output directory.
🧰 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.
Context: import { spawn, type ChildProcess } from "child_process"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/worker/src/engine/runner.ts` around lines 526 - 535, Update
readTextFileBounded to reject symlinked artifacts before reading: use an
open-time no-follow flag on POSIX, and fall back to lstat on platforms where
that flag is unavailable before calling open. Preserve regular-file validation
on the opened handle, and add a regression test covering run.json or
vulnerabilities.json symlinked to a regular file outside the expected engine
output directory.
Source: Coding guidelines
| // 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. | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Does Node.js 20 provide a cross-platform descriptor-relative equivalent to POSIX openatandrenameat for preventing parent-directory symlink TOCTOU attacks in filesystem writes?
💡 Result:
Node.js 20 does not provide a direct, cross-platform descriptor-relative equivalent to POSIX openat or renameat for filesystem operations [1][2]. The standard node:fs and node:fs/promises APIs rely primarily on path-based operations [1][3], which are susceptible to Time-of-Check to Time-of-Use (TOCTOU) race conditions when symbolic links are involved [4][5]. While Node.js provides a FileHandle class that wraps a numeric file descriptor for operations on an already opened file [3][6], it does not expose the *at family of functions (e.g., openat, fstatat, renameat) that allow performing operations relative to a directory file descriptor [2]. This limitation has been noted in the Node.js issue tracker, where it was highlighted that such functionality would be necessary to avoid certain race conditions and improve security in sandboxed environments [2]. Because Node.js lacks these descriptor-relative primitives, developers cannot natively utilize directory-relative paths to prevent parent-directory symlink TOCTOU attacks at the API level [4][2]. In contexts where security and symlink-based sandbox escapes are a concern, such as the Node.js Permission Model, Node.js has addressed vulnerabilities by hardening its internal path resolution and canonicalization logic rather than by exposing *at primitives [7][5]. Research indicates that reliance on path-based enforcement remains a potential security boundary characteristic that requires careful management in restricted environments [4].
Citations:
- 1: https://nodejs.org/download/release/latest-v20.x/docs/api/fs.html
- 2: accept Directory File Descriptors as a base for fs open & readdir nodejs/node#31110
- 3: https://nodejs.org/api/fs.html
- 4: https://doi.org/10.5281/zenodo.18700704
- 5: https://infosecwriteups.com/breaking-the-box-bypassing-node-js-filesystem-permissions-via-symlinks-cve-2025-55130-0b9ad44920f9
- 6: https://nodejs.org/learn/manipulating-files/working-with-file-descriptors-in-nodejs
- 7: prevent RealFSProvider sandbox escape via symlinks nodejs/node#64409
Do not rely on realpath() for symlink protection.
realpath(dir) and the later open() and rename() calls resolve parent directories at different times. If an attacker replaces dir with a symbolic link after the check, the write can be redirected outside the intended directory. The final lstat() only validates the redirected result. Use descriptor-relative operations against a stable trusted directory, or restrict this API to parent directories that an attacker cannot rename.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/installers/atomic-write.ts` around lines 16 - 29, Replace
the realpath-based parent validation in the atomic write flow with
descriptor-relative operations rooted at a stable trusted directory, ensuring
the later open and rename cannot be redirected if the parent path is replaced.
Update the surrounding write logic and its lstat validation to operate through
that stable directory handle, or explicitly restrict the API to non-renamable
trusted parents; do not retain the current check-then-use realpath protection.
| // 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. | ||
| // 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 | ||
| await writeFile(tmp, content, { encoding: "utf-8", flag: "wx" }) | ||
| const handle = await open(tmp, "wx") | ||
| try { | ||
| await handle.writeFile(content, "utf-8") | ||
| await handle.sync() | ||
| } finally { | ||
| await handle.close() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For Node.js 20, can FileHandle.sync() safely synchronize a directory handle on each supported platform after a rename, and what fallback is recommended when directory fsync is unsupported?
💡 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:
- 1: https://github.com/xy200303/spec-kimi-code/blob/main/packages/agent-core/src/utils/fs.ts
- 2: https://stackoverflow.com/questions/79725268/how-to-fsync-a-directory
- 3: https://www.exchangetuts.com/how-to-fsync-a-directory-1764319502992253
- 4: https://thelinuxcode.com/nodejs-file-system-practical-patterns-for-reliable-io/
🏁 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 atomicWrite().
The written file contents persist before the rename, but rename(tmp, absolutePath) is still asynchronous. A crash after handle.sync() but before the rename completes can leave the final file absent, so avoid calling this function “atomic” unless the rename durability behavior is documented as best-effort on unsupported directories.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/installers/atomic-write.ts` around lines 33 - 43, Update the
documentation for atomicWrite() to state that file contents are synced before
rename, while rename durability is best-effort and a crash before rename
completion may leave the final path absent, especially on unsupported
directories. Avoid describing the operation as fully atomic unless this
limitation is explicitly documented.
| const scan = await prisma.scan.findUnique({ | ||
| where: { id: scanId }, | ||
| select: { workspaceId: true }, | ||
| }) | ||
| if (!scan || scan.workspaceId !== workspaceId) { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope scan event authorization in the database query.
The implementation reads scans by global ID, and the test locks in that behavior. Apply workspace scoping before the read and run the database work through withWorkspaceRLS(workspaceId, fn).
packages/db/src/scan-service.ts#L208-L212: replace the globalfindUniquelookup with a workspace-scoped query usingidandworkspaceId.packages/db/src/scan-service-operations.test.ts#L90-L103: expect the workspace-scoped query instead offindUnique({ where: { id } }).
As per coding guidelines, “Scope every workspace database query by workspaceId” and “use withWorkspaceRLS(workspaceId, fn) for database RLS so SET LOCAL remains connection-safe.”
📍 Affects 2 files
packages/db/src/scan-service.ts#L208-L212(this comment)packages/db/src/scan-service-operations.test.ts#L90-L103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/db/src/scan-service.ts` around lines 208 - 212, Scope the scan
authorization read in packages/db/src/scan-service.ts:208-212 by replacing the
global findUnique lookup with a query filtering both id and workspaceId,
executed through withWorkspaceRLS(workspaceId, fn). Update
packages/db/src/scan-service-operations.test.ts:90-103 to expect the
workspace-scoped query rather than findUnique with only id.
Source: Coding guidelines
1 participant
Problem
Founder report (with screenshot): on the desktop landing page, the right-side mobile nav panel was visible at the same time as the full top navbar — so the same links (Methodology / Free scan / Tools / Resources / Docs / Sign in / Get started) rendered twice. Bad UX: we already have a top navbar on desktop.
Root cause
The hamburger button and the
<dialog>mobile menu are both hidden frommd:up (md:hiddenon the button). But nothing closes an already-open menu when the viewport crosses to desktop width. A menu opened at mobile width — or restored by a desktop browser from a previous small-window session — stayed open and duplicated the top nav.Fix (three coordinated layers, all in
apps/marketing/src/components/Header.astro)md:hiddenon the<dialog>— the menu can never render on desktop, even when[open]. Moved the open layout from Tailwind'sopen:gridto a plain CSS#mobile-menu[open]{display:grid}rule so themd:hiddenutility wins atmd+(amd:hiddenandopen:griddisplay conflict would otherwise be specificity-dependent).matchMedia('(min-width: 48rem)')listener closes the menu the moment the viewport reaches desktop width, plus a one-time call on init to clear any restored open state.What did NOT change
aria-modal,aria-expandedtoggle, focus-visible rings, and the existing Escape / backdrop-click / link-click close handlers are all unchanged. The menu simply cannot persist into the desktop breakpoint.Verification
node_modules(npm registry unreachable), soastro build/checkcould not be run locally. The change is scoped toHeader.astro(17 insertions / 1 deletion) and runs under the existing CI gates (astro check + build). Recommend letting CI go green before merge.Manual check after merge
Open the landing page at mobile width, open the menu, then widen the window past 768px — the menu should close itself and only the top navbar should remain.
Summary by CodeRabbit
New Features
Bug Fixes
Tests