Skip to content

Commit

Permalink
More tightly restrict allowed phase names.
Browse files Browse the repository at this point in the history
  • Loading branch information
@iclanton
iclanton committed Dec 30, 2021
1 parent 00664c9 commit c630e6e
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 39 deletions.
32 changes: 18 additions & 14 deletions apps/rush-lib/src/api/CommandLineConfiguration.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,9 @@ export class CommandLineConfiguration {
*/
public constructor(commandLineJson: ICommandLineJson | undefined) {
if (commandLineJson?.phases) {
const phaseNamePrefix: string = RushConstants.phaseNamePrefix;
const phaseNameRegexp: RegExp = new RegExp(
`^${RushConstants.phaseNamePrefix}[a-z][a-z0-9]*([-][a-z0-9]+)*$`
);
for (const phase of commandLineJson.phases) {
if (this.phases.has(phase.name)) {
throw new Error(
Expand All @@ -148,17 +150,13 @@ export class CommandLineConfiguration {
);
}

if (phase.name.substring(0, phaseNamePrefix.length) !== phaseNamePrefix) {
if (!phase.name.match(phaseNameRegexp)) {
throw new Error(
`In ${RushConstants.commandLineFilename}, the phase "${phase.name}"'s name ` +
`does not begin with the required prefix "${phaseNamePrefix}".`
);
}

if (phase.name.length <= phaseNamePrefix.length) {
throw new Error(
`In ${RushConstants.commandLineFilename}, the phase "${phase.name}"'s name ` +
`must have characters after "${phaseNamePrefix}"`
'is not a valid phase name. Phase names must begin with the ' +
`required prefix "${RushConstants.phaseNamePrefix}" followed by a name containing ` +
'lowercase letters, numbers, or hyphens. The name must start with a letter and ' +
'must not end with a hyphen.'
);
}

Expand Down Expand Up @@ -441,9 +439,9 @@ export class CommandLineConfiguration {
}

private _checkForPhaseSelfCycles(phase: IPhase, checkedPhases: Set<string> = new Set<string>()): void {
const dependencies: string[] | undefined = phase.dependencies?.self;
if (dependencies) {
for (const dependencyName of dependencies) {
const phaseSelfDependencies: string[] | undefined = phase.dependencies?.self;
if (phaseSelfDependencies) {
for (const dependencyName of phaseSelfDependencies) {
if (checkedPhases.has(dependencyName)) {
const dependencyNameForError: string =
typeof dependencyName === 'string' ? dependencyName : '<synthetic phase>';
Expand All @@ -457,7 +455,7 @@ export class CommandLineConfiguration {
if (!dependency) {
return; // Ignore, we check for this separately
} else {
if (dependencies.length > 1) {
if (phaseSelfDependencies.length > 1) {
this._checkForPhaseSelfCycles(
dependency,
// Clone the set of checked phases if there are multiple branches we need to check
Expand Down Expand Up @@ -549,6 +547,12 @@ export class CommandLineConfiguration {
this._additionalPathFolders.unshift(pathFolder);
}

/**
* This function replaces colons (":") with underscores ("_").
*
* ts-command-line restricts command names to lowercase letters, numbers, underscores, and colons.
* Replacing colons with underscores produces a filesystem-safe name.
*/
private _normalizeNameForLogFilenameIdentifiers(name: string): string {
return name.replace(/:/g, '_'); // Replace colons with underscores to be filesystem-safe
}
Expand Down
68 changes: 49 additions & 19 deletions apps/rush-lib/src/api/test/CommandLineConfiguration.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,36 @@ describe(CommandLineConfiguration.name, () => {
]
})
).toThrowErrorMatchingSnapshot();
expect(
() =>
new CommandLineConfiguration({
phases: [
{
name: '_phase:0'
}
]
})
).toThrowErrorMatchingSnapshot();
expect(
() =>
new CommandLineConfiguration({
phases: [
{
name: '_phase:A'
}
]
})
).toThrowErrorMatchingSnapshot();
expect(
() =>
new CommandLineConfiguration({
phases: [
{
name: '_phase:A-'
}
]
})
).toThrowErrorMatchingSnapshot();
});

it('Detects a missing phase', () => {
Expand All @@ -41,7 +71,7 @@ describe(CommandLineConfiguration.name, () => {
safeForSimultaneousRushProcesses: false,

enableParallelism: true,
phases: ['_phase:A']
phases: ['_phase:a']
}
]
})
Expand All @@ -54,9 +84,9 @@ describe(CommandLineConfiguration.name, () => {
new CommandLineConfiguration({
phases: [
{
name: '_phase:A',
name: '_phase:a',
dependencies: {
upstream: ['_phase:B']
upstream: ['_phase:b']
}
}
]
Expand All @@ -68,9 +98,9 @@ describe(CommandLineConfiguration.name, () => {
new CommandLineConfiguration({
phases: [
{
name: '_phase:A',
name: '_phase:a',
dependencies: {
self: ['_phase:B']
self: ['_phase:b']
}
}
]
Expand All @@ -84,21 +114,21 @@ describe(CommandLineConfiguration.name, () => {
new CommandLineConfiguration({
phases: [
{
name: '_phase:A',
name: '_phase:a',
dependencies: {
self: ['_phase:B']
self: ['_phase:b']
}
},
{
name: '_phase:B',
name: '_phase:b',
dependencies: {
self: ['_phase:C']
self: ['_phase:c']
}
},
{
name: '_phase:C',
name: '_phase:c',
dependencies: {
self: ['_phase:A']
self: ['_phase:a']
}
}
]
Expand Down Expand Up @@ -168,19 +198,19 @@ describe(CommandLineConfiguration.name, () => {
summary: 'custom-phased',
enableParallelism: true,
safeForSimultaneousRushProcesses: false,
phases: ['_phase:A']
phases: ['_phase:a']
}
],
phases: [
{
name: '_phase:A'
name: '_phase:a'
}
],
parameters: [
{
parameterKind: 'flag',
longName: '--flag',
associatedPhases: ['_phase:A'],
associatedPhases: ['_phase:a'],
description: 'flag'
}
]
Expand All @@ -202,25 +232,25 @@ describe(CommandLineConfiguration.name, () => {
summary: 'custom-phased',
enableParallelism: true,
safeForSimultaneousRushProcesses: false,
phases: ['_phase:A']
phases: ['_phase:a']
}
],
phases: [
{
name: '_phase:A',
name: '_phase:a',
dependencies: {
upstream: ['_phase:B']
upstream: ['_phase:b']
}
},
{
name: '_phase:B'
name: '_phase:b'
}
],
parameters: [
{
parameterKind: 'flag',
longName: '--flag',
associatedPhases: ['_phase:B'],
associatedPhases: ['_phase:b'],
description: 'flag'
}
]
Expand Down
18 changes: 12 additions & 6 deletions apps/rush-lib/src/api/test/__snapshots__/CommandLineConfiguration.test.ts.snap
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`CommandLineConfiguration Detects a cycle among phases 1`] = `"In command-line.json, there exists a cycle within the set of _phase:B dependencies: _phase:B, _phase:C, _phase:A"`;
exports[`CommandLineConfiguration Detects a cycle among phases 1`] = `"In command-line.json, there exists a cycle within the set of _phase:b dependencies: _phase:b, _phase:c, _phase:a"`;

exports[`CommandLineConfiguration Detects a missing phase 1`] = `"In command-line.json, in the \\"phases\\" property of the \\"example\\" command, the phase \\"_phase:A\\" does not exist."`;
exports[`CommandLineConfiguration Detects a missing phase 1`] = `"In command-line.json, in the \\"phases\\" property of the \\"example\\" command, the phase \\"_phase:a\\" does not exist."`;

exports[`CommandLineConfiguration Detects a missing phase dependency 1`] = `"In command-line.json, in the phase \\"_phase:A\\", the upstream dependency phase \\"_phase:B\\" does not exist."`;
exports[`CommandLineConfiguration Detects a missing phase dependency 1`] = `"In command-line.json, in the phase \\"_phase:a\\", the upstream dependency phase \\"_phase:b\\" does not exist."`;

exports[`CommandLineConfiguration Detects a missing phase dependency 2`] = `"In command-line.json, in the phase \\"_phase:A\\", the self dependency phase \\"_phase:B\\" does not exist."`;
exports[`CommandLineConfiguration Detects a missing phase dependency 2`] = `"In command-line.json, in the phase \\"_phase:a\\", the self dependency phase \\"_phase:b\\" does not exist."`;

exports[`CommandLineConfiguration Forbids a misnamed phase 1`] = `"In command-line.json, the phase \\"_faze:A\\"'s name does not begin with the required prefix \\"_phase:\\"."`;
exports[`CommandLineConfiguration Forbids a misnamed phase 1`] = `"In command-line.json, the phase \\"_faze:A\\"'s name is not a valid phase name. Phase names must begin with the required prefix \\"_phase:\\" followed by a name containing lowercase letters, numbers, or hyphens. The name must start with a letter and must not end with a hyphen."`;

exports[`CommandLineConfiguration Forbids a misnamed phase 2`] = `"In command-line.json, the phase \\"_phase:\\"'s name must have characters after \\"_phase:\\""`;
exports[`CommandLineConfiguration Forbids a misnamed phase 2`] = `"In command-line.json, the phase \\"_phase:\\"'s name is not a valid phase name. Phase names must begin with the required prefix \\"_phase:\\" followed by a name containing lowercase letters, numbers, or hyphens. The name must start with a letter and must not end with a hyphen."`;

exports[`CommandLineConfiguration Forbids a misnamed phase 3`] = `"In command-line.json, the phase \\"_phase:0\\"'s name is not a valid phase name. Phase names must begin with the required prefix \\"_phase:\\" followed by a name containing lowercase letters, numbers, or hyphens. The name must start with a letter and must not end with a hyphen."`;

exports[`CommandLineConfiguration Forbids a misnamed phase 4`] = `"In command-line.json, the phase \\"_phase:A\\"'s name is not a valid phase name. Phase names must begin with the required prefix \\"_phase:\\" followed by a name containing lowercase letters, numbers, or hyphens. The name must start with a letter and must not end with a hyphen."`;

exports[`CommandLineConfiguration Forbids a misnamed phase 5`] = `"In command-line.json, the phase \\"_phase:A-\\"'s name is not a valid phase name. Phase names must begin with the required prefix \\"_phase:\\" followed by a name containing lowercase letters, numbers, or hyphens. The name must start with a letter and must not end with a hyphen."`;

0 comments on commit c630e6e

Please sign in to comment.