Harden the public-content guard and the remote e2e secrets handling

README.md

@@ -117,15 +117,19 @@ otherwise. **Do NOT use the webpack proxy in production.**
 Point the dev server at a Pulp that listens elsewhere, including one behind HTTPS:
 
 ```sh
-API_PROXY=https://pulp.example.org DEV_HOST=127.0.0.1 npm start
+API_PROXY=https://pulp.example.org npm start
 ```
 
 - The proxy sends the target's own `Host` and presents the target's origin in the `Origin` and
   `Referer` headers. Pulp checks these for requests that change data after a browser sign-in
   (it refuses them with a CSRF error otherwise), so sign-in, reads, logout and writes all work
   through `http://localhost:8002/ui/`. The proxy adds no credential of its own.
-- `DEV_HOST=127.0.0.1` keeps the dev server reachable from your machine only. It is recommended
-  for a remote target; without it the server listens on all interfaces.
+- The dev server listens on all interfaces when the target is on your machine (the default
+  `http://localhost:8080`), and on `127.0.0.1` only when `API_PROXY` names any other target. The
+  proxy presents the target's own origin, so a dev server that anyone on your network could reach
+  would be a relay to the target. Set `DEV_HOST` to choose the address yourself, for example
+  `DEV_HOST=0.0.0.0` to expose it on purpose or `DEV_HOST=127.0.0.1` to keep a local target
+  private.
 - To use another port, for example when 8002 is taken: `npm start -- --port 8012`.
 - Use Chrome or Firefox. A deployment that sets `Secure` cookies is reached over plain
   `http://localhost`, which these browsers accept for localhost; Safari does not.
@@ -189,8 +193,10 @@ PULP_UI_E2E_BASE_URL=http://127.0.0.1:8002/ui/ \
 npx playwright test --project=remote-admin --project=remote-viewer
 ```
 
-Output of the remote projects (traces, screenshots, the HTML report) can show data of that
-deployment; keep it local.
+Output of the remote projects (screenshots, the HTML report) can show data of that
+deployment; keep it local. These projects record no Playwright trace, because a trace keeps the
+headers of every request, including the sign-in request with the account's credential. The
+smoke project, which runs against a throwaway local instance, keeps traces on failure.
 
 ### Git hooks
 
@@ -199,7 +205,9 @@ refuses internal hostnames, private addresses and other details that must not be
 before a commit and before a push; CI runs it again over the whole tree. You can add a private
 denylist of your own terms in a file outside the repository, named by the
 `PUBLIC_CHECK_DENYLIST` environment variable or by `git config publiccheck.denylist`. When it
-is configured but unreadable, the check fails instead of passing silently.
+is configured but unreadable, or holds no entry, the check fails instead of passing silently.
+When none is configured, the hooks say so on every run: only the public rules ran, and internal
+names that those rules cannot recognise are not checked.
 
 ## Configuration
 A production build (`npm run build`) can be further configured by serving a `/pulp-ui-config.json` alongside the built UI, mapped at `/`. 

config/dev-host.js

@@ -0,0 +1,40 @@
+// Address the dev server binds to.
+//
+// The dev proxy presents the target's own Host, Origin and Referer, which
+// defeats the target's origin checks for any client that reaches the dev
+// server. With a remote target, a dev server listening on every interface
+// would let anyone on the same network use this machine as a relay to a
+// server that may be reachable only from here. So when API_PROXY names a
+// target that is not on this machine the default is loopback only. DEV_HOST is
+// the explicit override in both directions.
+
+const LOOPBACK = '127.0.0.1';
+
+function isLocalTarget(target) {
+  let hostname;
+  try {
+    hostname = new URL(target).hostname.toLowerCase();
+  } catch {
+    // An unparsable target is treated as remote: the safe default.
+    return false;
+  }
+  return (
+    hostname === 'localhost' ||
+    hostname.endsWith('.localhost') ||
+    hostname === '[::1]' ||
+    /^127(\.\d{1,3}){3}$/.test(hostname)
+  );
+}
+
+/** The bind address for the dev server, or undefined to keep the default. */
+function devHost(env = process.env) {
+  if (env.DEV_HOST) {
+    return env.DEV_HOST;
+  }
+  if (env.API_PROXY && !isLocalTarget(env.API_PROXY)) {
+    return LOOPBACK;
+  }
+  return undefined;
+}
+
+module.exports = { devHost, isLocalTarget };

config/dev-host.test.ts

@@ -0,0 +1,48 @@
+// @vitest-environment node
+// The dev server must not listen on every interface while it relays requests
+// to a target that is not on this machine. DEV_HOST stays the explicit
+// override.
+import { createRequire } from 'node:module';
+import { describe, expect, it } from 'vitest';
+
+const require = createRequire(import.meta.url);
+const { devHost, isLocalTarget } = require('./dev-host.js');
+
+describe('the dev server bind address', () => {
+  it('keeps the default when no proxy target is set', () => {
+    expect(devHost({})).toBeUndefined();
+  });
+
+  it.each([
+    'http://localhost:8080',
+    'http://127.0.0.1:8080',
+    'http://127.1.2.3',
+    'http://[::1]:8080',
+    'http://pulp.localhost',
+  ])('keeps the default for the local target %s', (target) => {
+    expect(devHost({ API_PROXY: target })).toBeUndefined();
+  });
+
+  it.each([
+    'https://pulp.example.org',
+    'http://pulp.example.test:8080',
+    'http://pulp.localhost.example.org',
+    'not a url',
+  ])('binds to loopback for the target %s', (target) => {
+    expect(devHost({ API_PROXY: target })).toBe('127.0.0.1');
+  });
+
+  it('lets DEV_HOST override in both directions', () => {
+    expect(
+      devHost({ API_PROXY: 'https://pulp.example.org', DEV_HOST: '0.0.0.0' }),
+    ).toBe('0.0.0.0');
+    expect(
+      devHost({ API_PROXY: 'http://localhost:8080', DEV_HOST: '127.0.0.1' }),
+    ).toBe('127.0.0.1');
+  });
+
+  it('treats only this machine as local', () => {
+    expect(isLocalTarget('http://localhost')).toBe(true);
+    expect(isLocalTarget('https://example.org')).toBe(false);
+  });
+});

config/start.config.js

@@ -1,12 +1,13 @@
+const { devHost } = require('./dev-host');
 const webpackBase = require('./shared.config');
 
 const proxyTarget = process.env.API_PROXY || 'http://localhost:8080';
 
 module.exports = webpackBase({
-  // Address the dev server binds to. The default listens on all interfaces;
-  // set DEV_HOST=127.0.0.1 to keep a dev server that proxies to a remote
-  // Pulp reachable from this machine only.
-  DEV_HOST: process.env.DEV_HOST || undefined,
+  // Address the dev server binds to. Loopback only when API_PROXY names a
+  // target that is not on this machine, all interfaces otherwise; DEV_HOST
+  // overrides either (see config/dev-host.js).
+  DEV_HOST: devHost(),
 
   // Port that the UI is served over (override with: npm start -- --port N).
   DEV_PORT: 8002,

e2e/remote/session.spec.ts

@@ -52,9 +52,17 @@ test.describe('sessions of their own', () => {
     expect(session?.httpOnly, 'session cookie is HttpOnly').toBe(true);
     expect(session?.secure, 'session cookie is Secure').toBe(true);
 
+    // Booleans, never the values: a failed comparison prints both sides, and
+    // these hold the session cookie names and the password.
     const visible = await page.evaluate(() => document.cookie);
-    expect(visible).toContain('csrftoken');
-    expect(visible).not.toContain('sessionid');
+    expect(
+      visible.includes('csrftoken'),
+      'the CSRF cookie is visible to scripts',
+    ).toBe(true);
+    expect(
+      visible.includes('sessionid'),
+      'the session cookie is hidden from scripts',
+    ).toBe(false);
 
     const stored = await page.evaluate(() => ({
       everything: JSON.stringify([
@@ -64,9 +72,18 @@ test.describe('sessions of their own', () => {
       localCredentials: localStorage.getItem('credentials'),
       sessionCredentials: sessionStorage.getItem('credentials'),
     }));
-    expect(stored.everything).not.toContain(password);
-    expect(stored.localCredentials).toBeNull();
-    expect(stored.sessionCredentials).toBeNull();
+    expect(
+      stored.everything.includes(password),
+      'stored text must not contain the password',
+    ).toBe(false);
+    expect(
+      stored.localCredentials === null,
+      'localStorage holds no credentials entry',
+    ).toBe(true);
+    expect(
+      stored.sessionCredentials === null,
+      'sessionStorage holds no credentials entry',
+    ).toBe(true);
 
     // Exactly one call carries the Basic header: the sign in itself.
     const calls = watched.calls;

playwright.config.ts

@@ -12,6 +12,14 @@ import { storageStatePath } from './e2e/support/accounts';
 // sign in once through the API (remote-setup), reuse that session per role, and
 // end it again (remote-teardown). They are meant for a person at a terminal, not
 // for a hosted workflow: their output holds session data.
+// A trace records the headers of every request, and the sign-in requests (the
+// setup project, the API login helpers and the application's own sign-in)
+// carry the account's Basic credential. Projects that run against a real
+// deployment therefore keep no trace; failures are still evidenced by
+// screenshots. Only the smoke project, which runs against a throwaway local
+// instance, keeps traces.
+const untraced = { trace: 'off' } as const;
+
 // The screen crawler (e2e/crawl) visits every route of the application, signed
 // in as the account named by PULP_UI_CRAWL_ACCOUNT (admin unless set to
 // viewer), and writes its report to PULP_UI_CRAWL_OUT (crawl-report/ unless
@@ -25,6 +33,7 @@ function crawlProject(theme: 'light' | 'dark', viewport: 'desktop' | 'phone') {
     testMatch: 'crawl/**/*.spec.ts',
     metadata: { theme, viewport },
     use: {
+      ...untraced,
       colorScheme: theme,
       viewport: SCREEN_SIZES[viewport],
       ...(viewport === 'phone' ? { isMobile: true, hasTouch: true } : {}),
@@ -46,7 +55,9 @@ export default defineConfig({
   reporter: [['list'], ['html', { open: 'never' }]],
   use: {
     baseURL: process.env['PULP_UI_E2E_BASE_URL'],
-    // Failure evidence is kept, successful runs leave nothing behind.
+    // Failure evidence is kept, successful runs leave nothing behind. The
+    // projects that sign in to a real deployment turn tracing off (see
+    // "untraced").
     trace: 'retain-on-failure',
     screenshot: 'only-on-failure',
     viewport: { width: 1920, height: 1080 },
@@ -59,23 +70,25 @@ export default defineConfig({
     {
       name: 'remote-setup',
       testMatch: 'remote/auth.setup.ts',
+      use: untraced,
       teardown: 'remote-teardown',
     },
     {
       name: 'remote-teardown',
       testMatch: 'remote/auth.teardown.ts',
+      use: untraced,
     },
     {
       name: 'remote-admin',
       testMatch: 'remote/**/*.spec.ts',
       testIgnore: '**/*.viewer.spec.ts',
-      use: { storageState: 'playwright/.auth/admin.json' },
+      use: { ...untraced, storageState: 'playwright/.auth/admin.json' },
       dependencies: ['remote-setup'],
     },
     {
       name: 'remote-viewer',
       testMatch: 'remote/**/*.viewer.spec.ts',
-      use: { storageState: 'playwright/.auth/viewer.json' },
+      use: { ...untraced, storageState: 'playwright/.auth/viewer.json' },
       dependencies: ['remote-setup'],
     },
     crawlProject('light', 'desktop'),

scripts/check-public.mjs

@@ -32,9 +32,11 @@
 //
 // Private denylist: a file kept outside the repository, named by the
 // PUBLIC_CHECK_DENYLIST environment variable or by the git configuration key
-// publiccheck.denylist. When it is configured but cannot be read the guard
-// fails closed (exit 1). When nothing is configured only the public rules run
-// and one line says so.
+// publiccheck.denylist. When it is configured but cannot be read, or holds no
+// entry (empty, or only comments and blank lines), the guard fails closed
+// (exit 1): a list that matches nothing would otherwise pass everything in
+// silence. When nothing is configured only the public rules run and one line
+// on standard error says so on every run; CI has no list and relies on that.
 //
 // Usage:
 //   node scripts/check-public.mjs --staged
@@ -240,10 +242,17 @@ export function parseDenylist(text) {
 
 /** Builds the rule set used by scan(). Both arguments are optional text. */
 export function buildRules({ hostsText = '', denylistText = null } = {}) {
-  return {
-    hosts: parseHostList(hostsText),
-    denylist: denylistText === null ? null : parseDenylist(denylistText),
-  };
+  let denylist = null;
+  if (denylistText !== null) {
+    denylist = parseDenylist(denylistText);
+    if (denylist.length === 0) {
+      throw new Error(
+        'the private denylist (PUBLIC_CHECK_DENYLIST or git config ' +
+          'publiccheck.denylist) is configured but holds no entries',
+      );
+    }
+  }
+  return { hosts: parseHostList(hostsText), denylist };
 }
 
 function ipv4Octets(host) {
@@ -574,11 +583,17 @@ function git(args, cwd, input) {
   });
 }
 
+// --text makes git emit hunks for every file. Without it a file with a NUL
+// byte, or one marked "-diff" in .gitattributes, is reported as binary and its
+// content would never be scanned. --no-textconv keeps a configured conversion
+// filter from replacing the committed bytes with other text.
 const DIFF_FLAGS = [
   '-U0',
   '-M',
   '--no-color',
   '--no-ext-diff',
+  '--no-textconv',
+  '--text',
   '--src-prefix=a/',
   '--dst-prefix=b/',
 ];
@@ -661,10 +676,11 @@ function scanAll(rules, cwd) {
     } catch {
       continue;
     }
-    if (buf.includes(0)) {
-      continue;
-    }
-    findings.push(...scan(buf.toString('utf8'), path, rules));
+    // A NUL byte does not make a file safe to publish: decode such a file one
+    // byte per character so any address or name inside it is still seen.
+    findings.push(
+      ...scan(buf.toString(buf.includes(0) ? 'latin1' : 'utf8'), path, rules),
+    );
   }
   return { findings, offending: [], scanned: paths.length };
 }
@@ -780,6 +796,11 @@ export function main(argv) {
       result = scanAll(rules, cwd);
     }
 
+    if (denylistText === null) {
+      process.stderr.write(
+        'check-public: no private denylist configured; only the public rules ran\n',
+      );
+    }
     for (const f of result.findings) {
       process.stdout.write(`${f.path}:${f.line}: ${f.rule}: ${f.token}\n`);
     }
@@ -795,11 +816,6 @@ export function main(argv) {
       );
       return 1;
     }
-    if (denylistText === null && result.scanned > 0) {
-      process.stderr.write(
-        'check-public: no private denylist configured; only the public rules ran\n',
-      );
-    }
     return 0;
   } catch (err) {
     const stderr =