fix(security): 20 additional bugs found via sqry semantic code graph (round 2) by mr-k-man · Pull Request #806 · garrytan/gstack

mr-k-man · 2026-04-05T02:41:49Z

Summary

Second security audit of gstack's browse subsystem, Chrome extension, and CLI scripts — all findings beyond the 10 already reported in PR #664 and issues #665-#675.

20 vulnerabilities fixed across 13 files (1 HIGH, 1 MEDIUM-HIGH, 14 MEDIUM, 4 LOW)
119 regression tests (source-level + behavioral)
Method: sqry AST-based semantic code graph analysis + 4 parallel audit agents + 9 rounds of Codex plan review + mid-implementation code review

Findings by severity

#	Severity	Finding	File(s)
1	HIGH	Shell injection via bash-to-JS interpolation in `bin/gstack-learnings-search`	`bin/gstack-learnings-search`
2	MED-HIGH	Agent queue file poisoning — default permissions + no schema validation	`server.ts`, `sidebar-agent.ts`, `cli.ts`
3	MED	`/health` endpoint leaks `currentUrl` and `currentMessage` without auth	`server.ts`
4	MED	ReDoS via `new RegExp(args[1])` in `frame --url`	`meta-commands.ts`
5	MED	`chain` command bypasses watch-mode write guard	`meta-commands.ts`
6	MED	`cookie-import` allows cross-domain cookie planting (arbitrary `domain` field)	`write-commands.ts`
7	MED	CSS injection via `style` command value — no validation at 4 injection points	`write-commands.ts`, `cdp-inspector.ts`, `inspector.js`
8	MED	Session directory traversal via crafted `active.json` ID	`server.ts`
9	MED	`responsive` generated filenames skip `validateOutputPath`	`meta-commands.ts`
10	MED	`validateOutputPath` uses `path.resolve` not `realpathSync` — symlink bypass	`meta-commands.ts`, `write-commands.ts`, `snapshot.ts`
11	MED	`state load` restores cookies + navigates page URLs without validation	`meta-commands.ts`, `browser-manager.ts`
12	MED	`chatDomByTab` DOM serialization round-trip XSS in tab switch	`sidepanel.js`
13	MED	`switchChatTab` / `pollChat` mutual recursion (stack overflow)	`sidepanel.js`
14	MED	`cookie-import-browser --domain` accepts unvalidated domain	`write-commands.ts`
15	LOW-MED	Unsanitized `activeTabUrl` passed to `syncActiveTabByUrl` (2 call sites)	`server.ts`
16	LOW-MED	`inbox` outputs unvalidated user-controlled strings (prompt injection surface)	`meta-commands.ts`
17	LOW	Sidebar agent timeout handler missing SIGKILL escalation	`sidebar-agent.ts`
18	LOW	`viewport` accepts unbounded dimensions (OOM via multi-GB screenshots)	`write-commands.ts`
19	LOW	`wait` timeout has no upper bound (event loop block)	`write-commands.ts`
20	LOW	`stateFile` in queue entry missing path traversal check	`sidebar-agent.ts`

How we found them

Built sqry semantic code graph index (43,603 nodes, 36,355 edges, 188 files)
Queried for dangerous sinks (`exec`, `spawn`, `token`, `password`, `secret`), complexity hotspots, and call cycles
Dispatched 4 parallel audit agents targeting `server.ts`, `write-commands.ts`, `meta-commands.ts`, and `extension/` code
Cross-referenced all 25 raw findings against existing issues/PRs — 16 were genuinely new
Codex reviewed implementation plan through 9 rounds until unconditional APPROVE
Mid-implementation code review caught 4 additional gaps (all fixed)

Cross-validation with other community reports

7 of our original 10 findings from PR #664 were independently rediscovered by stedfn (#712-#717) and Gonzih (#741-#758). Several of these round 2 findings overlap with seanomich's audit (#783) but target different specific vectors.

Test results

Security regression tests (119/119 pass)

```
119 pass, 0 fail, 182 expect() calls
Ran 119 tests across 5 files [47ms]
```

E2E evals (33 pass, 0 regressions)

Ran in `gstack-ci` Docker image (Ubuntu 24.04 + Chromium 145/Playwright 1.58.2 + `--cap-add SYS_ADMIN`):

All previously-failing browse tests now pass:

Test	Result	Duration
browse-basic	PASS	12s
browse-snapshot	PASS	19s
qa-fix-loop	PASS	175s
browse qa-quick	PASS	84s
browse qa-only-no-fix	PASS	136s
browse qa-b6-static	PASS	111s
browse qa-b7-spa	PASS	158s
browse qa-b8-checkout	PASS	178s
sidebar-url-accuracy	PASS	<1s
skillmd-setup-discovery	PASS	6s
skillmd-no-local-binary	PASS	7s
skillmd-outside-git	PASS	5s
session-awareness	PASS	10s
operational-learning	PASS	7s
ship-local-workflow	PASS	15s
ship-coverage-audit	PASS	27s
document-release	PASS	46s
canary-workflow	PASS	34s
benchmark-workflow	PASS	49s
setup-deploy-workflow	PASS	18s
gstack-upgrade-happy-path	PASS	29s
codex-review	PASS	<1s
land-and-deploy (3 tests)	PASS	52-63s
review-army (6 tests)	PASS	11-53s
timeline-event-flow	PASS	<1s

1 unrelated failure (`qa-bootstrap` — test infrastructure, not our code).

Test plan

119 security regression tests pass (source-level + behavioral)
33 E2E evals pass including all browse/snapshot/QA tests
browse-basic, browse-snapshot, qa-fix-loop all pass (previously failing)
All existing path-validation, url-validation, and server-auth tests pass
No new regressions vs origin/main

…ymlink traversal Both meta-commands.ts and write-commands.ts validateOutputPath previously used path.resolve which resolves paths logically without following symlinks. A symlink at /tmp/safe pointing to /etc would pass the safe-directory check. Switched to fs.realpathSync (matching the existing validateReadPath pattern in read-commands.ts): - Resolve the full real path for existing files - Resolve the parent directory for new files (ENOENT case) - Also resolve SAFE_DIRECTORIES themselves through realpathSync (handles macOS /tmp → /private/tmp) Added browse/test/security-audit-r2.test.ts with source-level checks and behavioral tests including symlink-to-/etc attack verification.

Block data exfiltration via url(), expression(), @import, javascript:, and data: patterns in write-commands style handler, cdp-inspector modifyStyle, extension injectCSS (also validates id format), and extension toggleClass (validates className format). Adds source-level regression tests for all four injection points.

Replace direct bash variable interpolation into JS string literals with environment variable passing. Values are now passed via GSTACK_FILTER_* env vars and read with process.env inside the bun -e block, eliminating the shell injection vector where single quotes in --query/--type/--slug could break out of JS string literals and execute arbitrary code. Adds browse/test/learnings-injection.test.ts with source-level and behavioral tests to prevent regression.

…ions Add isValidQueueEntry() validator to sidebar-agent.ts that checks all 8 QueueEntry fields (prompt required, args array, cwd path traversal, typed optional fields). Invalid entries are skipped with console.warn. Queue directory and file permissions hardened to 0o700/0o600 in all three writers: server.ts, sidebar-agent.ts, and cli.ts. Tests added to security-audit-r2.test.ts.

- applyStyle in extension/inspector.js now validates CSS values with the same DANGEROUS_CSS pattern as injectCSS (blocks url(), expression(), @import, javascript:, data: exfiltration vectors) - snapshot.ts annotated screenshot path validation now uses realpathSync to resolve symlinks before checking against safe directories - cookie-import in write-commands.ts replaces inline path.resolve+isPathWithin with realpathSync-based validation consistent with validateOutputPath - isValidQueueEntry in sidebar-agent.ts now checks stateFile for '..' path traversal sequences in addition to type validation - Adds source-level regression tests for all four fixes in security-audit-r2.test.ts

…ted /health

…port-browser

… in tab switch

…ntrancy guard

…timeouts

sqry-release-plz bot added 16 commits April 5, 2026 02:00

fix(security): remove currentUrl and currentMessage from unauthentica…

dcdae09

…ted /health

fix(security): escape user input in frame --url to prevent ReDoS

4e8c644

fix(security): validate cookie domains in cookie-import and cookie-im…

ed7f169

…port-browser

fix(security): validate each responsive screenshot path, not just prefix

5cf8cad

fix(security): validate cookies and page URLs in state load

cec3d06

fix(security): validate activeTabUrl before syncActiveTabByUrl

2733610

fix(security): wrap inbox output as untrusted content

ff6b1b5

fix(security): replace DOM serialization round-trip with node cloning…

75161ad

… in tab switch

fix(security): break switchChatTab/pollChat mutual recursion with ree…

481b3f8

…ntrancy guard

fix(security): add SIGKILL escalation to sidebar-agent timeout handler

e966c75

fix(security): add bounds validation to viewport dimensions and wait …

c902fe2

…timeouts

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(security): 20 additional bugs found via sqry semantic code graph (round 2)#806

fix(security): 20 additional bugs found via sqry semantic code graph (round 2)#806
mr-k-man wants to merge 16 commits intogarrytan:mainfrom
mr-k-man:fix/security-audit-round2

mr-k-man commented Apr 5, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

mr-k-man commented Apr 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Findings by severity

How we found them

Cross-validation with other community reports

Test results

Security regression tests (119/119 pass)

E2E evals (33 pass, 0 regressions)

Test plan

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

mr-k-man commented Apr 5, 2026 •

edited

Loading