Skip to content

fix: bug fixes, security hardening, and unit tests #18

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.

Sign up for GitHub

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

Open
rubenmarcus wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: bug fixes, security hardening, and unit tests #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b1cfe7
fix: multiple bug fixes, security hardening, and unit tests
rubenmarcus Mar 23, 2026
6d089ef
merge: resolve conflict with main in closeOverlay
rubenmarcus Mar 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ import { auditSite, formatAuditReport } from './core/audit';
import { generateReport, formatReportMarkdown, formatReportJson } from './core/report';
import { writeFileSync, existsSync } from 'fs';
import { join } from 'path';

const VERSION = '0.0.2';
import { VERSION } from './index';

const HELP = `
aeo.js v${VERSION} — Answer Engine Optimization for the modern web
Expand Down
39 changes: 39 additions & 0 deletions src/core/raw-markdown.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,4 +195,43 @@ describe('generatePageMarkdownFiles', () => {
expect(writtenContent).toContain('# About');
expect(writtenContent).toContain('Content here');
});

it('should escape double quotes in title frontmatter', () => {
const config = createConfig({
pages: [
{ pathname: '/test', title: 'He said "hello"', content: 'Body text' },
],
});

generatePageMarkdownFiles(config);

const writtenContent = vi.mocked(writeFileSync).mock.calls[0][1] as string;
expect(writtenContent).toContain('title: "He said \\"hello\\""');
});

it('should escape double quotes in description frontmatter', () => {
const config = createConfig({
pages: [
{ pathname: '/test', title: 'Test', description: 'A "great" page', content: 'Body' },
],
});

generatePageMarkdownFiles(config);

const writtenContent = vi.mocked(writeFileSync).mock.calls[0][1] as string;
expect(writtenContent).toContain('description: "A \\"great\\" page"');
});

it('should escape backslashes in title frontmatter', () => {
const config = createConfig({
pages: [
{ pathname: '/test', title: 'Path\\to\\file', content: 'Body' },
],
});

generatePageMarkdownFiles(config);

const writtenContent = vi.mocked(writeFileSync).mock.calls[0][1] as string;
expect(writtenContent).toContain('title: "Path\\\\to\\\\file"');
});
});
8 changes: 6 additions & 2 deletions src/core/raw-markdown.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ function ensureDir(path: string): void {
mkdirSync(path, { recursive: true });
}

function escapeYamlString(s: string): string {
return s.replace(/\\/g, '\\\\').replace(/"/g, '\\"');
}

export function copyRawMarkdown(config: ResolvedAeoConfig): CopiedFile[] {
return copyMarkdownFiles(config);
}
Expand Down Expand Up @@ -89,8 +93,8 @@ export function generatePageMarkdownFiles(config: ResolvedAeoConfig): GeneratedM

// YAML frontmatter
lines.push('---');
if (pageTitle) lines.push(`title: "${pageTitle}"`);
if (page.description) lines.push(`description: "${page.description}"`);
if (pageTitle) lines.push(`title: "${escapeYamlString(pageTitle)}"`);
if (page.description) lines.push(`description: "${escapeYamlString(page.description)}"`);
lines.push(`url: ${pageUrl}`);
lines.push(`source: ${pageUrl}`);
lines.push(`generated_by: aeo.js`);
Expand Down
2 changes: 1 addition & 1 deletion src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { AeoConfig } from './types';

export const VERSION = '0.0.5';
export const VERSION = '0.0.11';

export function defineConfig(config: AeoConfig): AeoConfig {
return config;
Expand Down
5 changes: 4 additions & 1 deletion src/plugins/astro.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { generateAEOFiles } from '../core/generate';
import { resolveConfig } from '../core/utils';
import type { AeoConfig, PageEntry, ResolvedAeoConfig } from '../types';
import { join } from 'path';
import { join, resolve, sep } from 'path';
Copy link
Copy Markdown

@claude claude bot Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.

import { existsSync, mkdirSync, readFileSync, readdirSync, statSync, writeFileSync } from 'fs';
import { extractTextFromHtml, extractTitle, extractDescription, htmlToMarkdown } from '../core/html-extract';
import { generateSiteSchemas, generatePageSchemas, generateJsonLdScript } from '../core/schema';
Expand Down Expand Up @@ -305,6 +305,9 @@ if (!document.querySelector('meta[name="astro-view-transitions-enabled"]')) {

const filename = req.url.startsWith('/') ? req.url.slice(1) : req.url;

// Prevent path traversal attacks
if (filename.includes('..') || filename.includes('\0')) return next();

// Handwritten .md files in contentDir take priority
if (resolvedConfig.contentDir) {
const contentFile = join(process.cwd(), resolvedConfig.contentDir, filename);
Expand Down
5 changes: 4 additions & 1 deletion src/plugins/vite.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { extractTextFromHtml, extractTitle, extractDescription, htmlToMarkdown }
import { generatePageSchemas, generateSiteSchemas, generateJsonLdScript } from '../core/schema';
import { generateOGTagsHtml } from '../core/opengraph';
import type { AeoConfig, PageEntry } from '../types';
import { join, dirname } from 'path';
import { join, dirname, resolve, sep } from 'path';
import { readFileSync, readdirSync, statSync, existsSync } from 'fs';
import { fileURLToPath } from 'url';

Expand Down Expand Up @@ -93,6 +93,9 @@ export function aeoVitePlugin(options: AeoConfig = {}): any {

const filename = req.url.startsWith('/') ? req.url.slice(1) : req.url;

// Prevent path traversal attacks
if (filename.includes('..') || filename.includes('\0')) return next();

// Handwritten .md files in contentDir take priority
if (resolvedConfig.contentDir) {
const contentFile = join(process.cwd(), resolvedConfig.contentDir, filename);
Expand Down
146 changes: 146 additions & 0 deletions src/widget/core.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,5 +71,151 @@ describe('AeoWidget', () => {

expect(document.querySelector('.aeo-toggle')).toBeNull();
});

it('should remove keydown listener after destroy', () => {
widget = new AeoWidget({
config: {
title: 'Test',
url: 'https://test.com',
widget: { enabled: true },
},
});

const spy = vi.spyOn(document, 'removeEventListener');
widget.destroy();
widget = null;

expect(spy).toHaveBeenCalledWith('keydown', expect.any(Function));
spy.mockRestore();
});
});

describe('closeOverlay', () => {
it('should not cause stack overflow from mutual recursion', async () => {
widget = new AeoWidget({
config: {
title: 'Test',
url: 'https://test.com',
widget: { enabled: true },
},
});

// Click AI button to open overlay
const aiBtn = document.querySelector('.aeo-ai-btn') as HTMLElement;
aiBtn?.click();

// Wait for async overlay to appear
await vi.waitFor(() => {
expect(document.querySelector('.aeo-overlay')).not.toBeNull();
});

// Close overlay — if mutual recursion exists, this throws RangeError
expect(() => {
const closeBtn = document.querySelector('.aeo-close-btn') as HTMLElement;
closeBtn?.click();
}).not.toThrow();
});
});

describe('switchToAI', () => {
it('should show overlay when AI button is clicked', async () => {
widget = new AeoWidget({
config: {
title: 'Test',
url: 'https://test.com',
widget: { enabled: true },
},
});

const aiBtn = document.querySelector('.aeo-ai-btn') as HTMLElement;
aiBtn?.click();

await vi.waitFor(() => {
expect(document.querySelector('.aeo-overlay')).not.toBeNull();
});
});

it('should not create multiple overlays on rapid clicks', async () => {
widget = new AeoWidget({
config: {
title: 'Test',
url: 'https://test.com',
widget: { enabled: true },
},
});

const aiBtn = document.querySelector('.aeo-ai-btn') as HTMLElement;
aiBtn?.click();
aiBtn?.click();
aiBtn?.click();

await vi.waitFor(() => {
expect(document.querySelector('.aeo-overlay')).not.toBeNull();
});

const overlays = document.querySelectorAll('.aeo-overlay');
expect(overlays.length).toBe(1);
});
});

describe('switchToHuman', () => {
it('should close overlay when human button is clicked', async () => {
widget = new AeoWidget({
config: {
title: 'Test',
url: 'https://test.com',
widget: { enabled: true },
},
});

const aiBtn = document.querySelector('.aeo-ai-btn') as HTMLElement;
aiBtn?.click();

await vi.waitFor(() => {
expect(document.querySelector('.aeo-overlay')).not.toBeNull();
});

const humanBtn = document.querySelector('.aeo-human-btn') as HTMLElement;
humanBtn?.click();

expect(document.querySelector('.aeo-overlay')).toBeNull();
});
});

describe('keyboard events', () => {
it('should close overlay on Escape key', async () => {
widget = new AeoWidget({
config: {
title: 'Test',
url: 'https://test.com',
widget: { enabled: true },
},
});

const aiBtn = document.querySelector('.aeo-ai-btn') as HTMLElement;
aiBtn?.click();

await vi.waitFor(() => {
expect(document.querySelector('.aeo-overlay')).not.toBeNull();
});

document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape' }));

expect(document.querySelector('.aeo-overlay')).toBeNull();
});

it('should not throw when Escape is pressed without overlay', () => {
widget = new AeoWidget({
config: {
title: 'Test',
url: 'https://test.com',
widget: { enabled: true },
},
});

expect(() => {
document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape' }));
}).not.toThrow();
});
});
});
15 changes: 13 additions & 2 deletions src/widget/core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@ export class AeoWidget {
private config: AeoConfig;
private container: HTMLElement;
private isAIMode: boolean = false;
private isLoading: boolean = false;
private toggleElement?: HTMLElement;
private overlayElement?: HTMLElement;
private styleElement?: HTMLStyleElement;
private keydownHandler?: (e: KeyboardEvent) => void;

constructor(options: AeoWidgetOptions = {}) {
this.config = this.resolveConfig(options.config);
Expand Down Expand Up @@ -113,17 +115,21 @@ export class AeoWidget {
}
});

document.addEventListener('keydown', (e) => {
this.keydownHandler = (e: KeyboardEvent) => {
if (e.key === 'Escape' && this.overlayElement) {
this.closeOverlay();
}
});
};
document.addEventListener('keydown', this.keydownHandler);
}

private async switchToAI(): Promise<void> {
if (this.isLoading) return;
this.isLoading = true;
this.isAIMode = true;
this.updateToggleState();
await this.showOverlay();
this.isLoading = false;
}
Comment on lines 126 to 133
Copy link
Copy Markdown

@claude claude bot Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.


private switchToHuman(): void {
Comment on lines 115 to 135
Copy link
Copy Markdown

@claude claude bot Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.

Expand Down Expand Up @@ -496,6 +502,7 @@ export class AeoWidget {
this.overlayElement = undefined;
}
this.isAIMode = false;
this.isLoading = false;
this.updateToggleState();
}

Expand Down Expand Up @@ -541,6 +548,10 @@ export class AeoWidget {
}

public destroy(): void {
if (this.keydownHandler) {
document.removeEventListener('keydown', this.keydownHandler);
this.keydownHandler = undefined;
}
this.toggleElement?.remove();
this.overlayElement?.remove();
this.styleElement?.remove();
Expand Down