fix(FR-2877): address review findings (insecure-tls gate, --output, --also-include rename, password scrub, detectRole fail-fast, e2e dir check)

Author: yomybaby

packages/backend.ai-webui-smoke-cli/README.md

@@ -7,12 +7,51 @@ for the full spec. The operator-facing README (EN + KO) lands in FR-2883
 
 ## Usage (alpha)
 
+Prefer reading the password from stdin or an env var — passing
+`--password` on the command line exposes the secret to anything that
+can read the host process's argv. The CLI scrubs the argv value at
+startup, but the original invocation may still surface in shell history
+or process listings before the scrub runs.
+
 ```sh
-bai-smoke run \
+# Option 1 — password from stdin (recommended)
+printf '%s' "$BAI_PASSWORD" | bai-smoke run \
+  --endpoint https://webui.example.com \
+  --webserver https://webui.example.com \
+  --email admin@example.com \
+  --password-stdin \
+  --output ./smoke-report
+
+# Option 2 — password from env var
+BAI_SMOKE_PASSWORD="$BAI_PASSWORD" bai-smoke run \
   --endpoint https://webui.example.com \
+  --webserver https://webui.example.com \
   --email admin@example.com \
-  --password "***" \
   --output ./smoke-report
 ```
 
-This is an alpha MVP. Full operator docs land with FR-2883.
+To widen the selection beyond the default `@smoke*` set, use
+`--also-include`. To narrow the run, use `--pages`:
+
+```sh
+bai-smoke run \
+  --endpoint https://webui.example.com \
+  --email admin@example.com \
+  --password-stdin \
+  --also-include "@critical" \
+  --pages session,vfolder
+```
+
+## Limitations (alpha MVP)
+
+- Must be run from a `backend.ai-webui` monorepo checkout — the e2e
+  specs are not bundled yet. Tarball / single-binary distribution is
+  tracked in FR-2881.
+- Air-gap binary, `doctor` command, and rich diagnostic reports → Phase 2.
+- `--also-include` widens the smoke set; to narrow it, use `--pages`
+  instead. The legacy `--include` flag is a deprecated hidden alias and
+  will be removed in a future release.
+- `--insecure-tls` accepts self-signed certs but does not pin
+  fingerprints — only enable on trusted networks.
+
+This is an alpha MVP. Full operator docs (EN + KO) land with FR-2883.

packages/backend.ai-webui-smoke-cli/playwright.smoke.config.ts

@@ -1,4 +1,5 @@
 import { defineConfig, devices } from '@playwright/test';
+import { existsSync } from 'node:fs';
 import path from 'node:path';
 import { fileURLToPath } from 'node:url';
 
@@ -31,6 +32,18 @@ const __filename = fileURLToPath(import.meta.url);
 const __dirname = path.dirname(__filename);
 const E2E_DIR = path.resolve(__dirname, '..', '..', 'e2e');
 
+// FR-2877 MVP limitation: the smoke runner discovers specs from the
+// monorepo's e2e/ tree. A bundled tarball / standalone binary distribution
+// is tracked in FR-2881. Fail loudly when the directory is missing so the
+// user gets a clear message instead of a cryptic Playwright "no tests
+// found" output.
+if (!existsSync(E2E_DIR)) {
+  throw new Error(
+    `bai-smoke MVP requires running from a backend.ai-webui checkout. Expected e2e dir at: ${E2E_DIR}. ` +
+      `Tarball / single-binary distribution lands in FR-2881.`,
+  );
+}
+
 const reportDir = process.env.BAI_SMOKE_REPORT_DIR ?? path.resolve(process.cwd(), 'smoke-report');
 
 const workersEnv = process.env.BAI_SMOKE_WORKERS;
@@ -75,7 +88,7 @@ export default defineConfig({
     trace: 'retain-on-failure',
     video: 'retain-on-failure',
     headless: !headed,
-    ignoreHTTPSErrors: true,
+    ignoreHTTPSErrors: process.env.BAI_SMOKE_INSECURE_TLS === '1',
   },
   projects: [
     {

packages/backend.ai-webui-smoke-cli/src/cli.ts

@@ -96,7 +96,16 @@ program
       .choices(['auto', 'admin', 'user'])
       .default('auto'),
   )
-  .option('--include <tags>', 'Comma-separated extra tags to include (e.g. "@critical").')
+  .option(
+    '--also-include <tags>',
+    'Additional tag pattern(s) to OR onto the smoke selection (e.g. "@critical"). ' +
+      'To NARROW the smoke set, use --pages instead.',
+  )
+  // Backwards-compat alias for the original `--include` name. Deprecated —
+  // will be removed in a future release. Kept hidden from the main help.
+  .addOption(
+    new Option('--include <tags>', '(deprecated) alias for --also-include.').hideHelp(),
+  )
   .option('--exclude <tags>', 'Comma-separated tags to exclude.')
   .option(
     '--pages <names>',
@@ -108,12 +117,7 @@ program
     'Per-test timeout. Accepts "180s", "3m", or raw ms.',
     '180s',
   )
-  .option(
-    '--output <dir>',
-    'Output directory for the smoke report.',
-    () => defaultOutputDir(),
-    defaultOutputDir(),
-  )
+  .option('--output <dir>', 'Output directory for the smoke report.')
   .option('--headed', 'Run the browser in headed mode (debugging only).', false)
   .option('--insecure-tls', 'Accept self-signed TLS certificates.', false)
   .action(async (raw: Record<string, unknown>) => {
@@ -124,6 +128,9 @@ program
       );
       process.exit(2);
     }
+    // Scrub the password value from process.argv so accidental introspection
+    // (logs, crash dumps, `ps`-style helpers reading argv) cannot leak it.
+    scrubPasswordFromArgv();
 
     const endpoint = String(raw.endpoint);
     const webserver =
@@ -145,18 +152,25 @@ program
       return;
     }
 
+    // `--also-include` is the canonical flag; `--include` is a hidden alias.
+    const alsoIncludeRaw =
+      (raw.alsoInclude as string | string[] | undefined) ??
+      (raw.include as string | string[] | undefined);
+
+    const outputDir = path.resolve(String(raw.output ?? defaultOutputDir()));
+
     const opts: SmokeRunOptions = {
       endpoint,
       webserver,
       email: String(raw.email),
       password,
       role: (raw.role as SmokeRoleSelection) ?? 'auto',
-      include: splitCsvArg(raw.include as string | string[] | undefined),
+      include: splitCsvArg(alsoIncludeRaw),
       exclude: splitCsvArg(raw.exclude as string | string[] | undefined),
       pages: splitCsvArg(raw.pages as string | string[] | undefined),
       workers: typeof raw.workers === 'number' && raw.workers > 0 ? raw.workers : undefined,
       timeoutMs,
-      outputDir: path.resolve(String(raw.output ?? defaultOutputDir())),
+      outputDir,
       headed: raw.headed === true,
       insecureTls: raw.insecureTls === true,
     };
@@ -203,6 +217,27 @@ async function resolvePassword(raw: Record<string, unknown>): Promise<string | u
   return undefined;
 }
 
+/**
+ * Replace the resolved password value in `process.argv` with `***` so
+ * downstream introspection (crash dumps, logs that dump argv, child
+ * processes inheriting argv via APIs that read the parent's command
+ * line) cannot leak the secret. Covers both `--password VALUE` and
+ * `--password=VALUE` forms.
+ */
+function scrubPasswordFromArgv(): void {
+  for (let i = 0; i < process.argv.length; i++) {
+    const a = process.argv[i];
+    if (a == null) continue;
+    if (a.startsWith('--password=')) {
+      process.argv[i] = '--password=***';
+      continue;
+    }
+    if (a === '--password' && i > 0 && process.argv[i + 1] != null) {
+      process.argv[i + 1] = '***';
+    }
+  }
+}
+
 function readStdin(): Promise<string> {
   return new Promise((resolve, reject) => {
     const chunks: Buffer[] = [];

packages/backend.ai-webui-smoke-cli/src/config.ts

@@ -20,7 +20,10 @@ export interface SmokeRunOptions {
   password: string;
   /** Role selection mode (`--role`). */
   role: SmokeRoleSelection;
-  /** Extra include tags (`--include @critical,@dashboard` → `['@critical','@dashboard']`). */
+  /**
+   * Extra tags to OR onto the smoke selection. Populated from
+   * `--also-include` (preferred) or the deprecated `--include` alias.
+   */
   include?: string[];
   /** Exclude tags (`--exclude`). */
   exclude?: string[];
@@ -52,7 +55,7 @@ export interface SmokeRunOptions {
  * so a `user` run picks up `@smoke`, `@smoke-any`, and `@smoke-user` —
  * but NOT `@smoke-admin`.
  *
- * Additional `--include` tags are OR-ed in. `--exclude` tags become
+ * Additional `--also-include` tags are OR-ed in. `--exclude` tags become
  * `grepInvert`.
  */
 export function buildGrepExpression(
@@ -122,8 +125,11 @@ export function buildPlaywrightEnv(
 
   if (opts.insecureTls) {
     // Forward to both the runner's spawned children and any fetches
-    // inside the test harness. Only set when explicitly requested.
+    // inside the test harness. Only set on the spawned child env — we
+    // intentionally do NOT mutate process.env in the host runner.
     env.NODE_TLS_REJECT_UNAUTHORIZED = '0';
+    // Gate Playwright's `ignoreHTTPSErrors` in playwright.smoke.config.ts.
+    env.BAI_SMOKE_INSECURE_TLS = '1';
   }
 
   return env;

packages/backend.ai-webui-smoke-cli/src/runner.ts

@@ -32,7 +32,8 @@ export interface SmokeSummary {
   webserver: string;
   role: EffectiveRole;
   roleSelection: SmokeRunOptions['role'];
-  include?: string[];
+  /** Tags OR-ed onto the smoke selection via `--also-include`. */
+  alsoInclude?: string[];
   exclude?: string[];
   pages?: string[];
   grep?: string;
@@ -56,27 +57,48 @@ export interface SmokeSummary {
 /**
  * Auto-detect role by hitting the webserver's /server/login endpoint.
  *
- * On failure we conservatively default to `'user'` — fewer privileged
- * specs attempted is safer than running admin-only specs against a
- * misclassified account.
+ * On any failure (network error, non-200 status, malformed JSON,
+ * `authenticated !== true`) we throw. Silently defaulting to `'user'`
+ * was masking real misconfiguration — operators ended up with a green
+ * smoke run that had skipped every admin-tagged spec without saying so.
+ *
+ * The body shape we POST here (`{ username, password }`) mirrors what
+ * the SESSION-mode browser fixture submits to `/server/login`. The
+ * proper signed-request based detection — using the post-login
+ * `/func/auth/role` endpoint — is tracked in TODO(FR-2878).
  */
 export async function detectRole(opts: SmokeRunOptions): Promise<EffectiveRole> {
   const url = `${opts.webserver.replace(/\/$/, '')}/server/login`;
   // Honour --insecure-tls for the detection call so self-signed certs
-  // don't crash detection before we even reach Playwright.
+  // don't crash detection before we even reach Playwright. We only
+  // mutate the spawned-process env; the host runner's process.env is
+  // restored in `finally`.
   const previousTlsReject = process.env.NODE_TLS_REJECT_UNAUTHORIZED;
   if (opts.insecureTls) process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0';
   try {
-    const res = await fetch(url, {
-      method: 'POST',
-      headers: { 'Content-Type': 'application/json' },
-      body: JSON.stringify({ username: opts.email, password: opts.password }),
-    });
+    let res: Response;
+    try {
+      res = await fetch(url, {
+        method: 'POST',
+        headers: { 'Content-Type': 'application/json' },
+        // TODO(FR-2878): switch to signed-request /func/auth/role for a
+        // proper detection. This naive form matches what the SESSION-mode
+        // browser fixture submits to /server/login and works against the
+        // current staging cluster.
+        body: JSON.stringify({ username: opts.email, password: opts.password }),
+      });
+    } catch (err) {
+      throw new Error(
+        `Failed to auto-detect role for ${opts.email} against ${opts.webserver}: ` +
+          `${(err as Error).message}. Pass --role admin or --role user explicitly and retry.`,
+      );
+    }
     if (!res.ok) {
-      process.stderr.write(
-        `[bai-smoke] role detection: webserver returned HTTP ${res.status}; defaulting to "user".\n`,
+      throw new Error(
+        `Failed to auto-detect role for ${opts.email} against ${opts.webserver}: ` +
+          `webserver returned HTTP ${res.status}. ` +
+          `Pass --role admin or --role user explicitly and retry.`,
       );
-      return 'user';
     }
     const json = (await res.json().catch(() => null)) as
       | {
@@ -85,22 +107,18 @@ export async function detectRole(opts: SmokeRunOptions): Promise<EffectiveRole>
         }
       | null;
     if (!json || json.authenticated !== true) {
-      process.stderr.write(
-        '[bai-smoke] role detection: login not authenticated; defaulting to "user".\n',
+      throw new Error(
+        `Failed to auto-detect role for ${opts.email} against ${opts.webserver}: ` +
+          `login not authenticated (unexpected response shape or wrong credentials). ` +
+          `Pass --role admin or --role user explicitly and retry.`,
       );
-      return 'user';
     }
     const role = json.data?.role;
     const isAdmin = json.data?.is_admin === true;
     if (role === 'admin' || role === 'superadmin' || isAdmin) {
       return 'admin';
     }
     return 'user';
-  } catch (err) {
-    process.stderr.write(
-      `[bai-smoke] role detection failed: ${(err as Error).message}; defaulting to "user".\n`,
-    );
-    return 'user';
   } finally {
     if (opts.insecureTls) {
       if (previousTlsReject === undefined) {
@@ -175,7 +193,7 @@ export async function runSmoke(opts: SmokeRunOptions): Promise<SmokeRunResult> {
     webserver: opts.webserver,
     role: effectiveRole,
     roleSelection: opts.role,
-    include: opts.include,
+    alsoInclude: opts.include,
     exclude: opts.exclude,
     pages: opts.pages,
     grep,