Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/publish.yml
Original file line number Diff line number Diff line change
Expand Up @@ -596,7 +596,7 @@ jobs:
echo "Version set to: $VERSION"
echo "DRY_RUN set to: $DRY_RUN"
echo ""
pnpm nx-release --local=false $VERSION $DRY_RUN
pnpm nx nx-release-npm @nx/nx-source -- $VERSION $DRY_RUN

- name: (Stable Release Only) Trigger Docs Release
# Publish docs only on a full release
Expand Down
1 change: 1 addition & 0 deletions .nx/workflows/dynamic-changesets.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ assignment-rules:
# These projects should not need to be isolated.
- projects:
- nx-dev
- astro-docs
targets:
- build*
run-on:
Expand Down
65 changes: 49 additions & 16 deletions e2e/utils/global-setup.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import { Config } from '@jest/types';
import { existsSync, removeSync } from 'fs-extra';
import * as isCI from 'is-ci';
import { exec, execSync } from 'node:child_process';
import { ChildProcess, exec, execSync, spawn } from 'node:child_process';
import { join } from 'node:path';
import { registerTsConfigPaths } from '../../packages/nx/src/plugins/js/utils/register';
import { runLocalRelease } from '../../scripts/local-registry/populate-storage';

export default async function (globalConfig: Config.ConfigGlobals) {
try {
Expand All @@ -25,14 +24,23 @@ export default async function (globalConfig: Config.ConfigGlobals) {
const registry = `http://${listenAddress}:${port}`;
const authToken = 'secretVerdaccioToken';

while (true) {
await new Promise((resolve) => setTimeout(resolve, 250));
try {
await assertLocalRegistryIsRunning(registry);
break;
} catch {
console.log(`Waiting for Local registry to start on ${registry}...`);
}
// When running outside of Nx (e.g. Jest directly), start verdaccio ourselves
let verdaccioProcess: ChildProcess | undefined;
if (requiresLocalRelease && !(await isLocalRegistryRunning(registry))) {
console.log(
`Local registry not detected at ${registry}, starting verdaccio...`
);
verdaccioProcess = spawn(
'npx',
[
'verdaccio',
'--config',
'.verdaccio/config.yml',
'--listen',
`${listenAddress}:${port}`,
],
{ stdio: 'ignore' }
);
Comment on lines +33 to +43
Copy link
Contributor

Choose a reason for hiding this comment

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

Spawning verdaccio with detached: true without calling unref() will prevent the parent process from exiting until the child exits, defeating the purpose of background execution. Additionally, calling kill() on line 67 may not properly terminate a detached process.

Fix by either:

  1. Removing detached: true if the process should be tied to parent lifecycle
  2. Or calling verdaccioProcess.unref() after spawn and using process.kill(-verdaccioProcess.pid) to kill the process group:
verdaccioProcess = spawn(..., { stdio: 'ignore', detached: true });
verdaccioProcess.unref();

// In teardown:
if (verdaccioProcess && verdaccioProcess.pid) {
  process.kill(-verdaccioProcess.pid);
}

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

}

process.env.npm_config_registry = registry;
Expand All @@ -54,6 +62,11 @@ export default async function (globalConfig: Config.ConfigGlobals) {
global.e2eTeardown = () => {
// Clean up environment variable instead of npm config command
delete process.env[`npm_config_//${listenAddress}:${port}/:_authToken`];
// Kill verdaccio if we started it
if (verdaccioProcess) {
verdaccioProcess.kill();
verdaccioProcess = undefined;
}
};

/**
Expand All @@ -77,8 +90,26 @@ export default async function (globalConfig: Config.ConfigGlobals) {
if (requiresLocalRelease) {
console.log('Publishing packages to local registry');
const publishVersion = process.env.PUBLISHED_VERSION ?? 'major';
// Always show full release logs on CI, they should only happen once via e2e-ci
await runLocalRelease(publishVersion, isCI || isVerbose);
const verbose = isCI || isVerbose;
const releaseCommand = `pnpm nx-release --local ${publishVersion}`;
console.log(`> ${releaseCommand}`);
Comment on lines +94 to +95
Copy link
Contributor

Choose a reason for hiding this comment

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

Command string injection vulnerability: publishVersion comes from process.env.PUBLISHED_VERSION without validation and is directly interpolated into a shell command. If an attacker controls this environment variable, they can execute arbitrary commands.

const releaseCommand = `pnpm nx-release --local ${publishVersion}`;

Fix by passing as a proper argument array or validating the input:

const publishVersion = process.env.PUBLISHED_VERSION ?? 'major';
if (!/^(major|minor|patch|\d+\.\d+\.\d+)$/.test(publishVersion)) {
  throw new Error(`Invalid PUBLISHED_VERSION: ${publishVersion}`);
}
const releaseCommand = `pnpm nx-release --local ${publishVersion}`;
Suggested change
const releaseCommand = `pnpm nx-release --local ${publishVersion}`;
console.log(`> ${releaseCommand}`);
const publishVersion = process.env.PUBLISHED_VERSION ?? 'major';
if (!/^(major|minor|patch|\d+\.\d+\.\d+)$/.test(publishVersion)) {
throw new Error(`Invalid PUBLISHED_VERSION: ${publishVersion}`);
}
const releaseCommand = `pnpm nx-release --local ${publishVersion}`;
console.log(`> ${releaseCommand}`);

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

await new Promise<void>((resolve, reject) => {
const child = exec(releaseCommand, {
maxBuffer: 1024 * 1000000,
windowsHide: false,
});
if (verbose) {
child.stdout?.pipe(process.stdout);
child.stderr?.pipe(process.stderr);
}
child.on('exit', (code) => {
if (code === 0) {
resolve();
} else {
reject(new Error(`Local release failed with exit code ${code}`));
}
});
});
}
}
} catch (err) {
Expand Down Expand Up @@ -112,9 +143,11 @@ function getPublishedVersion(): Promise<string | undefined> {
});
}

async function assertLocalRegistryIsRunning(url) {
const response = await fetch(url);
if (!response.ok) {
throw new Error(`HTTP error! status: ${response.status}`);
async function isLocalRegistryRunning(url: string): Promise<boolean> {
try {
const response = await fetch(url);
return response.ok;
} catch {
return false;
}
}
2 changes: 1 addition & 1 deletion nx.json
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@
"nxCloudId": "62d013ea0852fe0a2df74438",
"nxCloudUrl": "https://staging.nx.app",
"parallel": 1,
"bust": 2,
"bust": 3,
"defaultBase": "master",
"sync": {
"applyChanges": true
Expand Down
13 changes: 8 additions & 5 deletions project.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,27 +22,30 @@
"input": "production",
"projects": ["tag:maven:dev.nx.maven"]
},
"{workspaceRoot}/scripts/local-registry",
"native"
],
"dependsOn": ["local-registry", "nx-release"],
"command": "echo 'Registry storage populated via nx-release dependency'",
"outputs": ["{workspaceRoot}/dist/local-registry/storage"]
},
"nx-release": {
"dependsOn": [
"local-registry",
{
"target": "build",
"projects": ["tag:npm:public"]
}
],
"command": "node ./scripts/local-registry/run-populate-storage.mjs",
"outputs": ["{workspaceRoot}/dist/local-registry/storage"]
"command": "ts-node -P ./scripts/tsconfig.release.json ./scripts/nx-release.ts"
},
"nx-release": {
"nx-release-npm": {
"dependsOn": [
{
"target": "build",
"projects": ["tag:npm:public"]
}
],
"command": "ts-node -P ./scripts/tsconfig.release.json ./scripts/nx-release.ts"
"command": "ts-node -P ./scripts/tsconfig.release.json ./scripts/nx-release.ts --local false"
},
"start-docker-registry": {
"continuous": true,
Expand Down
79 changes: 0 additions & 79 deletions scripts/local-registry/populate-storage.js

This file was deleted.

11 changes: 0 additions & 11 deletions scripts/local-registry/run-populate-storage.mjs

This file was deleted.

38 changes: 34 additions & 4 deletions scripts/nx-release.ts
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,13 @@ const VALID_AUTHORS_FOR_LATEST = [

hackFixForDevkitPeerDependencies();

if (options.local) {
const port = process.env.NX_LOCAL_REGISTRY_PORT ?? '4873';
const localRegistryUrl = `http://localhost:${port}`;
await waitForLocalRegistry(localRegistryUrl);
process.env.npm_config_registry = localRegistryUrl;
}

// Run with dynamic output-style so that we have more minimal logs by default but still always see errors
let publishCommand = `pnpm nx release publish --registry=${getRegistry()} --tag=${distTag} --output-style=dynamic --parallel=8`;
if (options.dryRun) {
Expand Down Expand Up @@ -400,10 +407,6 @@ function parseArgs() {
'Registry is still set to localhost! Run "pnpm local-registry disable" or pass --force'
);
}
} else {
if (!args.force && !registryIsLocalhost) {
throw new Error('--local was passed and registry is not localhost');
}
}

return true;
Expand Down Expand Up @@ -476,6 +479,33 @@ function determineDistTag(
return distTag;
}

function waitForLocalRegistry(registryUrl: string): Promise<void> {
console.log(`Waiting for local registry at ${registryUrl}...`);
return new Promise<void>((resolve, reject) => {
const timeout = setTimeout(() => {
clearInterval(interval);
reject(
new Error(
`Local registry at ${registryUrl} did not become available within 60 seconds`
)
);
}, 60_000);
const interval = setInterval(async () => {
try {
const response = await fetch(registryUrl);
if (response.ok) {
clearInterval(interval);
clearTimeout(timeout);
console.log('Local registry is ready.');
resolve();
}
} catch {
// Registry not up yet
}
}, 50);
Comment on lines +493 to +505
Copy link
Contributor

Choose a reason for hiding this comment

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

Race condition: Multiple overlapping fetch requests can occur if the async fetch takes longer than 50ms. Each interval tick starts a new async operation without waiting for previous ones to complete. This could spam the registry with requests during startup.

const interval = setInterval(async () => {
  try {
    const response = await fetch(registryUrl);
    if (response.ok) {
      clearInterval(interval);
      clearTimeout(timeout);
      console.log('Local registry is ready.');
      resolve();
    }
  } catch {
    // Registry not up yet
  }
}, 50);

Fix by using a flag to prevent concurrent checks:

let checking = false;
const interval = setInterval(async () => {
  if (checking) return;
  checking = true;
  try {
    const response = await fetch(registryUrl);
    if (response.ok) {
      clearInterval(interval);
      clearTimeout(timeout);
      console.log('Local registry is ready.');
      resolve();
    }
  } catch {
    // Registry not up yet
  } finally {
    checking = false;
  }
}, 50);
Suggested change
const interval = setInterval(async () => {
try {
const response = await fetch(registryUrl);
if (response.ok) {
clearInterval(interval);
clearTimeout(timeout);
console.log('Local registry is ready.');
resolve();
}
} catch {
// Registry not up yet
}
}, 50);
let checking = false;
const interval = setInterval(async () => {
if (checking) return;
checking = true;
try {
const response = await fetch(registryUrl);
if (response.ok) {
clearInterval(interval);
clearTimeout(timeout);
console.log('Local registry is ready.');
resolve();
}
} catch {
// Registry not up yet
} finally {
checking = false;
}
}, 50);

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

});
}

//TODO(@Coly010): Remove this after fixing up the release peer dep handling
function hackFixForDevkitPeerDependencies() {
const { readFileSync, writeFileSync } = require('fs');
Expand Down
Loading