From 9b41b909e1022299928157f75838d2b42c611cef Mon Sep 17 00:00:00 2001 From: Danilo Lakovic Date: Thu, 29 Oct 2026 22:01:40 +0100 Subject: [PATCH 1/5] Make the private denylist guard fail closed on an empty list and warn when unset A configured denylist with no active entry matched nothing and let every change through without output; it is now an error that names the setting without printing any entry. When no denylist is configured the guard still runs the public rules, but now says so on every run, including from the pre-commit and pre-push hooks with nothing to scan. --- README.md | 4 +++- scripts/check-public.mjs | 33 +++++++++++++++++++----------- scripts/check-public.test.mjs | 38 ++++++++++++++++++++++++++++++++++- 3 files changed, 61 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 4d47fc2c..50a02cef 100644 --- a/README.md +++ b/README.md @@ -199,7 +199,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 `/`. diff --git a/scripts/check-public.mjs b/scripts/check-public.mjs index db25ff04..5558b461 100644 --- a/scripts/check-public.mjs +++ b/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) { @@ -780,6 +789,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 +809,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 = diff --git a/scripts/check-public.test.mjs b/scripts/check-public.test.mjs index 344977f5..03e764bd 100644 --- a/scripts/check-public.test.mjs +++ b/scripts/check-public.test.mjs @@ -189,6 +189,14 @@ describe('denylist rule', () => { } }); + it('refuses a list with no active entry', () => { + for (const text of ['', '\n\n', '# comment only\n\n \n', '\r\n#x\r\n']) { + expect(() => rules('', text)).toThrow(/holds no entries/); + } + expect(() => rules('', 'entry\n')).not.toThrow(); + expect(rules('', null).denylist).toBeNull(); + }); + it('fails closed when the configured file is missing', () => { const dir = mkdtempSync(join(tmpdir(), 'guard-deny-')); try { @@ -422,7 +430,35 @@ describe('pre-commit hook in a real repository', () => { const dir = newRepo(root, 'empty'); const res = run('node', [SCRIPT, '--staged'], { cwd: dir }); expect(res.status).toBe(0); - expect(res.output).toBe(''); + expect(res.output).toMatch(/no private denylist configured/); + }); + + it('warns on every run when no denylist is configured, even with nothing to scan', () => { + const dir = newRepo(root, 'warn-hooks'); + const pre = run('sh', [join(HOOKS, 'pre-commit')], { cwd: dir }); + expect(pre.status).toBe(0); + expect(pre.stderr).toMatch(/no private denylist configured/); + const push = run('sh', [join(HOOKS, 'pre-push'), 'origin', 'none'], { + cwd: dir, + input: '', + }); + expect(push.status).toBe(0); + expect(push.stderr).toMatch(/no private denylist configured/); + }); + + it('refuses a configured denylist that holds no entries and names the setting', () => { + const dir = newRepo(root, 'empty-list'); + const file = join(root, 'deny-empty.txt'); + writeFileSync(file, '# only a comment zzcommenttext\n\n \n'); + stage(dir, 'notes.ts', '// zzanything\n'); + const res = commit(dir, { PUBLIC_CHECK_DENYLIST: file }); + expect(res.status).not.toBe(0); + expect(res.output).toMatch(/PUBLIC_CHECK_DENYLIST/); + expect(res.output).toMatch(/publiccheck\.denylist/); + expect(res.output).toMatch(/holds no entries/); + expect(res.output).not.toContain('zzcommenttext'); + writeFileSync(file, ''); + expect(commit(dir, { PUBLIC_CHECK_DENYLIST: file }).status).not.toBe(0); }); }); From 4214cd99051c738838e757c8b953c4439dd2d6fe Mon Sep 17 00:00:00 2001 From: Danilo Lakovic Date: Thu, 29 Oct 2026 22:02:16 +0100 Subject: [PATCH 2/5] Scan files with a NUL byte or a -diff attribute as text Git reports such files as binary, so staged and ranged diffs held no hunks for them, and the full-tree scan skipped any buffer with a NUL. A private address inside one was never seen. Diffs now use --text and --no-textconv, and the full-tree scan decodes NUL-bearing files one byte per character instead of skipping them. --- scripts/check-public.mjs | 15 +++++++++---- scripts/check-public.test.mjs | 40 +++++++++++++++++++++++++++++------ 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/scripts/check-public.mjs b/scripts/check-public.mjs index 5558b461..985c1f6c 100644 --- a/scripts/check-public.mjs +++ b/scripts/check-public.mjs @@ -583,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/', ]; @@ -670,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 }; } diff --git a/scripts/check-public.test.mjs b/scripts/check-public.test.mjs index 03e764bd..4c5ab456 100644 --- a/scripts/check-public.test.mjs +++ b/scripts/check-public.test.mjs @@ -582,16 +582,42 @@ describe('modes in real repositories', () => { expect(res2.stdout).toMatch(/^zzrangetoken\.ts:0: denylist: zzrangetoken/m); }); - it('skips binary files', () => { - const dir = newRepo(root, 'binary'); - writeFileSync( - join(dir, 'blob.bin'), - Buffer.concat([Buffer.from([0, 1, 2, 0]), Buffer.from(leak)]), - ); + it('scans a NUL-bearing file in --all and in a staged change', () => { + const dir = newRepo(root, 'binary', { hooks: false }); + const body = Buffer.concat([ + Buffer.from('host ' + ip(10, 1, 2, 3) + ' here\n'), + Buffer.from([0, 1, 2, 0]), + ]); + writeFileSync(join(dir, 'blob.bin'), body); git(dir, ['add', 'blob.bin']); + const staged = run('node', [SCRIPT, '--staged'], { cwd: dir }); + expect(staged.status).toBe(1); + expect(staged.stdout).toMatch(/^blob\.bin:1: private-address: /m); const res = git(dir, ['commit', '-q', '-m', 'Add blob']); expect(res.status, res.output).toBe(0); - expect(check(dir, ['--all']).status).toBe(0); + const all = check(dir, ['--all']); + expect(all.status).toBe(1); + expect(all.stdout).toMatch(/^blob\.bin:1: private-address: /m); + const head = git(dir, ['rev-parse', 'HEAD']).stdout.trim(); + const ranged = check(dir, ['--range', `${head}^!`]); + expect(ranged.status).toBe(1); + expect(ranged.stdout).toMatch(/^blob\.bin:1: private-address: /m); + }); + + it('scans a file marked -diff in .gitattributes', () => { + const dir = newRepo(root, 'nodiff', { hooks: false }); + write(dir, '.gitattributes', '*.md -diff\n'); + write(dir, 'notes.md', 'host ' + ip(10, 1, 2, 3) + ' here\n'); + git(dir, ['add', '.gitattributes', 'notes.md']); + const staged = run('node', [SCRIPT, '--staged'], { cwd: dir }); + expect(staged.status).toBe(1); + expect(staged.stdout).toMatch(/^notes\.md:1: private-address: /m); + const res = git(dir, ['commit', '-q', '-m', 'Add notes']); + expect(res.status, res.output).toBe(0); + const head = git(dir, ['rev-parse', 'HEAD']).stdout.trim(); + const range = check(dir, ['--range', `${head}^!`]); + expect(range.status).toBe(1); + expect(range.stdout).toMatch(/^notes\.md:1: private-address: /m); }); it('does not mistake an added line that starts with plus signs for a header', () => { From 30b99275b7faa4fd5d704148c329095e0ce791f0 Mon Sep 17 00:00:00 2001 From: Danilo Lakovic Date: Thu, 29 Oct 2026 22:03:01 +0100 Subject: [PATCH 3/5] Record no Playwright trace for projects that sign in to a deployment A trace keeps the headers of every request, so a failing run left the Basic credential of the sign-in requests in the test output and the HTML report. The setup, teardown, remote and crawl projects now run untraced; the smoke project keeps its failure traces. --- README.md | 6 ++++-- playwright.config.ts | 19 ++++++++++++++++--- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 50a02cef..efcc358a 100644 --- a/README.md +++ b/README.md @@ -189,8 +189,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 diff --git a/playwright.config.ts b/playwright.config.ts index a18e59f5..9e9005c6 100644 --- a/playwright.config.ts +++ b/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'), From 0cb89bd0311ea1011f4b97df3ec1cc530112529e Mon Sep 17 00:00:00 2001 From: Danilo Lakovic Date: Thu, 29 Oct 2026 22:03:15 +0100 Subject: [PATCH 4/5] Assert on booleans where a failure would print a secret A failed not.toContain(password) prints the received string, which here is the stored text holding the password, into the terminal, the HTML report and the error context files. The storage and cookie assertions now compare a boolean and carry a message instead. --- e2e/remote/session.spec.ts | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/e2e/remote/session.spec.ts b/e2e/remote/session.spec.ts index c44948b4..eddaf807 100644 --- a/e2e/remote/session.spec.ts +++ b/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; From 48b3d8d133deda3c2d9d8b8b9fd9a84b261cba3b Mon Sep 17 00:00:00 2001 From: Danilo Lakovic Date: Thu, 29 Oct 2026 22:03:56 +0100 Subject: [PATCH 5/5] Bind the dev server to loopback when it proxies to a remote target The proxy presents the target's own Host, Origin and Referer, so a dev server listening on every interface is a relay to the target for anyone on the network. With API_PROXY naming a target that is not on this machine the default bind is now 127.0.0.1; DEV_HOST remains the explicit override. --- README.md | 10 ++++++--- config/dev-host.js | 40 ++++++++++++++++++++++++++++++++++ config/dev-host.test.ts | 48 +++++++++++++++++++++++++++++++++++++++++ config/start.config.js | 9 ++++---- 4 files changed, 100 insertions(+), 7 deletions(-) create mode 100644 config/dev-host.js create mode 100644 config/dev-host.test.ts diff --git a/README.md b/README.md index efcc358a..df64e681 100644 --- a/README.md +++ b/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. diff --git a/config/dev-host.js b/config/dev-host.js new file mode 100644 index 00000000..1e5119ab --- /dev/null +++ b/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 }; diff --git a/config/dev-host.test.ts b/config/dev-host.test.ts new file mode 100644 index 00000000..543c612c --- /dev/null +++ b/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); + }); +}); diff --git a/config/start.config.js b/config/start.config.js index 1c415595..76690c60 100644 --- a/config/start.config.js +++ b/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,