Skip to content

fix: support npm 12 pack metadata - #18

Merged
Esquetta merged 1 commit into
mainfrom
fix/npm12-pack-json
Jul 21, 2026
Merged

fix: support npm 12 pack metadata#18
Esquetta merged 1 commit into
mainfrom
fix/npm12-pack-json

Conversation

@Esquetta

Copy link
Copy Markdown
Owner

Summary

  • normalize both npm 10/11 array and npm 12 keyed-object pack JSON
  • select the first valid tarball metadata record
  • preserve explicit failure for malformed output

Verification

  • npm 12 integration and parser tests: 4 passed
  • TypeScript check passed
  • git diff --check passed

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix npm pack JSON parsing for npm 12 metadata format

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Normalize npm pack JSON across npm 10/11 arrays and npm 12 keyed objects.
• Select the first pack entry containing a tarball filename.
• Add parser tests and preserve explicit failure when no tarball is returned.
Diagram

graph TD
  A(["doctor npm command"]) --> B["packNpmPackage()"] --> C{{"npm CLI: npm pack --json"}} --> D(["parseNpmPackOutput()"])
  D --> E(["select filename entry"])
  E --> F["tarball .tgz path"]

  subgraph Legend
    direction LR
    _cmd(["Command/Function"]) ~~~ _mod["Module/Data"] ~~~ _ext{{"External"}}
  end
Loading
High-Level Assessment

The chosen approach (normalizing both known npm pack JSON shapes into a single entry list, then selecting the first entry with a filename) is the most direct and robust compatibility fix. Alternative strategies like npm-version detection or stricter schema validation would add complexity without clear additional benefit for this use case.

Files changed (2) +41 / -3

Bug fix (1) +21 / -3
npm-package-doctor.tsNormalize npm pack JSON and pick first tarball entry +21/-3

Normalize npm pack JSON and pick first tarball entry

• Exports NpmPackEntry and introduces parseNpmPackOutput() to normalize npm pack JSON output from either an array (npm 10/11) or a keyed object (npm 12). Updates packNpmPackage() to use the parser and to select the first entry containing a string filename, preserving the existing error when no tarball is produced.

src/core/npm-package-doctor.ts

Tests (1) +20 / -0
npm-command.test.tsAdd unit tests for npm pack output normalization +20/-0

Add unit tests for npm pack output normalization

• Adds tests asserting that parseNpmPackOutput() accepts both array and keyed-object JSON shapes and filters out non-record shapes (null, primitives, non-object array entries). This validates compatibility behavior across npm 10–12 output formats.

tests/npm-command.test.ts

@Esquetta
Esquetta merged commit 2dd309c into main Jul 21, 2026
2 checks passed
@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Tarball path unvalidated 🐞 Bug ☼ Reliability
Description
packNpmPackage() now selects the first parsed record with any string filename and builds
tarballPath via path.join() without validating that it is a .tgz under destinationPath. That
path is then passed into extractTarGz() for gunzip+tar extraction, so unexpected output (e.g.,
../something or a non-tarball filename) can cause confusing failures or extraction attempts
outside the intended workspace.
Code

src/core/npm-package-doctor.ts[R229-233]

+  const packEntries = parseNpmPackOutput(stdout);
+  const metadata = packEntries.find((entry) => typeof entry.filename === "string");

  if (!metadata?.filename) {
    throw new Error(`npm pack did not return a tarball for ${packageSpec}`);
Evidence
The PR changed selection to find() any string filename and then uses it to form tarballPath,
which is subsequently gunzipped and extracted; without .tgz/containment checks, unexpected
filename values can cause extraction failures or attempt to read outside the intended destination
directory.

src/core/npm-package-doctor.ts[207-242]
src/core/npm-package-doctor.ts[148-183]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`packNpmPackage()` accepts any string `metadata.filename` and turns it into `tarballPath` without verifying that it is a tarball path located within `destinationPath`. That path is then immediately gunzipped/extracted.

### Issue Context
`npm pack --json` output is treated as trusted, but this code is parsing external command output. Defensive validation should ensure we only attempt extraction on a real tarball created in the requested destination directory.

### Fix Focus Areas
- src/core/npm-package-doctor.ts[207-242]

### Recommended fix
1. Tighten tarball selection:
  - Prefer entries where `filename` is a string and ends with `.tgz`.
2. Resolve and validate the tarball path:
  - Compute `const candidatePath = path.isAbsolute(filename) ? filename : path.resolve(destinationPath, filename)`.
  - Reject if `!isPathWithinRoot(destinationPath, candidatePath)`.
  - Optionally `await stat(candidatePath)` and ensure it’s a file to fail early with a clearer error.
3. Use `candidatePath` as `tarballPath`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

);
const packEntries = JSON.parse(stdout) as NpmPackEntry[];
const metadata = packEntries[0];
const packEntries = parseNpmPackOutput(stdout);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

1. Tarball path unvalidated 🐞 Bug ☼ Reliability

packNpmPackage() now selects the first parsed record with any string filename and builds
tarballPath via path.join() without validating that it is a .tgz under destinationPath. That
path is then passed into extractTarGz() for gunzip+tar extraction, so unexpected output (e.g.,
../something or a non-tarball filename) can cause confusing failures or extraction attempts
outside the intended workspace.
Agent Prompt
### Issue description
`packNpmPackage()` accepts any string `metadata.filename` and turns it into `tarballPath` without verifying that it is a tarball path located within `destinationPath`. That path is then immediately gunzipped/extracted.

### Issue Context
`npm pack --json` output is treated as trusted, but this code is parsing external command output. Defensive validation should ensure we only attempt extraction on a real tarball created in the requested destination directory.

### Fix Focus Areas
- src/core/npm-package-doctor.ts[207-242]

### Recommended fix
1. Tighten tarball selection:
   - Prefer entries where `filename` is a string and ends with `.tgz`.
2. Resolve and validate the tarball path:
   - Compute `const candidatePath = path.isAbsolute(filename) ? filename : path.resolve(destinationPath, filename)`.
   - Reject if `!isPathWithinRoot(destinationPath, candidatePath)`.
   - Optionally `await stat(candidatePath)` and ensure it’s a file to fail early with a clearer error.
3. Use `candidatePath` as `tarballPath`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

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