diff --git a/README.md b/README.md index 4d47fc2c..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. @@ -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 `/`. 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, 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; 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'), diff --git a/scripts/check-public.mjs b/scripts/check-public.mjs index db25ff04..985c1f6c 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) { @@ -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 = diff --git a/scripts/check-public.test.mjs b/scripts/check-public.test.mjs index 344977f5..4c5ab456 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); }); }); @@ -546,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', () => {