chore(deps): update express from 4.21.2 to 5.2.1#8
Open
yiliang114 wants to merge 1 commit into
Open
Conversation
[email protected] was a stale peer dependency residual at the top-level node_modules. It was originally pulled in to satisfy express-rate-limit's peerDependency "express >= 4.11" when express@4 was still the latest tag. Since express@5 is now latest and equally satisfies ">= 4.11", updating removes ~1200 lines of unused express@4 dependency tree from the lockfile. Closes QwenLM#4457
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Qwen Code review did not complete successfully (it may have been superseded by a newer review request). See workflow logs. |
Owner
Author
|
@qwen-code /review |
|
Qwen Code review did not complete successfully (it may have been superseded by a newer review request). See workflow logs. |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // macOS/Linux: All VSCode-like IDEs (VSCode, Cursor, Windsurf, etc.) | ||
| // are Electron-based, so we always need ELECTRON_RUN_AS_NODE=1 | ||
| // to run Node.js scripts using the IDE's bundled runtime. | ||
| const quotePosix = (s: string) => `"${s.replace(/"/g, '\\"')}"`; |
| // system-prompt context (output isn't rendered as HTML), but leaving | ||
| // them would waste tokens and trip static analyzers (CodeQL flags | ||
| // "incomplete multi-character sanitization" without this step). | ||
| return result.replace(/<!--/g, ''); |
Comment on lines
+46
to
+48
| const sanitized = suffix | ||
| .replace(/[^a-zA-Z0-9._-]+/g, '-') | ||
| .replace(/^-+|-+$/g, ''); |
Comment on lines
+55
to
+59
| return str.replace(regex, (match, key) => | ||
| context[key as keyof VariableContext] == null | ||
| ? match | ||
| : (context[key as keyof VariableContext] as string), | ||
| ); |
Comment on lines
+125
to
+129
| const processedText = text.replace( | ||
| /--- Content from referenced files ---\n([\s\S]*?)\n--- End of content ---/g, | ||
| (match, content) => | ||
| `\n> **Referenced Files:**\n\n${createCodeBlock(content)}\n`, | ||
| ); |
| buildArgs += `-f ${path.resolve(projectSandboxDockerfile)} -i ${image}`; | ||
| } | ||
| execSync( | ||
| `cd ${gcRoot} && node scripts/build_sandbox.js -s ${buildArgs}`, |
| case 'emacs': | ||
| return { | ||
| command: executable, | ||
| args: [filePath], |
| case 'zed': { | ||
| return { | ||
| command: executable, | ||
| args: [filePath, '--wait'], |
| // Validate that the output is parseable before writing to disk. | ||
| // This prevents corrupted settings files that would block startup. | ||
| try { | ||
| parse(updatedContent); |
| // Fallback to 'bash' and let the system handle it | ||
| cachedBashPath = 'bash'; | ||
| return 'bash'; | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Qwen Code Review — PR #8
Reviewed by: Qwen Code (qwen3.7-max) • Files changed: 2,710 • Scope: Express 4→5 migration, HTTP ACP bridge, filesystem abstraction, device flow auth
Summary
The Express 4→5 migration is clean — no breaking-change violations (res.send(status,body), app.del, query parser changes) were found. Async error propagation, SSE lifecycle management, and type safety are all correct under Express 5 semantics.
The security posture is solid: timing-safe bearer auth, comprehensive path traversal protection via resolveWithinWorkspace, CORS browser-origin rejection, and host allowlist enforcement.
10 findings across security, code quality, performance, and test coverage. Three acknowledged issues (TOCTOU in initWorkspace, model-switch timeout, errorPayload forwarding) already have tracked follow-ups.
Findings
| # | Area | Severity | File | Issue |
|---|---|---|---|---|
| 1 | Security | Medium | workspaceFileRead.ts:104 |
sendFsError leaks raw err.message (internal paths) on unexpected 500 errors |
| 2 | Code Quality | Medium | server.ts:1859-1887 |
Global error handler responses lack code field for SDK branching |
| 3 | Code Quality | Medium | httpAcpBridge.ts:3640 |
TOCTOU race between lstat symlink reject and writeFile in initWorkspace (acknowledged, Stage-2 follow-up) |
| 4 | Code Quality | Medium | httpAcpBridge.ts:3310 |
Model-switch timeout can produce contradictory model_switch_failed → model_switched SSE events (acknowledged, FIXME stage-2) |
| 5 | Performance | Medium | workspaceFileSystem.ts:603 |
Glob materializes ALL matches into memory before applying the maxResults cap |
| 6 | Performance | Medium | workspaceFileSystem.ts:628,665 |
Glob post-processing issues realpath + lstat (2 syscalls) per match hit |
| 7 | Performance | Medium | workspaceFileSystem.ts:1195-1234 |
readText reads/hashes entire file even for small line-window requests |
| 8 | Test Coverage | High | httpAcpBridge.ts:622 |
Permission timeout auto-cancellation has zero test coverage |
| 9 | Test Coverage | High | httpAcpBridge.ts:547 |
Per-session pending permission cap (maxPendingPerSession) has zero test coverage |
| 10 | Test Coverage | High | server.ts:1859 |
SyntaxError → 400 branch of error handler has no test |
Inline comments below provide details and suggested fixes for each finding.
| ); | ||
| res.status(500).json({ | ||
| errorKind: 'internal_error', | ||
| error: err instanceof Error ? err.message : String(err), |
There was a problem hiding this comment.
[Security · Medium] sendFsError sends raw err.message to the client on unexpected 500 errors. Node.js filesystem errors include full absolute paths (e.g., EACCES: permission denied, open '/etc/shadow'), which leaks internal filesystem layout to authenticated clients.
The app-level error handler in server.ts:1887 correctly returns a generic 'Internal server error' for 500s — this function bypasses that pattern.
The raw error is already logged to stderr, so operators retain full diagnostics.
Suggested fix:
res.status(500).json({
errorKind: 'internal_error',
error: 'Internal server error',
status: 500,
});Note: this function is also used by workspaceFileWrite.ts, so the leak surface includes both read and write routes.
| 'status' in err && | ||
| (err as { status: number }).status === 400 | ||
| ) { | ||
| res.status(400).json({ error: 'Invalid JSON in request body' }); |
There was a problem hiding this comment.
[Code Quality · Medium] The global error handler returns { error: string } for the 400, 413, and 500 branches — no code field. Every other error path in the codebase (sendBridgeError, sendFsError, route-level errors) includes a machine-readable code or errorKind field for SDK client branching.
SDK clients that implement a single error-parser cannot distinguish these three responses programmatically.
Suggested fix: Add code fields:
// 400:
res.status(400).json({ error: 'Invalid JSON in request body', code: 'invalid_json' });
// 413:
res.status(413).json({ error: 'Request body too large (max 10 MB)', code: 'payload_too_large' });
// 500:
res.status(500).json({ error: 'Internal server error', code: 'internal_error' });| // chain-aware resolution + audit hooks once `initWorkspace` | ||
| // routes through that boundary (tracked as a follow-up). | ||
| try { | ||
| const lst = await fs.lstat(target); |
There was a problem hiding this comment.
[Code Quality · Medium] TOCTOU race: lstat(target) rejects symlinks, then writeFile(target, '', 'utf8') follows symlinks. Between the two calls, a local process could replace the regular file with a symlink pointing outside the workspace, and writeFile would follow it.
The force: true path is especially concerning — it would truncate the external symlink target.
Suggested fix: Use fs.open(target, O_WRONLY | O_CREAT | O_TRUNC | O_NOFOLLOW) to atomically open without following symlinks. On Linux, O_NOFOLLOW causes open() to fail with ELOOP if the target is a symlink.
(Acknowledged in code as a Stage-1 limitation with tracked follow-up — flagging for visibility.)
| Promise.race([ | ||
| withTimeout( | ||
| conn.unstable_setSessionModel(normalized), | ||
| initTimeoutMs, |
There was a problem hiding this comment.
[Code Quality · Medium] When unstable_setSessionModel exceeds initTimeoutMs, withTimeout rejects but does NOT cancel the underlying ACP call. The bridge publishes model_switch_failed to SSE subscribers. If the agent subsequently completes, a model_switched event follows — creating a contradictory sequence for subscribers.
Suggested fix: When the timeout fires, set a flag. If a late model_switched notification arrives for a timed-out switch, publish a distinct model_switch_late_success frame (or suppress it entirely and let the HTTP caller retry).
(Acknowledged as FIXME(stage-2) — flagging for visibility.)
| // `loadIgnoreRules` defaults (which already include `.git` | ||
| // as a default ignore dir). | ||
| const matches = await globAsync(pattern, { | ||
| cwd: cwdReal, |
There was a problem hiding this comment.
[Performance · Medium] globAsync returns ALL matching paths as an array before the maxResults cap is applied in the post-walk loop (line 615). For a pattern like **/* on a large workspace (100K+ files outside node_modules/.git), this materializes the entire match set into memory.
Suggested fix: Use the streaming Glob class from the glob library (new Glob(pattern, opts)) which yields matches lazily via async iterator. Stop iterating once maxResults + 1 (for the truncation probe) is reached. This stops the directory walk early and avoids the full allocation.
| // already realpath'd the hit, so an extra `lstat` here is | ||
| // cheap; on `lstat` failure (raced unlink) we conservatively | ||
| // treat the hit as a file so the file-pattern check still | ||
| // runs. |
There was a problem hiding this comment.
[Performance · Medium] Each glob match incurs 2 sequential async syscalls: fsp.realpath(absolute) (line 628) + fsp.lstat(canonical) (here). With the default maxResults of 5000, that's 10,000 syscalls serializing through the libuv thread pool.
The lstat determines file-vs-directory kind for the ignore filter. The glob library's withFileTypes: true option would provide Dirent objects directly, eliminating the per-hit lstat.
Suggested fix: Pass withFileTypes: true to globAsync and use the returned Dirent objects for kind detection. If combined with the streaming fix above, pipeline realpath calls with bounded concurrency (e.g., p-limit of 4-8).
| throw new FsError('symlink_escape', `path is a symlink: ${p}`, { | ||
| hint: 're-resolve the target file instead of reading through a link', | ||
| }); | ||
| } |
There was a problem hiding this comment.
[Performance · Medium] readTextSnapshotFromResolvedFile reads the entire file (up to 256KB via readStableRegularFileBuffer), decodes it, computes SHA-256 synchronously on the event loop (hashBuffer(raw) at line ~1234), and only THEN slices the requested line window.
For line-windowed reads (?line=5&limit=10), this is 5-10x more I/O and CPU than needed. Under concurrent reads, the synchronous hashBuffer blocks the event loop (~0.5ms per 256KB buffer), delaying SSE heartbeats.
Suggested fix: For line-windowed reads, consider reading only enough bytes to satisfy the line range. If the full-file hash is needed for response metadata, use the streaming hashRegularFileAtPath (which uses a 64KB reusable buffer) instead of hashBuffer on the already-allocated full buffer.
| // When the deadline fires, roll back the pending (so a late | ||
| // vote returns 404) and resolve as cancelled (unwinding the | ||
| // agent's awaiting promise so the per-session FIFO can drain). | ||
| if (this.permissionTimeoutMs > 0) { |
There was a problem hiding this comment.
[Test Coverage · High] The permission timeout mechanism (lines 622-636) has zero test coverage. This code involves setTimeout, unref(), a settled flag guard against double-resolution, and rollbackPending — all state-management logic where a silent bug would manifest as permissions hanging forever (memory leak + blocked FIFO) or late votes succeeding after rollback (inconsistent state).
Suggested test: Create a bridge with permissionTimeoutMs: 50. Issue a permission request, do NOT vote. Assert the permission resolves as { outcome: 'cancelled' } within ~100ms. Then vote on the same request ID and assert 404 (confirming rollback removed the pending entry).
|
|
||
| // Bd1z5: per-session cap. Reject before registering so we never | ||
| // grow `pendingPermissionIds` past the limit. | ||
| if (entry.pendingPermissionIds.size >= this.maxPendingPerSession) { |
There was a problem hiding this comment.
[Test Coverage · High] The maxPendingPerSession cap (lines 547-555) has zero test coverage. If the size comparison is off-by-one or the early-return path fails to prevent registration, the pending set could grow unbounded (memory leak under a chatty agent).
Suggested test: Create a bridge with maxPendingPerSession: 2. Issue 2 permission requests without voting. Issue a 3rd and assert it resolves as { outcome: 'cancelled' } while the first 2 remain pending. Vote on one, issue a 4th — assert it accepts (confirming the set decremented).
| 'status' in err && | ||
| (err as { status: number }).status === 400 | ||
| ) { | ||
| res.status(400).json({ error: 'Invalid JSON in request body' }); |
There was a problem hiding this comment.
[Test Coverage · High] The SyntaxError → 400 branch of the error handler has no test. The 413 branch IS tested (server.test.ts:3342), but this branch is not. If the instanceof SyntaxError check or status === 400 guard is accidentally altered, clients receive a 500 or an Express HTML error page instead of the expected JSON — breaking SDK clients.
Suggested test: Send a POST with body {not valid json and Content-Type: application/json. Assert status 400 and response body {"error": "Invalid JSON in request body"}.
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test PR - large (1 file, +237/-1436)