You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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)
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
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:
functionreadJsonDetailed(filePath,allowedBase){// Resolve to catch any symlink/.. tricksconstresolved=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:
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.
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".
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.
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
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.
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
npm test)src/schema.jsonupdated if new commands or flags were addedREADME.mdupdated if new top-level commands or categories were added.changeset/) — only required for user-facing changes (new commands, flags, bug fixes, breaking changes)