Skip to content

chore: sync core lib and CLAUDE.md from agent-core#14

Closed
avifenesh wants to merge 1 commit into
mainfrom
chore/sync-core-learn-20260426-130759
Closed

chore: sync core lib and CLAUDE.md from agent-core#14
avifenesh wants to merge 1 commit into
mainfrom
chore/sync-core-learn-20260426-130759

Conversation

@avifenesh
Copy link
Copy Markdown
Contributor

@avifenesh avifenesh commented Apr 26, 2026

Automated sync of lib/ and CLAUDE.md from agent-core.


Note

Medium Risk
Touches the runtime binary download/install path, adding checksum verification and new extraction logic (including a custom PowerShell zip extractor), which could cause install failures or platform-specific regressions if release assets or archive layouts differ.

Overview
Hardens the agent-analyzer runtime installer against supply-chain and archive-extraction attacks. Downloads now require a matching .sha256 sidecar (with an explicit local-dev-only skipChecksum escape hatch) and abort on mismatch before any extraction.

Extraction is reworked to use an isolated scratch directory with pre-validation of archive entry paths (rejecting absolute/UNC/drive-letter paths and .. traversal), post-extraction checks for symlinks/escape, and only the expected binary is copied into ~/.agent-sh/bin while all other extracted content is discarded. On Windows, zip handling switches from Expand-Archive command strings to a -File PowerShell helper script that validates entries and avoids quoting/space issues.

Adds a comprehensive node:test suite for checksum parsing/verification, path validation, scratch cleanup, and basic tar/zip extraction behavior, and exports several helper functions for testing/advanced use.

Reviewed by Cursor Bugbot for commit 647deaf. Configure here.

@avifenesh
Copy link
Copy Markdown
Contributor Author

Superseded by fresh sync after agent-core upstreamed missing files (workflow-state.js full version + lib/repo-intel/queries.js). Close older PR to avoid conflicts.

@avifenesh avifenesh closed this Apr 26, 2026
@avifenesh avifenesh deleted the chore/sync-core-learn-20260426-130759 branch April 26, 2026 13:12
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request implements security hardening for binary downloading and extraction, adding SHA-256 verification, path traversal protections, and isolated scratch directories. Feedback focuses on improving the robustness of the tar process by explicitly defining stdin for portability, using stdin.end(buf) for stream management, and correctly handling Buffer chunks to avoid character encoding issues.

Comment thread lib/binary/index.js
const tar = cp.spawn('tar', ['xz', '-C', tarDest], {
stdio: ['pipe', 'pipe', 'pipe']
});
const tar = cp.spawn('tar', ['-tz'], { stdio: ['pipe', 'pipe', 'pipe'] });
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Explicitly specifying -f - is more portable and ensures tar reads from stdin across different implementations (e.g., some versions of GNU tar might otherwise try to read from a default device).

Suggested change
const tar = cp.spawn('tar', ['-tz'], { stdio: ['pipe', 'pipe', 'pipe'] });
const tar = cp.spawn('tar', ['-tzf', '-'], { stdio: ['pipe', 'pipe', 'pipe'] });

Comment thread lib/binary/index.js
Comment on lines +306 to 307
tar.stdout.on('data', function(d) { stdout += d; });
tar.stderr.on('data', function(d) { stderr += d; });
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Appending a Buffer to a string relies on implicit conversion which can be problematic if a multi-byte character is split across chunks. It is safer to collect chunks and join them at the end.

    const stdoutChunks = [];
    const stderrChunks = [];
    tar.stdout.on('data', function(d) { stdoutChunks.push(d); });
    tar.stderr.on('data', function(d) { stderrChunks.push(d); });

Comment thread lib/binary/index.js
reject(new Error('tar -tz listing failed (code ' + code + '): ' + stderr));
return;
}
const entries = stdout.split(/\r?\n/).filter(function(l) { return l.length > 0; });
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Update this to use the collected chunks.

      const stdout = Buffer.concat(stdoutChunks).toString('utf8');
      const stderr = Buffer.concat(stderrChunks).toString('utf8');
      const entries = stdout.split(/\r?\n/).filter(function(l) { return l.length > 0; });

Comment thread lib/binary/index.js
Comment on lines +317 to +318
tar.stdin.write(buf);
tar.stdin.end();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Using tar.stdin.end(buf) is more concise and robust than separate write and end calls, as it handles the final write and stream closure in one operation.

Suggested change
tar.stdin.write(buf);
tar.stdin.end();
tar.stdin.end(buf);

Comment thread lib/binary/index.js

try {
await new Promise(function(resolve, reject) {
const tar = cp.spawn('tar', ['xz', '-C', scratch], { stdio: ['pipe', 'pipe', 'pipe'] });
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Similar to the listing command, explicitly specifying -f - is recommended for portability when piping the archive buffer to stdin.

Suggested change
const tar = cp.spawn('tar', ['xz', '-C', scratch], { stdio: ['pipe', 'pipe', 'pipe'] });
const tar = cp.spawn('tar', ['-xzf', '-', '-C', scratch], { stdio: ['pipe', 'pipe', 'pipe'] });

Comment thread lib/binary/index.js
Comment on lines +407 to +408
tar.stdin.write(buf);
tar.stdin.end();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Use tar.stdin.end(buf) for consistency and robustness.

Suggested change
tar.stdin.write(buf);
tar.stdin.end();
tar.stdin.end(buf);

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 647deaf. Configure here.

Comment thread lib/binary/index.js
const scratch = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-analyzer-zip-'));
const tmpZip = path.join(scratch, '__archive.zip');
const scriptDir = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-analyzer-ps-'));
const scriptPath = path.join(scriptDir, 'extract.ps1');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Scratch directory leaked if scriptDir creation fails

Low Severity

In extractZipToScratch, the scratch temp directory is created at line 503, then a second fs.mkdtempSync for scriptDir runs at line 505 — before the try block at line 508. If the second mkdtempSync throws (e.g. disk full), scratch is never cleaned up: the function's own catch block is never entered, and the caller's finally guard (if (scratch) rmrf(scratch)) also skips cleanup because the caller's scratch variable remains undefined. This leaks a temp directory. Contrast with extractTarGzToScratch, which correctly places mkdtempSync immediately before a try block.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 647deaf. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant