assign-ports: warn on malformed explicit port env vars (follow-up to #242)

packages/shaka-shared/src/__tests__/assign-ports.test.ts

@@ -31,6 +31,7 @@ describe('assignPortsAutomatically', () => {
   });
 
   afterEach(() => {
+    jest.restoreAllMocks();
     fs.rmSync(dir, { recursive: true, force: true });
   });
 
@@ -76,6 +77,61 @@ describe('assignPortsAutomatically', () => {
     expect(fs.existsSync(settingsPath)).toBe(false);
   });
 
+  it('warns when an explicit port var is present but not a valid integer, and falls through', () => {
+    const warn = jest.spyOn(console, 'warn').mockImplementation(() => {});
+    const deps: AssignPortsDeps = {
+      ...baseDeps(),
+      env: { SHAKAPERF_CONTROL_PORT: '40x0', SHAKAPERF_EXPERIMENT_PORT: '4001' },
+    };
+    expect(assignPortsAutomatically({ ...pref, key: 'a' }, deps)).toEqual({ control: 3040, experiment: 3050 });
+    expect(warn).toHaveBeenCalledWith(expect.stringContaining('not a valid port'));
+    expect(warn).toHaveBeenCalledTimes(1);
+    warn.mockRestore();
+  });
+
+  it('warns when an explicit port exceeds the TCP range, and falls through', () => {
+    const warn = jest.spyOn(console, 'warn').mockImplementation(() => {});
+    const deps: AssignPortsDeps = {
+      ...baseDeps(),
+      env: { SHAKAPERF_CONTROL_PORT: '99999', SHAKAPERF_EXPERIMENT_PORT: '4001' },
+    };
+    expect(assignPortsAutomatically({ ...pref, key: 'a' }, deps)).toEqual({ control: 3040, experiment: 3050 });
+    expect(warn).toHaveBeenCalledWith(expect.stringContaining('not a valid port'));
+    expect(warn).toHaveBeenCalledTimes(1);
+    warn.mockRestore();
+  });
+
+  it('warns about a privileged explicit port but still uses (and does not persist) the pair', () => {
+    const warn = jest.spyOn(console, 'warn').mockImplementation(() => {});
+    const deps: AssignPortsDeps = {
+      ...baseDeps(),
+      env: { SHAKAPERF_CONTROL_PORT: '80', SHAKAPERF_EXPERIMENT_PORT: '81' },
+    };
+    expect(assignPortsAutomatically({ ...pref, key: 'a' }, deps)).toEqual({ control: 80, experiment: 81 });
+    expect(warn).toHaveBeenCalledWith(expect.stringContaining('privileged'));
+    expect(fs.existsSync(settingsPath)).toBe(false);
+    warn.mockRestore();
+  });
+
+  it('warns when only one explicit port var is set, and falls through', () => {
+    const warn = jest.spyOn(console, 'warn').mockImplementation(() => {});
+    const deps: AssignPortsDeps = { ...baseDeps(), env: { SHAKAPERF_CONTROL_PORT: '4000' } };
+    expect(assignPortsAutomatically({ ...pref, key: 'a' }, deps)).toEqual({ control: 3040, experiment: 3050 });
+    expect(warn).toHaveBeenCalledWith(expect.stringContaining('must be set to valid ports'));
+    warn.mockRestore();
+  });
+
+  it('treats blank/absent explicit port vars as unset without warning', () => {
+    const warn = jest.spyOn(console, 'warn').mockImplementation(() => {});
+    const deps: AssignPortsDeps = {
+      ...baseDeps(),
+      env: { SHAKAPERF_CONTROL_PORT: '  ', SHAKAPERF_EXPERIMENT_PORT: '' },
+    };
+    expect(assignPortsAutomatically({ ...pref, key: 'a' }, deps)).toEqual({ control: 3040, experiment: 3050 });
+    expect(warn).not.toHaveBeenCalled();
+    warn.mockRestore();
+  });
+
   it('derives the pair from SHAKAPERF_BASE_PORT and does not persist it', () => {
     const deps: AssignPortsDeps = { ...baseDeps(), env: { SHAKAPERF_BASE_PORT: '4200' } };
     expect(assignPortsAutomatically({ ...pref, key: 'a' }, deps)).toEqual({ control: 4200, experiment: 4201 });

packages/shaka-shared/src/assign-ports.ts

@@ -123,12 +123,53 @@ function lsofPortInUse(port: number): boolean {
   }
 }
 
+/**
+ * Parse one explicit-port env var (`SHAKAPERF_CONTROL_PORT` /
+ * `SHAKAPERF_EXPERIMENT_PORT`). Returns the port when present and valid; null
+ * when absent or blank (silently). Validation mirrors `readBasePortOverride`
+ * for malformed and privileged values, but explicit ports may use the full
+ * `1..MAX_PORT` range because no derived experiment offset is added. Keeps a
+ * typo'd value from being swallowed silently.
+ */
+function parseExplicitPort(env: NodeJS.ProcessEnv, varName: string): number | null {
+  const raw = env[varName];
+  if (raw == null) return null;
+  const stripped = raw.trim();
+  if (stripped === '') return null;
+  const port = Number(stripped);
+  if (!/^\d+$/.test(stripped) || port < 1 || port > MAX_PORT) {
+    console.warn(`assignPortsAutomatically: ${varName}="${raw}" is not a valid port (1..${MAX_PORT}); ignoring.`);
+    return null;
+  }
+  if (port <= PRIVILEGED_PORT_MAX) {
+    console.warn(
+      `assignPortsAutomatically: ${varName}="${raw}" is in the privileged range ` +
+        `(1..${PRIVILEGED_PORT_MAX}); binding will fail without root.`,
+    );
+  }
+  return port;
+}
+
 function readEnvOverride(env: NodeJS.ProcessEnv): AssignedPorts | null {
-  const control = Number(env.SHAKAPERF_CONTROL_PORT);
-  const experiment = Number(env.SHAKAPERF_EXPERIMENT_PORT);
-  if (Number.isInteger(control) && control > 0 && Number.isInteger(experiment) && experiment > 0) {
+  const control = parseExplicitPort(env, 'SHAKAPERF_CONTROL_PORT');
+  const experiment = parseExplicitPort(env, 'SHAKAPERF_EXPERIMENT_PORT');
+  if (control != null && experiment != null) {
     return { control, experiment };
   }
+  // Both vars are required to pin an explicit pair. If exactly one side was
+  // provided, warn that the whole override was dropped. When both sides were
+  // provided but one was malformed, parseExplicitPort has already emitted the
+  // specific warning, so avoid adding a second generic message.
+  const isPresent = (value?: string): boolean => value != null && value.trim() !== '';
+  const controlPresent = isPresent(env.SHAKAPERF_CONTROL_PORT);
+  const experimentPresent = isPresent(env.SHAKAPERF_EXPERIMENT_PORT);
+  if (controlPresent !== experimentPresent) {
+    console.warn(
+      'assignPortsAutomatically: both SHAKAPERF_CONTROL_PORT and ' +
+        'SHAKAPERF_EXPERIMENT_PORT must be set to valid ports to pin an explicit ' +
+        'pair; falling through to automatic assignment.',
+    );
+  }
   return null;
 }