fix: bug fixes, security hardening, and unit tests

[rubenmarcus]

Summary

• Break mutual recursion between closeOverlay() and switchToHuman() — infinite call loop caused silent stack overflow on every overlay close (existed since first commit)
• Path traversal prevention in Vite and Astro dev server middleware — reject .. and null bytes in .md file requests
• YAML injection fix — escape " and \ in frontmatter title/description strings
• Event listener memory leak — store keydown handler reference, remove in destroy()
• Race condition guard — isLoading flag prevents multiple overlays from rapid AI button clicks
• VERSION sync — single source of truth in index.ts (0.0.11), imported by CLI instead of hardcoded 0.0.2
• 10 new unit tests covering all fixes (142 → 152 tests, all passing)

Test plan

• npx vitest run — 152 tests pass (10 new)
• npx tsc --noEmit — clean compile
• Manual: open widget → close overlay → no console errors
• Manual: rapid-click AI button → only one overlay appears
• Manual: npx aeo.js --version shows 0.0.11

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
aeo-js	Ready	Preview, Comment	Mar 23, 2026 0:47am

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[claude]

🔴 A stale loadContent() invocation can attach duplicate event listeners to the new overlay after an open-close-reopen sequence. When the first suspended fetch resumes, it queries copyBtn/downloadBtn via this.overlayElement (now pointing to the second overlay), and the second loadContent() then adds its own listeners too -- causing every Copy or Download click to fire twice. The new rapid-click test does not cover this scenario; capturing this.overlayElement in a local variable before each await and checking it afterward would prevent the stale instance from corrupting the new overlay.

Extended reasoning...

What the bug is and how it manifests

In loadContent() (src/widget/core.ts around line 370), the local variable wrapper is captured from this.overlayElement.querySelector before the await fetch() suspension point. If closeOverlay() runs while the fetch is in flight and a new switchToAI() cycle begins, this.overlayElement is reassigned to a brand-new overlay element. When the original suspended loadContent() resumes, wrapper still references the old detached overlay node, but the subsequent this.overlayElement.querySelector('.aeo-copy-btn') and this.overlayElement.querySelector('.aeo-download-btn') queries are evaluated AFTER the await against the updated this.overlayElement, which now points to the second overlay. The stale invocation attaches click handlers to the new overlay's buttons, and when the second loadContent() also completes it attaches another set, leaving both buttons with duplicate listeners that fire twice per click.

The specific code path that triggers it

1. switchToAI() sets isLoading = true, creates overlay1, sets this.overlayElement = overlay1, then calls await this.showOverlay() which calls await this.loadContent(). loadContent() captures wrapper = overlay1.querySelector('.aeo-content-wrapper'), then suspends at await fetch(mdPath).
2. User clicks Human: closeOverlay() removes overlay1, sets this.overlayElement = undefined, sets this.isLoading = false.
3. User clicks AI again: isLoading is false so switchToAI() enters, creates overlay2, sets this.overlayElement = overlay2, starts a second loadContent() which suspends at its own await fetch(mdPath).
4. First fetch resolves. First loadContent() resumes. Guard if (!this.overlayElement) return passes because this.overlayElement is now overlay2 (not null). wrapper.innerHTML writes to the detached overlay1 node -- harmless and invisible. Then this.overlayElement.querySelector('.aeo-copy-btn') returns overlay2's copy button, and addEventListener attaches the first stale listener.
5. Second fetch resolves. Second loadContent() resumes and attaches its own listener to the same overlay2 copy/download buttons. Both buttons now have two click handlers each.

Why existing code does not prevent it

The isLoading flag introduced by this PR blocks entry into switchToAI() for rapid same-flow clicks (open, open, open). However, closeOverlay() resets isLoading = false, which is necessary to allow a future open. Once that reset happens, the second AI click is allowed -- but the first loadContent() is still suspended and will resume with a stale view of this.overlayElement. The guard inside loadContent() only checks if (!this.overlayElement) return, which was designed for the simpler close-without-reopen case. It is bypassed by the reopen case because this.overlayElement is non-null at resumption time (it points to the new overlay).

Impact

Every click on Copy triggers copyToClipboard() twice, showing two toasts and making two navigator.clipboard.writeText() calls. Every click on Download triggers downloadMarkdown() twice, spawning two anchor click events and two blob URL objects, which may produce two download dialogs in some browsers. This is a genuine user-visible defect on slow network connections where fetch latency makes the race window wide. The open-close-reopen flow is a natural user interaction.

Step-by-step concrete proof

1. User clicks AI. isLoading becomes true. this.overlayElement = overlayA. loadContent() suspends at await fetch('/page.md').
2. User clicks Human. closeOverlay(): overlayA.remove(), this.overlayElement = undefined, isLoading = false.
3. User clicks AI again. isLoading is false, so proceeds. this.overlayElement = overlayB. Second loadContent() suspends at await fetch('/page.md').
4. First fetch resolves. First loadContent() resumes. this.overlayElement is overlayB -- guard passes. wrapper (overlayA's content-wrapper) gets innerHTML overwritten with no visible effect. copyBtn = this.overlayElement.querySelector('.aeo-copy-btn') returns overlayB's copy button. First addEventListener call attaches a stale handler.
5. Second fetch resolves. Second loadContent() resumes. copyBtn = this.overlayElement.querySelector('.aeo-copy-btn') returns the same overlayB copy button again. Second addEventListener call attaches a canonical handler. Both fire on every click.

How to fix it

At the start of loadContent(), capture: const myOverlay = this.overlayElement. Replace all subsequent this.overlayElement references within loadContent() with myOverlay. After each await, add: if (myOverlay !== this.overlayElement) return. This ensures the stale invocation self-terminates before attaching any listeners to the new overlay. An integer generation counter or an AbortController are equivalent alternatives.

[claude]

🔴 Both src/plugins/astro.ts and src/plugins/vite.ts import resolve and sep from path but never use them — the intended path canonicalization check was never implemented. The current filename.includes("...") guard is a string heuristic that can be bypassed by percent-encoded traversal sequences (e.g. %2e%2e), and the dead imports should either be removed or used to complete the intended resolve()-based containment check.

Extended reasoning...

Incomplete Path Traversal Defense — Dead Imports Signal Missing Canonicalization

What the bug is

Both src/plugins/astro.ts (line 4) and src/plugins/vite.ts (line 7) now import resolve and sep from Node path module, but neither symbol is actually called anywhere in either file. Every other occurrence of resolve in these files is a variable name (resolvedConfig, resolvedPagesDir) or a Vite config object key (resolve: { alias: ... }), not a call to path.resolve. The symbol sep does not appear at all outside the import lines.

The specific code path

The path traversal guard added in this PR checks:

if (filename.includes('..') || filename.includes('�')) return next();

This runs before the middleware constructs the on-disk path via join(process.cwd(), resolvedConfig.contentDir, filename) and passes it to existsSync / readFileSync. The check is a string-level heuristic that catches literal .. sequences but does not canonicalize the path.

Why existing code does not prevent bypass

The presence of resolve and sep in the import statement strongly suggests the intended implementation was:

const base = resolve(join(process.cwd(), resolvedConfig.contentDir));
const fullPath = resolve(join(base, filename));
if (\!fullPath.startsWith(base + sep)) return next();

This check was never written. Without resolve(), a request for /foo/%2e%2e/etc/passwd.md produces filename = 'foo/%2e%2e/etc/passwd.md'. The includes('..') test is false because the dots are percent-encoded. Whether Node.js or the OS later normalizes %2e%2e to .. when resolving the path depends on the runtime and filesystem. path.resolve() applied first would canonicalize it before the containment test, closing this gap.

Impact

These middlewares run only on the local dev server (localhost) and only for .md-suffixed requests, so practical exploitability is limited. However, if the dev server is exposed on a non-loopback interface (e.g. --host 0.0.0.0, common in Docker/CI environments), a remote attacker on the same network could potentially read files outside the content directory. The dead imports are also a clear code defect signaling incomplete work.

How to fix

Option A: Complete the implementation using the already-imported symbols in both astro.ts and vite.ts:

const base = resolve(join(process.cwd(), resolvedConfig.contentDir));
const fullPath = resolve(join(base, filename));
if (\!fullPath.startsWith(base + sep)) return next();

Option B: Remove resolve and sep from both import lines if the simpler string check is considered sufficient.

Step-by-step proof

1. Attacker sends GET /foo/%2e%2e/etc/passwd.md to the dev server.
2. req.url is /foo/%2e%2e/etc/passwd.md; after slicing the leading slash, filename = foo/%2e%2e/etc/passwd.md.
3. filename.includes('..') evaluates to false because the dots are encoded as %2e.
4. filename.includes('�') is also false.
5. Both guards pass; the middleware proceeds to join(process.cwd(), contentDir, filename) and passes the result to existsSync/readFileSync.
6. On a standard Linux filesystem, readFileSync with a literal %2e%2e in the path does not treat it as .., so this particular vector fails at OS level. However, middleware frameworks that URL-decode before dispatch, or symlink-based directory structures, could still produce a traversal.
7. With the resolve()-based fix, step 5 would yield a canonicalized absolute path and startsWith(base + sep) would reject any path escaping contentDir regardless of encoding.

[claude]

🟡 In switchToAI(), isLoading is set to true before await this.showOverlay() but is only reset on the success path (line 132). If showOverlay() throws, isLoading stays true, preventing future AI button clicks until the user manually clicks the Human button to trigger closeOverlay(). The fix is a try/finally block around the await.

Extended reasoning...

Analysis

switchToAI() in src/widget/core.ts (lines 126-133) sets this.isLoading = true before awaiting showOverlay(), but only resets it on the happy path. If showOverlay() throws before returning, this.isLoading stays true. Because the AI click handler guards with if (this.isLoading) return, subsequent AI button clicks are silently swallowed — no overlay appears and no error is surfaced. this.isAIMode is also already true at that point, so the toggle visually shows AI as active with no content underneath.

Addressing the Refutation

The refutation raises three valid points worth addressing directly:

1. container.appendChild() on a detached container does not throw — Correct. Modern browsers allow appending to detached subtrees without error. The specific scenario in the bug report about detached containers causing appendChild to throw is inaccurate.

2. loadContent() has a comprehensive try/catch — Also correct; loadContent() never propagates exceptions upward. The remaining DOM operations in showOverlay() (createElement, innerHTML, addEventListener) are effectively infallible under normal browser conditions.

3. Recovery is possible via the Human button — Partially correct. Since isAIMode = true is set before the await, the Human button handler's condition checks this.isAIMode which is already true, so clicking Human calls switchToHuman() which calls closeOverlay() which resets isLoading = false. The widget is NOT permanently broken — it can be recovered manually.

Residual Impact

While showOverlay() is effectively non-throwing in the current implementation and manual recovery is available via the Human button, the code pattern is still incorrect. Any future change to showOverlay() that introduces a throwing path would silently put the widget in a confusing intermediate state. The isLoading flag is new code introduced by this PR, and adopting try/finally from the start costs nothing and prevents future regressions.

Concrete Example

1. showOverlay() throws (e.g., future code change adds a validation guard).
2. isLoading stays true; clicking AI button again does nothing — silently ignored.
3. User sees AI button highlighted with no overlay visible — confusing UX.
4. Recovery requires clicking Human button, which is non-obvious.

Fix

Wrap the await in try/finally: try { await this.showOverlay(); } finally { this.isLoading = false; }. This is the standard pattern for async state flags and costs nothing.