From 0e5d2724005d6102fbb6482cb449be84af9fcbbb Mon Sep 17 00:00:00 2001 From: Eugene Kalinin Date: Mon, 28 Sep 2026 21:54:44 +0300 Subject: [PATCH 1/2] chore(ci): smoke-test --with-npm on windows-latest --with-npm takes install_npm_win on Windows, a path the ubuntu integration job never reaches, so a broken npm there went unnoticed (#310). The new job builds an environment with the command from the issue (--node=17.4.0 --npm=8.3.1) and with the default --npm=latest, then runs activate.bat and `npm install` in a project, as the reporter did. --- .github/workflows/tests.yml | 23 ++++++++++++++++ tests/nodeenv_test.py | 52 +++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 490e707..449c8ef 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -119,6 +119,29 @@ jobs: export NODEENV_GIT_BASH="$(cygpath -w "$BASH")" pytest -m integration -k git_bash tests/ -v + # --with-npm installs npm another way on Windows (install_npm_win), one + # the ubuntu integration job never takes - issue #310 + with-npm-windows: + runs-on: windows-latest + timeout-minutes: 10 + + steps: + - uses: actions/checkout@v4 + + - name: Set up Python 3.14 + uses: actions/setup-python@v5 + with: + python-version: '3.14' + + - name: Install dependencies + run: | + python -m pip install --upgrade pip + pip install -r requirements-dev.txt + + - name: Run --with-npm integration test + run: | + pytest -m integration -k with_npm tests/ -v + coverage: runs-on: ubuntu-latest steps: diff --git a/tests/nodeenv_test.py b/tests/nodeenv_test.py index 461a6ac..602981d 100644 --- a/tests/nodeenv_test.py +++ b/tests/nodeenv_test.py @@ -7,6 +7,7 @@ else: from shlex import quote as _quote import io +import json import os.path import pathlib import shutil @@ -168,6 +169,57 @@ def test_smoke_git_bash(tmpdir): 'npm root -g is %s, outside %s' % (npm_root, nenv_path) +@pytest.mark.integration +@pytest.mark.skipif( + sys.platform != 'win32', reason='install_npm_win only runs on Windows') +@pytest.mark.parametrize('versions', ( + # the command from the issue + ['--node=17.4.0', '--npm=8.3.1'], + # --npm defaults to latest + [], +), ids=('issue', 'default')) +def test_smoke_with_npm_win(tmpdir, versions): + """ + The npm --with-npm installs on Windows has to install packages, not + only answer `npm --version`. + https://github.com/ekalinin/nodeenv/issues/310 + """ + nenv_path = tmpdir.join('nenv').strpath + subprocess.check_call([ + 'coverage', 'run', '-p', + '-m', 'nodeenv', '--prebuilt', '--with-npm', + ] + versions + [nenv_path]) + + # the steps of the issue: activate.bat, then npm in a project + project = tmpdir.mkdir('project') + project.join('package.json').write('{"name": "p", "version": "1.0.0"}') + probe = tmpdir.join('probe.bat') + probe.write( + '@echo off\n' + 'call "%s\\Scripts\\activate.bat"\n' + 'call npm --version > npm-version.txt\n' + 'if errorlevel 1 exit /b 1\n' + 'call npm install is-number --no-audit --no-fund\n' % nenv_path) + proc = subprocess.run( + ['cmd', '/c', probe.strpath], cwd=project.strpath, + stdout=subprocess.PIPE, stderr=subprocess.PIPE) + report = 'exit %s\n--- stdout ---\n%s\n--- stderr ---\n%s' % ( + proc.returncode, + proc.stdout.decode('utf-8', 'replace'), + proc.stderr.decode('utf-8', 'replace')) + + assert proc.returncode == 0, report + # the npm that ran is the one in the environment, not one the runner + # has on PATH + npm_package = os.path.join( + nenv_path, 'Scripts', 'node_modules', 'npm', 'package.json') + with open(npm_package) as f: + installed = json.load(f)['version'] + assert project.join('npm-version.txt').read().strip() == installed, \ + report + assert project.join('node_modules', 'is-number').check(dir=1), report + + @pytest.mark.integration @pytest.mark.skipif(sys.platform == 'win32', reason='-n system is posix only') def test_smoke_n_system_special_chars(tmpdir): From 8b67db33e04712c4771723eea7b170a701b2756a Mon Sep 17 00:00:00 2001 From: Eugene Kalinin Date: Mon, 28 Sep 2026 21:50:40 +0300 Subject: [PATCH 2/2] fix(nodeenv): install npm from the registry tarball on Windows install_npm_win downloaded github.com/npm/cli/archive/v.zip, the source repository of npm rather than the published package. That tree is not an installable npm: - npm 8.x links its workspaces into node_modules with symlinks, and zipfile.extractall writes them as text files holding the link target, so npm dies with "Unexpected token '.'" on node_modules/libnpmfund - npm >= 9 has no workspace packages in node_modules at all, so even `npm --version` fails with "Cannot find module '@npmcli/config'" - the default --npm=latest asked for archive/vlatest.zip, a 404 Resolve the version or dist-tag through registry.npmjs.org and unpack the tarball its metadata points to, with filter='data' as for the node archive. The tarball carries bin/npm too, so the Cygwin branch copies it instead of fetching it from raw.githubusercontent.com. The mock-based TestInstallNpmWin tests pinned the GitHub URL and zipfile calls; they now run the real unpacking against a fake registry. Fixes #310 --- CHANGES | 5 + nodeenv.py | 24 ++- tests/nodeenv_test.py | 376 ++++++++++++++++-------------------------- 3 files changed, 163 insertions(+), 242 deletions(-) diff --git a/CHANGES b/CHANGES index dd85ff5..d9ba0e1 100644 --- a/CHANGES +++ b/CHANGES @@ -8,6 +8,11 @@ Version [unreleased] environment and restore it on deactivate: `npx` exports the outer prefix, and `npm.cmd` runs the npm it finds there instead of its own `#309 `_ +- `--with-npm` on Windows and Cygwin installs npm from the registry tarball + instead of the GitHub source archive, whose workspace symlinks unpacked as + text files (npm 8) and whose workspaces are missing (npm >= 9); the default + `--npm=latest` no longer ends in a 404 there + `#310 `_ Version 1.11.0 -------------- diff --git a/nodeenv.py b/nodeenv.py index c91e621..7ee2a0b 100644 --- a/nodeenv.py +++ b/nodeenv.py @@ -1185,12 +1185,17 @@ def install_npm(env_dir, _src_dir, args): def install_npm_win(env_dir, src_dir, args): """ - Download source code for npm, unpack it + Download npm as published to the registry, unpack it and install it in virtual environment. """ logger.info(' * Install npm.js (%s) ... ' % args.npm, extra=dict(continued=True)) - npm_url = 'https://github.com/npm/cli/archive/v%s.zip' % args.npm + # Not the GitHub source archive: its workspace symlinks unpack as text + # files, or the workspaces are missing from it altogether + # https://github.com/ekalinin/nodeenv/issues/310 + npm_meta_url = 'https://registry.npmjs.org/npm/%s' % args.npm + npm_meta = json.loads(urlopen(npm_meta_url).read().decode('UTF-8')) + npm_url = npm_meta['dist']['tarball'] npm_contents = io.BytesIO(urlopen(npm_url).read()) bin_path = join(env_dir, 'Scripts') @@ -1205,10 +1210,14 @@ def install_npm_win(env_dir, src_dir, args): if os.path.exists(join(bin_path, 'npm-cli.js')): os.remove(join(bin_path, 'npm-cli.js')) - with zipfile.ZipFile(npm_contents, 'r') as zipf: - zipf.extractall(src_dir) + npm_src_dir = join(src_dir, 'npm-%s' % npm_meta['version']) + with tarfile_open(fileobj=npm_contents) as tarf: + if sys.version_info >= (3, 12): + tarf.extractall(npm_src_dir, filter="data") + else: + tarf.extractall(npm_src_dir) - npm_ver = 'cli-%s' % args.npm + npm_ver = join('npm-%s' % npm_meta['version'], 'package') shutil.copytree(join(src_dir, npm_ver), node_modules_path) shutil.copy(join(src_dir, npm_ver, 'bin', 'npm.cmd'), join(bin_path, 'npm.cmd')) @@ -1220,9 +1229,8 @@ def install_npm_win(env_dir, src_dir, args): join(env_dir, 'bin', 'npm-cli.js')) shutil.copytree(join(bin_path, 'node_modules'), join(env_dir, 'bin', 'node_modules')) - npm_gh_url = 'https://raw.githubusercontent.com/npm/cli' - npm_bin_url = '{}/{}/bin/npm'.format(npm_gh_url, args.npm) - writefile(join(env_dir, 'bin', 'npm'), urlopen(npm_bin_url).read()) + shutil.copy(join(src_dir, npm_ver, 'bin', 'npm'), + join(env_dir, 'bin', 'npm')) def _read_packages(filenames): diff --git a/tests/nodeenv_test.py b/tests/nodeenv_test.py index 602981d..c6be29f 100644 --- a/tests/nodeenv_test.py +++ b/tests/nodeenv_test.py @@ -15,6 +15,7 @@ import sys import platform import ssl +import tarfile import zipfile try: @@ -2427,251 +2428,158 @@ def test_install_npm_specific_version_formats(self): assert env['npm_install'] == version -class TestInstallNpmWin: - """Tests for install_npm_win function""" - - def test_install_npm_win_basic(self): - """Test basic Windows npm installation""" - args = mock.Mock() - args.npm = '8.19.2' - - env_dir = 'C:\\path\\to\\env' - src_dir = 'C:\\path\\to\\src' +NPM_REGISTRY = 'https://registry.npmjs.org/npm' - # Mock the zip file content - mock_zip_content = b'PK\x03\x04...' # Simplified zip header - mock_response = mock.Mock() - mock_response.read.return_value = mock_zip_content - - mock_zip = mock.Mock() - mock_zip.__enter__ = mock.Mock(return_value=mock_zip) - mock_zip.__exit__ = mock.Mock(return_value=False) - - with mock.patch.object( - nodeenv, 'urlopen', return_value=mock_response - ), \ - mock.patch.object(nodeenv, 'is_CYGWIN', False), \ - mock.patch('zipfile.ZipFile', return_value=mock_zip), \ - mock.patch('os.path.exists', return_value=False), \ - mock.patch('shutil.copytree') as mock_copytree, \ - mock.patch('shutil.copy') as mock_copy, \ - mock.patch.object(nodeenv.logger, 'info') as mock_logger: - nodeenv.install_npm_win(env_dir, src_dir, args) - - # Verify URL was constructed correctly - expected_url = 'https://github.com/npm/cli/archive/v8.19.2.zip' - nodeenv.urlopen.assert_called_once_with(expected_url) - - # Verify extraction happened - mock_zip.extractall.assert_called_once_with(src_dir) - - # Verify copytree and copy were called - assert mock_copytree.called - assert mock_copy.call_count == 2 - - # Verify logging - log_calls = [call[0][0] for call in mock_logger.call_args_list] - assert any('8.19.2' in str(call) for call in log_calls) - - def test_install_npm_win_removes_existing_files(self): - """Test that existing npm files are removed before installation""" - args = mock.Mock() - args.npm = '9.0.0' +# A published npm tarball: everything lives under package/, the scripts in +# bin/ are executable and the workspaces are bundled as plain directories +NPM_TARBALL_FILES = { + 'package/package.json': ('{"name": "npm"}', 0o644), + 'package/bin/npm': ('#!/usr/bin/env bash\n# npm\n', 0o755), + 'package/bin/npm.cmd': (':: npm.cmd\n', 0o755), + 'package/bin/npm-cli.js': ('#!/usr/bin/env node\n// npm-cli\n', 0o755), + 'package/node_modules/@npmcli/config/package.json': + ('{"name": "@npmcli/config"}', 0o644), +} - env_dir = 'C:\\env' - src_dir = 'C:\\src' - mock_zip_content = b'PK\x03\x04...' - mock_response = mock.Mock() - mock_response.read.return_value = mock_zip_content - - mock_zip = mock.Mock() - mock_zip.__enter__ = mock.Mock(return_value=mock_zip) - mock_zip.__exit__ = mock.Mock(return_value=False) - - # Simulate existing files - def exists_side_effect(path): - if ('node_modules' in path or 'npm.cmd' in path or - 'npm-cli.js' in path): - return True - return False - - with mock.patch.object( - nodeenv, 'urlopen', return_value=mock_response - ), \ - mock.patch.object(nodeenv, 'is_CYGWIN', False), \ - mock.patch('zipfile.ZipFile', return_value=mock_zip), \ - mock.patch('os.path.exists', side_effect=exists_side_effect), \ - mock.patch('shutil.rmtree') as mock_rmtree, \ - mock.patch('os.remove') as mock_remove, \ - mock.patch('shutil.copytree'), \ - mock.patch('shutil.copy'), \ - mock.patch.object(nodeenv.logger, 'info'): - nodeenv.install_npm_win(env_dir, src_dir, args) - - # Verify cleanup happened - mock_rmtree.assert_called_once() - assert mock_remove.call_count == 2 - - def test_install_npm_win_cygwin(self): - """Test Windows npm installation on CYGWIN""" - args = mock.Mock() - args.npm = '7.24.2' - - env_dir = '/cygdrive/c/env' - src_dir = '/cygdrive/c/src' - - mock_zip_content = b'PK\x03\x04...' - mock_response = mock.Mock() - mock_response.read.return_value = mock_zip_content - - mock_npm_script = b'#!/bin/sh\n# npm script' - mock_npm_response = mock.Mock() - mock_npm_response.read.return_value = mock_npm_script - - mock_zip = mock.Mock() - mock_zip.__enter__ = mock.Mock(return_value=mock_zip) - mock_zip.__exit__ = mock.Mock(return_value=False) - - with mock.patch.object(nodeenv, 'urlopen') as mock_urlopen, \ - mock.patch.object(nodeenv, 'is_CYGWIN', True), \ - mock.patch.object(nodeenv, 'writefile') as mock_writefile, \ - mock.patch('zipfile.ZipFile', return_value=mock_zip), \ - mock.patch('os.path.exists', return_value=False), \ - mock.patch('shutil.copytree'), \ - mock.patch('shutil.copy'), \ - mock.patch.object(nodeenv.logger, 'info'): - mock_urlopen.side_effect = [mock_response, mock_npm_response] - - nodeenv.install_npm_win(env_dir, src_dir, args) - - # Verify that CYGWIN-specific operations happened - assert mock_urlopen.call_count == 2 - assert mock_writefile.called - - # Verify the raw GitHub URL was called - calls = [str(call) for call in mock_urlopen.call_args_list] - assert any( - 'raw.githubusercontent.com' in str(call) for call in calls - ) - - def test_install_npm_win_different_versions(self): - """Test Windows npm installation with different version formats""" - test_versions = ['8.0.0', '9.5.1', '10.0.0'] - - for version in test_versions: - args = mock.Mock() - args.npm = version - - env_dir = 'C:\\env' - src_dir = 'C:\\src' - - mock_zip_content = b'PK\x03\x04...' - mock_response = mock.Mock() - mock_response.read.return_value = mock_zip_content - - mock_zip = mock.Mock() - mock_zip.__enter__ = mock.Mock(return_value=mock_zip) - mock_zip.__exit__ = mock.Mock(return_value=False) - - with mock.patch.object( - nodeenv, 'urlopen', return_value=mock_response - ) as mock_urlopen, \ - mock.patch.object(nodeenv, 'is_CYGWIN', False), \ - mock.patch('zipfile.ZipFile', return_value=mock_zip), \ - mock.patch('os.path.exists', return_value=False), \ - mock.patch('shutil.copytree'), \ - mock.patch('shutil.copy'), \ - mock.patch.object(nodeenv.logger, 'info'): - nodeenv.install_npm_win(env_dir, src_dir, args) - - # Verify correct URL for each version - expected_url = ( - f'https://github.com/npm/cli/archive/v{version}.zip' - ) - mock_urlopen.assert_called_with(expected_url) - - def test_install_npm_win_paths(self): - """Test that Windows npm installation uses correct paths""" - args = mock.Mock() - args.npm = '8.5.0' - - env_dir = 'C:\\Users\\test\\env' - src_dir = 'C:\\Users\\test\\src' - - mock_zip_content = b'PK\x03\x04...' - mock_response = mock.Mock() - mock_response.read.return_value = mock_zip_content - - mock_zip = mock.Mock() - mock_zip.__enter__ = mock.Mock(return_value=mock_zip) - mock_zip.__exit__ = mock.Mock(return_value=False) - - with mock.patch.object( - nodeenv, 'urlopen', return_value=mock_response - ), \ - mock.patch.object(nodeenv, 'is_CYGWIN', False), \ - mock.patch('zipfile.ZipFile', return_value=mock_zip), \ - mock.patch('os.path.exists', return_value=False), \ - mock.patch('shutil.copytree') as mock_copytree, \ - mock.patch('shutil.copy') as mock_copy, \ - mock.patch.object(nodeenv.logger, 'info'): - nodeenv.install_npm_win(env_dir, src_dir, args) - - # Verify paths - copytree_call = mock_copytree.call_args[0] - src_path = copytree_call[0] - dst_path = copytree_call[1] - - assert 'cli-8.5.0' in src_path - expected_path = os.path.join( - env_dir, 'Scripts', 'node_modules', 'npm' - ) - assert expected_path == dst_path +def _npm_tarball(files): + buf = io.BytesIO() + with tarfile.open(fileobj=buf, mode='w:gz') as tf: + for name, (content, mode) in sorted(files.items()): + data = content.encode('utf-8') + info = tarfile.TarInfo(name) + info.size = len(data) + info.mode = mode + tf.addfile(info, io.BytesIO(data)) + return buf.getvalue() - # Verify copy calls use correct paths - copy_calls = mock_copy.call_args_list - assert len(copy_calls) == 2 - assert any('npm.cmd' in str(call) for call in copy_calls) - assert any('npm-cli.js' in str(call) for call in copy_calls) - def test_install_npm_win_zip_extraction(self): - """Test that zip file is properly extracted""" - args = mock.Mock() - args.npm = '9.1.0' +def _npm_registry(spec, version, files): + """ + Answer like the npm registry does for `spec` (a version or a dist-tag): + its metadata document, and the tarball that document points to. Any + other URL gets a 404. + """ + tarball = '%s/-/npm-%s.tgz' % (NPM_REGISTRY, version) + meta = { + 'name': 'npm', + 'version': version, + 'dist': { + 'shasum': '0' * 40, + 'tarball': tarball, + 'fileCount': len(files), + 'integrity': 'sha512-', + }, + } + responses = { + '%s/%s' % (NPM_REGISTRY, spec): json.dumps(meta).encode('utf-8'), + tarball: _npm_tarball(files), + } + + def urlopen(url): + if url not in responses: + raise nodeenv.urllib2.HTTPError(url, 404, 'Not Found', {}, None) + return io.BytesIO(responses[url]) + return urlopen + + +def _npm_env(tmpdir): + """An environment as install_npm_win finds it: node in, no npm yet""" + env_dir = tmpdir.mkdir('env') + env_dir.mkdir('Scripts') + env_dir.mkdir('bin') + env_dir.mkdir('src') + return env_dir + + +def _install_npm_win(env_dir, spec='8.3.1', version='8.3.1', + files=NPM_TARBALL_FILES): + class args: + npm = spec - env_dir = 'C:\\test' - src_dir = 'C:\\test\\src' + with mock.patch.object(nodeenv, 'urlopen', + _npm_registry(spec, version, files)): + nodeenv.install_npm_win(env_dir.strpath, env_dir.join('src').strpath, + args) - mock_zip_content = b'PK\x03\x04...' - mock_response = mock.Mock() - mock_response.read.return_value = mock_zip_content - mock_zip = mock.Mock() - mock_zip.__enter__ = mock.Mock(return_value=mock_zip) - mock_zip.__exit__ = mock.Mock(return_value=False) - mock_zip.extractall = mock.Mock() +class TestInstallNpmWin: + """Tests for install_npm_win function""" - with mock.patch.object( - nodeenv, 'urlopen', return_value=mock_response - ), \ - mock.patch.object(nodeenv, 'is_CYGWIN', False), \ - mock.patch( - 'zipfile.ZipFile', return_value=mock_zip - ) as mock_zipfile, \ - mock.patch('os.path.exists', return_value=False), \ - mock.patch('shutil.copytree'), \ - mock.patch('shutil.copy'), \ - mock.patch.object(nodeenv.logger, 'info'): - nodeenv.install_npm_win(env_dir, src_dir, args) + @pytest.mark.parametrize(('spec', 'version'), ( + ('8.3.1', '8.3.1'), + # the default --npm, a dist-tag the registry resolves + ('latest', '12.1.0'), + )) + def test_install_npm_win_installs_the_registry_tarball(self, tmpdir, + spec, version): + """ + The GitHub source archive of npm is not an installable npm: its + workspace symlinks unpack as text files (npm 8) or the workspaces + are missing altogether (npm >= 9). + https://github.com/ekalinin/nodeenv/issues/310 + """ + env_dir = _npm_env(tmpdir) + with mock.patch.object(nodeenv, 'is_CYGWIN', False): + _install_npm_win(env_dir, spec, version) + + scripts = env_dir.join('Scripts') + npm_dir = scripts.join('node_modules', 'npm') + assert npm_dir.join('node_modules', '@npmcli', 'config', + 'package.json').read() == \ + '{"name": "@npmcli/config"}' + assert npm_dir.join('bin', 'npm-cli.js').read() == \ + '#!/usr/bin/env node\n// npm-cli\n' + assert scripts.join('npm.cmd').read() == ':: npm.cmd\n' + assert scripts.join('npm-cli.js').read() == \ + '#!/usr/bin/env node\n// npm-cli\n' + + def test_install_npm_win_removes_existing_files(self, tmpdir): + """A reinstall replaces the npm already in Scripts""" + env_dir = _npm_env(tmpdir) + scripts = env_dir.join('Scripts') + scripts.mkdir('node_modules').mkdir('npm').join('stale.js').write('') + scripts.join('npm.cmd').write(':: old npm.cmd\n') + scripts.join('npm-cli.js').write('// old npm-cli\n') + + with mock.patch.object(nodeenv, 'is_CYGWIN', False): + _install_npm_win(env_dir) + + assert not scripts.join('node_modules', 'npm', 'stale.js').check() + assert scripts.join('npm.cmd').read() == ':: npm.cmd\n' + assert scripts.join('npm-cli.js').read() == \ + '#!/usr/bin/env node\n// npm-cli\n' + + def test_install_npm_win_cygwin(self, tmpdir): + """bin/npm for Cygwin ships in the tarball, no second download""" + env_dir = _npm_env(tmpdir) + with mock.patch.object(nodeenv, 'is_CYGWIN', True): + _install_npm_win(env_dir) + + bin_npm = env_dir.join('bin', 'npm') + assert bin_npm.read() == '#!/usr/bin/env bash\n# npm\n' + assert os.access(bin_npm.strpath, os.X_OK) + assert env_dir.join('bin', 'npm-cli.js').read() == \ + '#!/usr/bin/env node\n// npm-cli\n' + assert env_dir.join('bin', 'node_modules', 'npm', 'node_modules', + '@npmcli', 'config', 'package.json').check(file=1) + + @pytest.mark.skipif(sys.version_info < (3, 12), + reason='tarfile has extraction filters since 3.12') + def test_install_npm_win_keeps_data_filter(self, tmpdir): + """ + A member pointing out of the unpack directory is refused + (CVE-2007-4559) + """ + env_dir = _npm_env(tmpdir) + files = dict(NPM_TARBALL_FILES) + files['package/../../escaped'] = ('', 0o644) - # Verify ZipFile was created with the BytesIO content - mock_zipfile.assert_called_once() - zip_args = mock_zipfile.call_args[0] - assert hasattr(zip_args[0], 'read') # Should be BytesIO object + with mock.patch.object(nodeenv, 'is_CYGWIN', False), \ + pytest.raises(tarfile.OutsideDestinationError): + _install_npm_win(env_dir, files=files) - # Verify extraction - mock_zip.extractall.assert_called_once_with(src_dir) + assert not env_dir.join('src', 'escaped').check() class TestCertifi: