Skip to content

[Aikido] AI Fix for Potential file inclusion attack via reading file - #492

Open
aikido-autofix[bot] wants to merge 1 commit into
mainfrom
fix/aikido-security-sast-88328555-9rug
Open

[Aikido] AI Fix for Potential file inclusion attack via reading file#492
aikido-autofix[bot] wants to merge 1 commit into
mainfrom
fix/aikido-security-sast-88328555-9rug

Conversation

@aikido-autofix

Copy link
Copy Markdown
Contributor

Summary

This patch mitigates potential file inclusion attack via reading file in the 'readJsonDetailed' function by rejecting file paths containing parent directory traversal sequences or absolute paths before performing file system read operations.

AI Confidence Level (confidence in the quality of this automated fix): Medium confidence: Aikido has validated similar fixes and observed positive outcomes. Validation is required.

Checklist

  • Tests pass (npm test)
  • src/schema.json updated if new commands or flags were added
  • README.md updated if new top-level commands or categories were added
  • Changeset added (.changeset/) — only required for user-facing changes (new commands, flags, bug fixes, breaking changes)

@nansen-pr-reviewer

Copy link
Copy Markdown

pr-reviewer Summary for #56b01c1

📝 3 findings

Review completed. Please address the findings below.

Findings by Severity

Severity Count
🔴 Critical 1
🟡 Medium 1
🔵 Low 1

Review effort: 2/5 (Simple)

Summary

This PR aims to mitigate a path-traversal risk in readJsonDetailed but introduces a critical regression that will silently break all JSON file reads in doctor.js. The .. check also has a correctness gap that leaves the security property incomplete. The fix needs to be reworked before merging.

Findings

src/doctor.js — lines 56–58

🔴 CRITICAL — path.isAbsolute() check breaks all file reads

if (filePath.includes('..') || path.isAbsolute(filePath)) {
  return { data: null, error: 'unreadable' };
}

Every single call to readJsonDetailed in this file passes an absolute path — they are all constructed with path.join(getConfigDir(env), ...) or similar, which always produces an absolute path once HOME is resolved. As a result, every call returns { data: null, error: 'unreadable' } immediately, meaning:

  • nansen auth status reports config as unreadable / no API key
  • nansen doctor reports all config files, wallet files, cost-map, update-check, and limit-order-auth as unreadable
  • Wallet enumeration returns zero wallets

The fix completely defeats the feature it was supposed to protect. The correct approach is to validate that the resolved path stays within the expected base directory, not to reject all absolute paths:

function readJsonDetailed(filePath, allowedBase) {
  // Resolve to catch any symlink/.. tricks
  const resolved = path.resolve(filePath);
  if (allowedBase && !resolved.startsWith(path.resolve(allowedBase) + path.sep)) {
    return { data: null, error: 'unreadable' };
  }
  // ... existing fs.readFileSync ...
}

Alternatively, check that the caller-supplied path doesn't escape its intended directory by comparing path.resolve(filePath) against the expected parent.

Action: Rework the guard — remove the path.isAbsolute() rejection and replace the .. string check with a path.resolve-based confinement check.


🟡 MEDIUM — filePath.includes('..') is ineffective against the real attack vector

Even without the isAbsolute regression, the .. string check is unreliable:

  1. path.join() (used at every call site) normalises .. sequences before they reach readJsonDetailed. A path like path.join(walletsDir, '../../../etc/passwd.json') becomes /home/user/.nansen/etc/passwd.json — the .. tokens are gone and the check is bypassed.
  2. Filenames that literally contain the substring .. (e.g. test..wallet.json) are incorrectly rejected even though they don't traverse directories.

The test comment in doctor.test.js acknowledges this ('...json' and 'test..json' are flagged as "may be rejected"), but this means the check doesn't actually prevent traversal via path.join arguments, which is the realistic attack vector.

Action: Replace filePath.includes('..') with a path.resolve(filePath).startsWith(resolvedExpectedBase) check so that traversal is caught regardless of whether .. tokens survive normalization.


🔵 LOW — Missing changeset

This is a user-facing security bug fix in a published npm package (nansen-cli). Per AGENTS.md/CLAUDE.md, a changeset file is required. No .changeset/*.md file was added in this PR.

Action: Add .changeset/<descriptive-name>.md with "nansen-cli": patch and a brief description such as "Fix path traversal vulnerability in readJsonDetailed".


Token usage: 14 input, 3,495 output, 165,263 cache read, 32,165 cache write | Usage Guide

New pushes are reviewed automatically with a 10-minute cooldown between reviews. To request a review at any time, comment @nansen-pr-reviewer re-review.

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.

0 participants