ClawHub
Back to skill

Security audit

Code Refactor

Security checks for vulnerabilities and agentic risk

Overview

This refactoring skill matches its stated purpose, but it can modify arbitrary files and run caller-configured shell commands without enough scoping or confirmation.

Review before installing. Use this only on trusted workspaces, prefer dry-run mode, inspect diffs before applying, and avoid passing custom testCommand values or running it against untrusted repositories until it enforces workspace path limits and safer test execution.

A.I.G

Vulnerability Patterns
  • Unauthorized Access and Privilege EscalationObtains permissions beyond the task's legitimate needs
  • Insecure Skill Coding PracticesFinds exploitable flaws such as hardcoded secrets or command injection
  • Skill Instruction HijackingAlters the agent's session goals or safety constraints when the skill loads
  • Agent Memory PoisoningWrites attacker-controlled rules into memory that affect later sessions
  • Remote Payload Retrieval and ExecutionFetches external code whose behavior can change after review
Findings (3)

T09 · Insecure Skill Coding Practices

Error
Location
src/validator.js:131
Finding

Configurable Shell Command Allows Arbitrary Command Execution

Content
View full analysis

Vulnerability Details

File Location: src/validator.js:12-15, 126-136
Vulnerability Type: OS command injection and execution of untrusted project scripts
Risk Level: High

Vulnerable Code

javascript
this.options = {
  testCommand: options.testCommand || 'npm test',
  coverageThreshold: options.coverageThreshold || 80,
  timeout: options.timeout || 60000,
  ...options
};
javascript
// Run tests
const output = execSync(this.options.testCommand, {
  cwd,
  encoding: 'utf-8',
  timeout: this.options.timeout,
  stdio: ['pipe', 'pipe', 'pipe']
});

Technical Analysis

testCommand is accepted through the public constructor options and passed as a single string to child_process.execSync. String commands executed through execSync are interpreted by a shell, so shell metacharacters, command substitutions, redirections, and command chaining operators contained in an attacker-influenced option can execute additional commands.

For example, a value conceptually equivalent to npm test; attacker-command would cause both commands to be interpreted by the shell. The exact payload available depends on the host operating system.

Even when the default npm test value is used, validation executes the test script from the package.json in context.cwd. A malicious or untrusted target project can therefore supply an arbitrary test script. No sandbox, privilege reduction, network restriction, environment filtering, or explicit approval boundary is applied before execution.

Attack Path

  1. An attacker causes the Agent to validate an untrusted project or influences the options passed to TestValidator.
  2. The attacker either:
    • supplies shell syntax in options.testCommand; or
    • places a malicious test script in the target project's package.json.
  3. validate() invokes runTests().
  4. runTests() executes the command through execSync in the selected ...[truncated 721 chars]
Remediation
View remediation

Remediation Suggestions

  1. Replace execSync with execFileSync or spawnSync, using a fixed executable and a separately constructed argument array.
  2. Do not accept an unrestricted shell command string. If customization is necessary, allow only predefined test runners and validated argument lists.
  3. Set shell: false explicitly and reject shell metacharacters rather than attempting to escape arbitrary shell input.
  4. Canonicalize and validate context.cwd, and require it to remain inside an approved workspace.
  5. Require explicit user approval before running scripts from an untrusted target project's package.json.
  6. Execute project tests in an isolated subprocess or container with:
    • a minimal environment-variable allowlist;
    • restricted filesystem mounts;
    • disabled or restricted network access;
    • resource and process limits;
    • a non-privileged operating-system identity.
  7. Add tests proving that command separators and command-substitution syntax cannot trigger secondary commands.

T05 · Unauthorized Access and Privilege Escalation

Warning
Location
src/applier.js:101
Finding

Unrestricted File Paths Permit Writes Outside the Intended Workspace

Content
View full analysis

Vulnerability Details

File Location: src/applier.js:101-114, 146, 252-257
Vulnerability Type: Arbitrary filesystem modification and unsafe backup handling
Risk Level: Medium

Vulnerable Code

javascript
async applyExtractConstant(change, context) {
  const filePath = context.filePath || change.filePath;
  
  if (!fs.existsSync(filePath)) {
    return {
      success: false,
      change: change.type,
      message: `File not found: ${filePath}`
    };
  }

  // Create backup
  if (this.options.backup) {
    const backupPath = `${filePath}.backup`;
    fs.copyFileSync(filePath, backupPath);
  }

  let code = fs.readFileSync(filePath, 'utf-8');
javascript
    fs.writeFileSync(filePath, updatedCode);
javascript
rollback(filePath) {
  const backupPath = `${filePath}.backup`;
  
  if (fs.existsSync(backupPath)) {
    fs.copyFileSync(backupPath, filePath);
    fs.unlinkSync(backupPath);
    return {
      success: true,
      message: `Rolled back changes to ${filePath}`
    };
  }

Technical Analysis

Public change and rollback operations accept caller-supplied file paths without resolving them against an approved project root. The implementation does not canonicalize paths, verify workspace containment, reject symbolic links, or confirm that the target and backup are regular files.

Consequently, absolute paths and traversal paths can select any file writable by the Agent account. A symbolic link can also redirect an apparently workspace-local path to a target outside the workspace.

Backups use a predictable adjacent name formed by appending .backup. copyFileSync overwrites an existing destination by default, so applying a change may destroy a pre-existing backup file. rollback() similarly trusts both paths and removes the backup after copying it.

Attack Path

  1. An attacker influences a change.filePath, `context.fileP ...[truncated 1030 chars]
Remediation
View remediation

Remediation Suggestions

  1. Require an explicit, trusted workspace root for every operation.
  2. Resolve both the workspace and target through fs.realpathSync, then enforce containment using path.relative.
  3. Reject absolute paths supplied by untrusted callers and reject resolved paths outside the workspace.
  4. Use lstat to reject symbolic links and verify that the target is a regular file.
  5. Create backups with unique, unpredictable names and exclusive creation flags so existing files cannot be overwritten.
  6. Store backups in a controlled directory rather than adjacent to arbitrary target files.
  7. Revalidate the target immediately before writing to reduce time-of-check/time-of-use risks.
  8. Write to a securely created temporary file and atomically rename it into place only after validation succeeds.
  9. Require explicit confirmation before modifying files and record the canonical target path in an audit log.
  10. Apply identical path controls to rollback(), analyze(), and all batch operations.

T09 · Insecure Skill Coding Practices

Warning
Location
src/applier.js:118
Finding

Global Textual Replacement Can Corrupt Source Code and Change Unrelated Values

Content
View full analysis

Vulnerability Details

File Location: src/applier.js:118-146
Vulnerability Type: Unsafe source-code transformation
Risk Level: Medium

Vulnerable Code

javascript
let code = fs.readFileSync(filePath, 'utf-8');
const lines = code.split('\n');

// Find and replace magic number with constant
const value = change.original.match(/:\s*(\d+)/)?.[1];
if (value) {
  const constName = this.generateConstantName(value);
  
  // Add constant declaration at top
  const constDeclaration = `const ${constName} = ${value};`;
  
  // Check if constant already exists
  if (!code.includes(constDeclaration)) {
    // Find first non-import line
    let insertIndex = 0;
    for (let i = 0; i < lines.length; i++) {
      if (!lines[i].trim().startsWith('import') && !lines[i].trim().startsWith('const') && lines[i].trim()) {
        insertIndex = i;
        break;
      }
    }
    lines.splice(insertIndex, 0, constDeclaration);
  }
  
  // Replace magic number with constant (simple replacement)
  const updatedCode = lines.join('\n').replace(
    new RegExp(`(?<![\\w'])(${value})(?![\\w'])`, 'g'),
    constName
  );
  
  fs.writeFileSync(filePath, updatedCode);
}

Technical Analysis

The implementation transforms JavaScript using a global regular-expression replacement rather than a parser. The replacement is not restricted to the issue's reported line and column. It can therefore replace matching digits in unrelated expressions, comments, string literals, template literals, regular-expression literals, property names, or semantically distinct values elsewhere in the file.

The constant declaration is inserted before the global replacement occurs. As a result, the declaration itself can be modified. For a detected value of 42, the inserted declaration:

javascript
const CONSTANT_42 = 42;

can become:

javascript
const CONSTANT_42 = 
...[truncated 1649 chars]
Remediation
View remediation

Remediation Suggestions

  1. Replace regular-expression source rewriting with an established JavaScript parser and AST transformation library.
  2. Transform only the numeric literal represented by the analyzer's exact source range.
  3. Never transform literals inside strings, comments, templates, regular expressions, or unrelated syntax nodes.
  4. Insert the declaration as a separate AST node so its initializer cannot be selected by the replacement pass.
  5. Detect naming collisions and choose a scope-safe identifier.
  6. Reparse the generated source before writing it.
  7. Run tests and semantic validation in isolation, and treat validation failure as an unsuccessful application.
  8. Generate a precise diff and require approval before committing the change.
  9. Write changes atomically and automatically restore the original file if parsing or validation fails.
  10. Add regression tests covering the constant initializer, comments, strings, template literals, repeated values, object keys, and unrelated numeric literals.

SkillSpector

Vulnerability Patterns
  • Data ExfiltrationExternal Transmission, Env Variable Harvesting, File System Enumeration
  • Privilege EscalationExcessive Permissions, Sudo/Root Execution, Credential Access
  • MCP Tool PoisoningHidden Instructions, Unicode Deception, Parameter Description Injection
  • Prompt InjectionInstruction Override, Hidden Instructions, Exfiltration Commands
  • Supply ChainUnpinned Dependencies, External Script Fetching, Obfuscated Code
Findings (11)

Natural-Language Policy Violations

Medium
Category
Not specified by scanner
Confidence
94% confidence
Finding

The primary descriptive text is written in Chinese and the file does not indicate that users may choose another language or that the skill is intentionally limited to a Chinese-speaking context. This creates a natural-language locale policy issue because the skill appears to impose a specific language without user opt-in.

Content

No source excerpt is available for this finding.

Intent-Code Divergence

Medium
Category
Not specified by scanner
Confidence
93% confidence
Finding

The docstring for applyRefactoringPlan says it 'applies a refactoring plan', implying code changes are performed. However, for extract-function, parameter-object, simplify-nesting, and reduce-complexity, the implementation only returns 'prepared' messages with requiresManualReview and performs no refactoring, creating a direct mismatch between the documentation and behavior.

Content

No source excerpt is available for this finding.

Missing User Warnings

Medium
Category
Not specified by scanner
Confidence
89% confidence
Finding

The code writes to arbitrary file paths from plan/context and silently creates backup and overwrite operations without any confirmation, path restriction, or policy check. In an agent skill context where plans may be influenced by untrusted inputs, this can enable unintended modification of sensitive project files or destructive changes outside the intended workspace.

Content

No source excerpt is available for this finding.

Missing User Warnings

Medium
Category
Not specified by scanner
Confidence
84% confidence
Finding

Rollback restores from and then deletes the backup file with no confirmation or retention policy. In an automation context, this can permanently remove recovery artifacts and make accidental or malicious rollback operations harder to detect or undo, increasing the risk of irreversible file-state changes.

Content

No source excerpt is available for this finding.

Intent-Code Divergence

Medium
Category
Not specified by scanner
Confidence
95% confidence
Finding

fullRefactor() accepts caller options but does not forward them into either the dry-run or apply phases, instead calling this.refactor(filePath, { dryRun: true/false }) with fresh option objects. This can silently disable expected safety controls or execution constraints supplied by the caller, causing the actual refactor/validation behavior to differ from what the caller requested; in particular, context-dependent safeguards, scoping, or execution settings may be omitted while rollback is the only option preserved.

Content

No source excerpt is available for this finding.

Missing User Warnings

Medium
Category
Not specified by scanner
Confidence
95% confidence
Finding

The validator executes a configurable shell command via execSync using this.options.testCommand, which can be influenced by callers through constructor options. If untrusted input reaches testCommand, an attacker can execute arbitrary OS commands in the current project directory with the privileges of the running process. In an agent skill context, this is especially dangerous because validation may be triggered automatically and without an interactive safety check.

Content

No source excerpt is available for this finding.

Natural-Language Policy Violations

Low
Category
Not specified by scanner
Confidence
84% confidence
Finding

The package description is written only in Chinese, indicating a language-specific presentation with no visible opt-in or alternative language option. Under the policy, language constraints should either offer user choice or be clearly justified as region-specific.

Content

No source excerpt is available for this finding.

Natural-Language Policy Violations

Low
Category
Not specified by scanner
Confidence
91% confidence
Finding

The header comment includes Chinese descriptions ("代码分析器", "检测代码异味、计算复杂度指标") alongside English, which imposes a specific language on readers without any opt-in or explanation. Under the policy, locale or language constraints should either be optional for the user or clearly justified.

Content

No source excerpt is available for this finding.

Intent-Code Divergence

Low
Category
Not specified by scanner
Confidence
74% confidence
Finding

The inline comment 'Create backup' appears within only the extract-constant path, while the class as a whole presents itself as a general change applier with rollback support. Since other change handlers report success without creating backups or making changes, the documented intent around rollback/backup behavior is inconsistent with the broader implementation.

Content

No source excerpt is available for this finding.

Natural-Language Policy Violations

Low
Category
Not specified by scanner
Confidence
88% confidence
Finding

This code mixes English identifiers with Chinese-only descriptive comments and JSDoc text such as '生成重构建议' and '生成重构方案'. The file provides no indication that language choice is configurable or intentionally limited to a region-specific audience, which can violate a language/locale policy requiring user opt-in or documented justification.

Content

No source excerpt is available for this finding.

Natural-Language Policy Violations

Low
Category
Not specified by scanner
Confidence
84% confidence
Finding

The file's human-readable descriptions and comments are predominantly in Chinese, which imposes a specific language choice in the skill's natural-language interface without any indication of user preference or locale justification. Under the stated policy, forcing a specific language without opt-in can be a locale-policy violation.

Content

No source excerpt is available for this finding.

Static analysis

Detected: suspicious.dangerous_exec, suspicious.dynamic_code_execution

Shell command execution detected (child_process).

Critical
Code
suspicious.dangerous_exec
Location
src/validator.js:131

Dynamic code execution detected.

Critical
Code
suspicious.dynamic_code_execution
Location
src/validator.js:99