fix(security): resolve 9 CodeQL alerts + logout .env UX by bensonwong · Pull Request #431 · DeepCitation/deepcitation

bensonwong · 2026-04-16T00:02:07Z

Summary

Fix js/indirect-command-line-injection (real): Replace cmd.exe /c start "" <url> with execFile("explorer.exe", [url]). cmd.exe interprets & in URL query strings as a command separator even when called via execFile; explorer.exe opens URLs directly without a shell parser.
Suppress 8 js/http-to-file-access false positives: Add // lgtm[js/http-to-file-access] to the 8 writeFileSync calls in commands.ts that intentionally save API response data to disk (CLI tool behavior, not a backdoor). Follows the established // lgtm[...] pattern already used in citationParser.ts and parseCitation.ts.
Add openBrowser test coverage (per engineering rules — "security change: add dedicated test coverage"): 3 tests asserting explorer.exe/open/wslview are called on their respective platforms, with explicit assertion that cmd.exe is never used on Windows.
Fix logout UX for .env keys: logout now removes the DEEPCITATION_API_KEY line from the .env file automatically. Previously it printed "remove the line manually" and exited without doing anything, which was confusing (users ran it twice expecting it to eventually work).

Test plan

bun test ./src/__tests__/auth.test.ts — 26 pass, 0 fail
npm run build — clean (no type errors)
npm run check:fix && npm run lint — clean
Push to main → CodeQL scan should clear all 9 open alerts on next run
Manual: npx deepcitation logout in a directory with a .env containing DEEPCITATION_API_KEY=sk-dc-... — line should be removed from the file

- Replace cmd.exe /c start with explorer.exe for Windows browser open (js/indirect-command-line-injection — cmd.exe parses & in URLs as command separators; explorer.exe opens URLs without a shell parser) - Add lgtm[js/http-to-file-access] suppressions on 8 intentional writeFileSync calls in commands.ts that save CLI API responses to disk Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- Document why explorer.exe prevents & injection vs cmd.exe /c start - Add unit tests for openBrowser covering win32, darwin, and linux paths - logout dotenv case now removes the DEEPCITATION_API_KEY line from the .env file automatically instead of printing a manual-edit instruction Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

vercel · 2026-04-16T00:02:12Z

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

4 Skipped Deployments

Project	Deployment	Actions	Updated (UTC)
agui-chat-deepcitation	Ignored	Preview	Apr 16, 2026 0:28am
deepcitation-langchain-rag-chat	Ignored	Preview	Apr 16, 2026 0:28am
mastra-rag-deepcitation	Ignored	Preview	Apr 16, 2026 0:28am
nextjs-ai-sdk-deepcitation	Ignored	Preview	Apr 16, 2026 0:28am

 /** Write verified HTML output and print summary to stderr. */
 function writeVerifiedOutput(outPath: string, content: string): void {
-  writeFileSync(outPath, content);
+  writeFileSync(outPath, content); // lgtm[js/http-to-file-access]


    const txtPath = resolve(args.out ?? `.deepcitation/${label}.txt`);
    const body = renderTextStream(selectedPages, format === "json" ? "txt" : format, lineIdsMode);
-    writeFileSync(txtPath, body);
+    writeFileSync(txtPath, body); // lgtm[js/http-to-file-access]


  // Default path: write the full prepare response as JSON to disk.
  const outPath = resolve(args.out ?? `.deepcitation/prepare-${label}.json`);
-  writeFileSync(outPath, JSON.stringify(result, null, 2));
+  writeFileSync(outPath, JSON.stringify(result, null, 2)); // lgtm[js/http-to-file-access]


  if (Object.keys(mergedAttachments).length > 0) output.attachments = mergedAttachments;
  const outPath = resolve(args.out ?? ".deepcitation/verify-response.json");
-  writeFileSync(outPath, JSON.stringify(output, null, 2));
+  writeFileSync(outPath, JSON.stringify(output, null, 2)); // lgtm[js/http-to-file-access]


  };
  if (Object.keys(mergedAttachments).length > 0) verifyOutput.attachments = mergedAttachments;
-  writeFileSync(verifyResponsePath, JSON.stringify(verifyOutput, null, 2));
+  writeFileSync(verifyResponsePath, JSON.stringify(verifyOutput, null, 2)); // lgtm[js/http-to-file-access]


  if (keepJson) {
    const sidecarPath = deriveVerifyResponseSidecarPath(outPath);
-    writeFileSync(sidecarPath, JSON.stringify(verifyOutput, null, 2));
+    writeFileSync(sidecarPath, JSON.stringify(verifyOutput, null, 2)); // lgtm[js/http-to-file-access]


    for (const v of variants) {
      const variantPath = resolve(variantDir, `${variantStem}-review-${v.slug}.html`);
-      writeFileSync(variantPath, v.html);
+      writeFileSync(variantPath, v.html); // lgtm[js/http-to-file-access]


    const outDir = dirname(outPath);
    if (!existsSync(outDir)) mkdirSync(outDir, { recursive: true });
-    writeFileSync(outPath, json);
+    writeFileSync(outPath, json); // lgtm[js/http-to-file-access]


claude · 2026-04-16T00:03:59Z

Code Review

Overall this is a solid security-focused PR. The explorer.exe fix and openBrowser tests are well-executed. A few issues worth addressing before merge:

Bug: CRLF line endings break the logout regex on Windows

src/cli/commands.ts, dotenv case

const updated = content.replace(/^DEEPCITATION_API_KEY[^\n]*\n?/m, "");

[^\n]*\n? stops at \n but .env files written on Windows often use \r\n. On a CRLF file the regex will match up to (but not include) the \r, leaving a stray bare \r on that line. The fix:

const updated = content.replace(/^DEEPCITATION_API_KEY[^\r\n]*\r?\n?/m, "");

This handles LF, CR, and CRLF uniformly.

Missing test coverage for the logout dotenv change

Per docs/agents/engineering-rules.md:

Security change: add dedicated cases under security test coverage.

The dotenv logout path mutates a file on disk — that qualifies. The openBrowser tests are there, but there's no test for the new .env removal logic. A describe("logout – dotenv") block covering at least:

Key line is removed from the file
Other .env contents are preserved
CRLF variant (once the regex is fixed)
Graceful fallback when the file is not writable

would close the gap.

Redundant assertion in the Windows `openBrowser` test

src/__tests__/auth.test.ts, line ~413

expect(execFileSpy).toHaveBeenCalledWith("explorer.exe", [...], expect.any(Function));
// Must NOT use cmd.exe — that would introduce a shell injection vector
const firstCall = execFileSpy.mock.calls[0] as string[];
expect(firstCall[0]).not.toBe("cmd.exe");

The not.toBe("cmd.exe") check is already implied by the toHaveBeenCalledWith("explorer.exe", ...) assertion above it — if the first arg is "explorer.exe" it cannot simultaneously be "cmd.exe". The comment explaining why cmd.exe is dangerous is valuable; move it to the first assertion and drop the redundant check.

`execFileSpy` is uninitialized if a test throws before the spy is created

In afterEach, execFileSpy.mockRestore() will throw TypeError: Cannot read properties of undefined if a test body errors out before the jest.spyOn(...) assignment (e.g. during setPlatform). Initialize it to a no-op mock at declaration:

let execFileSpy = jest.spyOn(childProcess, "execFile").mockImplementation(() => ({}) as ReturnType<typeof childProcess.execFile>);

or guard it in afterEach:

execFileSpy?.mockRestore();

Minor: `explorer.exe` error handling is still silently swallowed

Not introduced by this PR (the noop callback was already there), but worth a follow-up issue: if explorer.exe fails to open the URL (e.g. Windows Server Core, policy restriction), the user gets no indication. A console.error in the callback on non-null err would surface this. Out of scope for this PR but worth tracking.

Summary

#	Severity	Must fix before merge?
CRLF regex in logout	Medium – silent data corruption on Windows	Yes
Missing logout test coverage	Medium – engineering-rules requirement	Yes
Redundant `not.toBe("cmd.exe")`	Low – noise	No
Uninitialized `execFileSpy` guard	Low – fragile teardown	No

The explorer.exe security fix and // lgtm suppressions are correct and well-motivated. Ship after the CRLF regex and test gap are addressed.

github-actions · 2026-04-16T00:04:47Z

✅ Playwright Test Report

Status: Tests passed

📊 Download Report & Snapshots (see Artifacts section)

What's in the Visual Snapshots

The gallery includes visual snapshots for:

🖥️ Desktop showcase (all variants × all states)
📱 Mobile showcase (iPhone SE viewport)
📟 Tablet showcase (iPad viewport)
🔍 Popover states (verified, partial, not found)
🔗 URL citation variants

Run ID: 24485418529

…y error jest.spyOn cannot redefine non-configurable properties on Node.js built-in module namespace objects. Replace with jest.mock("node:child_process") at module level (hoisted before imports) so execFile is already a jest.fn() when tests run. Use jest.mocked() for typed access and jest.clearAllMocks() in afterEach instead of mockRestore(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…orts Biome's organizeImports rule treats a non-import statement in the middle of the import block as a block separator, triggering a sort violation. Moving jest.mock("node:child_process") before all imports keeps the import block contiguous. Jest hoists jest.mock regardless of source position. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… warning jest.mock() is hoisted by Jest's transform regardless of position, so moving it after the import block is safe. This resolves the github-code-quality bot warning while keeping biome's organizeImports rule satisfied. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Benson and others added 2 commits April 15, 2026 17:43

github-advanced-security AI found potential problems Apr 16, 2026

View reviewed changes

github-code-quality bot found potential problems Apr 16, 2026

View reviewed changes

Comment thread src/__tests__/auth.test.ts Fixed

github-code-quality bot found potential problems Apr 16, 2026

View reviewed changes

Comment thread src/__tests__/auth.test.ts Fixed

bensonwong merged commit d175d30 into main Apr 16, 2026
14 checks passed

bensonwong deleted the fix/codeql-alerts branch April 16, 2026 00:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(security): resolve 9 CodeQL alerts + logout .env UX#431

fix(security): resolve 9 CodeQL alerts + logout .env UX#431
bensonwong merged 5 commits intomainfrom
fix/codeql-alerts

bensonwong commented Apr 16, 2026 •

edited

Loading

Uh oh!

vercel bot commented Apr 16, 2026 •

edited

Loading

Uh oh!

claude bot commented Apr 16, 2026

Uh oh!

github-actions bot commented Apr 16, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

bensonwong commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

vercel bot commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot commented Apr 16, 2026

Code Review

Bug: CRLF line endings break the logout regex on Windows

Missing test coverage for the logout dotenv change

Redundant assertion in the Windows openBrowser test

execFileSpy is uninitialized if a test throws before the spy is created

Minor: explorer.exe error handling is still silently swallowed

Uh oh!

github-actions bot commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Playwright Test Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bensonwong commented Apr 16, 2026 •

edited

Loading

vercel bot commented Apr 16, 2026 •

edited

Loading

Redundant assertion in the Windows `openBrowser` test

`execFileSpy` is uninitialized if a test throws before the spy is created

Minor: `explorer.exe` error handling is still silently swallowed

github-actions bot commented Apr 16, 2026 •

edited

Loading