diff --git a/CHANGELOG.md b/CHANGELOG.md index 527d79b..ec3a704 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -90,8 +90,17 @@ minor is for. that terminal corrupted `vendor/` and `node_modules/`. - `composer install` retries: a transient registry error no longer ends a first run. -- The dev server refuses to start on a taken port. Nuxt falls back to a - random one, which silently breaks the OAuth callback. +- The dev server moves to the next free port between 3000 and 3009 when + 3000 is taken, and prints which one it took. Nuxt's own fallback picks a + random port, which silently breaks the OAuth callback; every port in + that range has a callback registered, so any of them is safe. A `PORT` + you name is still yours - a busy one fails, rather than moving + somewhere you did not ask for. +- The dev server refuses a port with no registered OAuth callback. A + `PORT` outside 3000-3009 that `OAUTH_CALLBACK` does not name started + fine and then failed only at login, since the browser builds its + callback from the port it is on. Backends this repo did not provision + are left alone: their consumers were registered out of sight. - The dev container no longer leaves Xdebug active, which made every `php` and `composer` call wait for a debugger. - The druxt patch is described without a link to a private merge diff --git a/README.md b/README.md index 9936d53..8d13962 100644 --- a/README.md +++ b/README.md @@ -81,7 +81,8 @@ WSL2, or a container backend - see [Windows](#windows). ``` - Drupal backend: http://127.0.0.1:8888 - - Nuxt frontend: http://localhost:3000 + - Nuxt frontend: http://localhost:3000 (or the next free port up to + 3009, which it prints) - One-time Drupal login: `npm run login` `npm run dev` and `npm run start` automatically start the local backend @@ -150,6 +151,23 @@ Then: 3. `npm run dev` as above. `npm run drush -- ` is proxied through `lando drush`. +### Troubleshooting + +#### Port 3000 is already in use + +`npm run dev` takes the next free port between 3000 and 3009 and says +which one it picked. Provisioning registers an OAuth callback for every +port in that range, so login keeps working on whichever one it uses. + +Naming a port yourself turns that off: `PORT=3005 npm run dev` uses +3005 or fails, because a port you asked for is a decision rather than a +default. If the whole range is busy, `npm run dev` says so instead of +letting Nuxt fall back to a random port and break login. + +A port outside 3000-3009 has no registered callback, so `npm run dev` +refuses that too. To use one, set `OAUTH_CALLBACK` in `.env` to +`http://localhost:/callback` and re-run `npm run provision`. + #### Login fails with invalid_client in a dev container The browser builds the OAuth callback from its own address. An IDE @@ -198,7 +216,7 @@ npm run dev ``` - Drupal backend: http://127.0.0.1:8888 -- Nuxt frontend: http://localhost:3000 +- Nuxt frontend: http://localhost:3000 (or the next free port up to 3009) ## How to use it @@ -210,7 +228,7 @@ In a Development Container (VS Code, Codespaces, DevPod), forwarded ports are ac | Port | Service | | ------ | ------------------------------------------------------------------------------------- | -| `3000` | Nuxt.js | +| `3000` | Nuxt.js (3000-3009: `npm run dev` takes the first free one) | | `3003` | Storybook | | `8888` | Drupal (local `.devtools` backend - DDEV serves at its own `*.ddev.site` URL instead) | diff --git a/scripts/dev.mjs b/scripts/dev.mjs index 4adc602..0fdf58f 100644 --- a/scripts/dev.mjs +++ b/scripts/dev.mjs @@ -7,89 +7,164 @@ import { checkOauth } from './check-oauth.mjs' import { + FRONTEND_PORTS, NUXT_DIR, + backendIsProvisionedHere, ensureBackend, ensureOauthClientId, exitWithError, + firstFreePort, foregroundNpm, isPortOpen, + portIsRegistered, readEnv, } from './lib.mjs' -const PORT = Number(process.env.PORT) || 3000 +// Nuxt binds 0.0.0.0 (see nuxt.config.js), which answers on loopback +// too, so this is the probe for "is that port taken". +const HOST = '127.0.0.1' +const PORT_RANGE = `${FRONTEND_PORTS[0]}-${FRONTEND_PORTS[FRONTEND_PORTS.length - 1]}` +const ENV_PORT = Number(process.env.PORT) +// A usable PORT in the environment is a decision; the default is only a +// starting point. An empty or unparsable one is neither. +const PORT_IS_EXPLICIT = Number.isInteger(ENV_PORT) && ENV_PORT > 0 +const REQUESTED_PORT = PORT_IS_EXPLICIT ? ENV_PORT : FRONTEND_PORTS[0] /** - * Refuse to start when the frontend port is taken. + * Pick the port to serve the frontend on. * * Nuxt's dev server does not fail on a busy port - it falls back to a * random one. The OAuth consumer in Drupal is registered against a fixed - * callback URL, so the login round trip then fails with a bare + * set of callback URLs, so the login round trip then fails with a bare * `invalid_client` from Drupal, pointing nowhere near the real cause. + * + * Provisioning registers all of FRONTEND_PORTS for exactly that reason, + * which makes a busy default a choice rather than a failure: take the + * next registered port and say so. A port the user named is theirs. + * + * Something else can still take the port between this check and Nuxt + * binding it, which lands back on Nuxt's own random fallback - the same + * place an unguarded start would have been anyway. */ -async function ensureFrontendPortFree() { - if (!(await isPortOpen('127.0.0.1', PORT))) { - return +async function resolveFrontendPort() { + if (!(await isPortOpen(HOST, REQUESTED_PORT))) { + return REQUESTED_PORT } - const callback = readEnv().OAUTH_CALLBACK || `http://localhost:${PORT}/callback` - exitWithError( - `Port ${PORT} is already in use.\n\n` + - ` Nuxt would fall back to a random port, and login would then fail with\n` + - ` {"error":"invalid_client"} - Drupal has the consumer registered for\n` + - ` ${callback}, which would no longer match.\n\n` + - ` Free the port (another dev server, or another copy of this project),\n` + - ` or commit to a different one: set OAUTH_CALLBACK in .env to the port\n` + - ` you want, re-run \`npm run provision\` to re-register the consumer,\n` + - ` then start with \`PORT= npm run dev\`.` - ) + if (PORT_IS_EXPLICIT) { + exitWithError( + `Port ${REQUESTED_PORT} is already in use, and PORT asks for it by name.\n\n` + + ` Nuxt would fall back to a random port, and login would then fail with\n` + + ` {"error":"invalid_client"} - Drupal only accepts a callback it has\n` + + ` registered.\n\n` + + ` Free the port (another dev server, or another copy of this project),\n` + + ` or drop PORT and let \`npm run dev\` take the first free one of\n` + + ` ${PORT_RANGE}.` + ) + } + + const port = await firstFreePort(HOST) + if (port === null) { + exitWithError( + `Ports ${PORT_RANGE} are all in use.\n\n` + + ` Nuxt would fall back to a random port, and login would then fail with\n` + + ` {"error":"invalid_client"} - those are the only callbacks Drupal has\n` + + ` registered.\n\n` + + ` Free one of them, or commit to a port outside the range: set\n` + + ` OAUTH_CALLBACK in .env to that port, re-run \`npm run provision\` to\n` + + ` re-register the consumer, then start with \`PORT= npm run dev\`.` + ) + } + + console.log(`Port ${REQUESTED_PORT} is in use - starting on ${port} instead.`) + console.log(`Login still works: Drupal accepts the callback on any of ${PORT_RANGE}.`) + console.log('') + return port +} + +/** The port OAUTH_CALLBACK names, or null when it names nothing usable. */ +function callbackPort(callback) { + if (!callback) { + return null + } + try { + const parsed = new URL(callback) + return Number(parsed.port) || (parsed.protocol === 'https:' ? 443 : 80) + } catch { + return null + } } /** - * The consumer is registered for one callback URL. Serving the frontend - * on a different port than that URL names fails the same way a busy - * port does, just without anything else looking wrong. + * Refuse a frontend port Drupal has no callback registered for. + * + * The browser builds redirect_uri from its own origin, so serving on an + * unregistered port fails login with `invalid_client` while the rest of + * the site works. Provisioning registers FRONTEND_PORTS plus whatever + * OAUTH_CALLBACK names, and those are the only safe ports. + * + * Checking REQUESTED_PORT covers the port actually served: resolution + * either keeps that port or moves inside FRONTEND_PORTS, which is + * registered either way. */ -function ensureCallbackMatchesPort() { - const callback = readEnv().OAUTH_CALLBACK - if (!callback) { +function ensurePortHasCallback(backend) { + if (portIsRegistered(REQUESTED_PORT)) { return } - let parsed - try { - parsed = new URL(callback) - } catch { + const callback = readEnv().OAUTH_CALLBACK + const port = callbackPort(callback) + if (port === REQUESTED_PORT) { return } - const callbackPort = Number(parsed.port) || (parsed.protocol === 'https:' ? 443 : 80) - if (callbackPort === PORT) { + if (port !== null) { + exitWithError( + `OAUTH_CALLBACK names port ${port}, but the dev server would run on ${REQUESTED_PORT}.\n\n` + + ` Login would fail with {"error":"invalid_client"} - Drupal only accepts the\n` + + ` callback it has registered (${callback}).\n\n` + + ` Either start on that port with \`PORT=${port} npm run dev\`, or set\n` + + ` OAUTH_CALLBACK to port ${REQUESTED_PORT} and re-run \`npm run provision\` to\n` + + ` re-register the consumer.` + ) + } + + // Nothing registers this port. Only say so for a backend this repo + // provisioned - a remote one registered its consumer out of sight. + if (!backendIsProvisionedHere(backend)) { return } exitWithError( - `OAUTH_CALLBACK names port ${callbackPort}, but the dev server would run on ${PORT}.\n\n` + - ` Login would fail with {"error":"invalid_client"} - Drupal only accepts the\n` + - ` callback it has registered (${callback}).\n\n` + - ` Either start on that port with \`PORT=${callbackPort} npm run dev\`, or set\n` + - ` OAUTH_CALLBACK to port ${PORT} and re-run \`npm run provision\` to\n` + - ` re-register the consumer.` + `PORT is ${REQUESTED_PORT}, which has no OAuth callback registered.\n\n` + + ` Login would fail with {"error":"invalid_client"} while the rest of the\n` + + ` site works - the browser builds its callback from the port it is on,\n` + + ` and Drupal registers ${PORT_RANGE} plus whatever OAUTH_CALLBACK names.\n\n` + + ` Use a port from ${PORT_RANGE}, or set OAUTH_CALLBACK in .env to\n` + + ` http://localhost:${REQUESTED_PORT}/callback and re-run \`npm run provision\`\n` + + ` to register it.` ) } async function main() { - await ensureBackend() + const backend = await ensureBackend() ensureOauthClientId() - ensureCallbackMatchesPort() - await ensureFrontendPortFree() + ensurePortHasCallback(backend) // Confirm the backend will actually accept this consumer. Nuxt reads // OAUTH_CLIENT_ID once at startup, so a stale value - or a consumer // left over from an older provision - shows up only as a failed login // in the browser, with nothing in the terminal to explain it. await checkOauth() - console.log(`Starting the Nuxt dev server -> http://localhost:${PORT}`) + // Last thing before the spawn. Everything above is config, and none of + // it needs the port, so choosing one here leaves the smallest window + // for another process to take it in the meantime. + const port = await resolveFrontendPort() + console.log(`Starting the Nuxt dev server -> http://localhost:${port}`) console.log('') - process.exitCode = await foregroundNpm(['run', 'dev'], { cwd: NUXT_DIR }) + process.exitCode = await foregroundNpm(['run', 'dev'], { + cwd: NUXT_DIR, + env: { PORT: String(port) }, + }) } main().catch((error) => exitWithError(error.message)) diff --git a/scripts/lib.mjs b/scripts/lib.mjs index b00efc8..8cd2ebe 100644 --- a/scripts/lib.mjs +++ b/scripts/lib.mjs @@ -225,6 +225,42 @@ export async function waitForPort(host, port, timeoutSeconds = 30) { return false } +/** + * The ports the frontend may serve on. Provisioning registers an OAuth + * callback for every one of them (drupal/.devtools/provision and + * .ddev/commands/web/druxt-add-consumer), so moving between them never + * breaks login. Anything outside the list does, because the browser + * builds redirect_uri from its own origin and Drupal rejects an + * unregistered one as, confusingly, invalid_client. + */ +export const FRONTEND_PORTS = Array.from({ length: 10 }, (_, index) => 3000 + index) + +/** + * True when this repo's own tooling provisioned the backend, and so + * knows what its OAuth consumer has registered. A remote backend was + * set up somewhere this checkout cannot see, so its registrations are + * not this repo's to assert. + */ +export function backendIsProvisionedHere(backend) { + return Boolean(backend.managed || backend.ddev || backend.lando) +} + +/** True when a port has an OAuth callback registered for it. */ +export function portIsRegistered(port) { + return FRONTEND_PORTS.includes(port) +} + +/** + * The first of `ports` nothing is listening on, or null when they are + * all taken. Checked in order, so a free 3000 always wins. + */ +export async function firstFreePort(host, ports = FRONTEND_PORTS) { + for (const port of ports) { + if (!(await isPortOpen(host, port, 500))) return port + } + return null +} + /** * Run a command to completion, inheriting stdio. Throws on failure. */ diff --git a/test/guards.test.mjs b/test/guards.test.mjs index 94743f8..91b5195 100644 --- a/test/guards.test.mjs +++ b/test/guards.test.mjs @@ -11,12 +11,15 @@ import assert from 'node:assert/strict' import { spawn } from 'node:child_process' import fs from 'node:fs' +import http from 'node:http' import net from 'node:net' import os from 'node:os' import path from 'node:path' import { fileURLToPath } from 'node:url' import { after, before, describe, it } from 'node:test' +import { FRONTEND_PORTS } from '../scripts/lib.mjs' + const REPO = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..') let workspace @@ -38,6 +41,40 @@ function occupy() { }) } +/** + * Hold one named port, or resolve null when something else already + * holds it - either way the guard under test sees it as taken. + */ +function occupyPort(port) { + return new Promise((resolve) => { + const server = net.createServer() + server.once('error', () => resolve(null)) + server.listen(port, '127.0.0.1', () => resolve(server)) + }) +} + +/** + * A backend that answers the OAuth check the way a provisioned Drupal + * does: authorize redirects to the login form, and a bogus code is + * rejected as a bad code. dev.mjs gets past `checkOauth` and reaches + * its port handling, then fails trying to start Nuxt - there is no + * nuxt/ in the throwaway workspace, which is what ends each run. + */ +function stubBackend() { + return new Promise((resolve) => { + const server = http.createServer((request, response) => { + if (request.url.startsWith('/oauth/token')) { + response.writeHead(400, { 'content-type': 'application/json' }) + response.end(JSON.stringify({ error: 'invalid_grant' })) + return + } + response.writeHead(302, { location: '/user/login' }) + response.end() + }) + server.listen(0, '127.0.0.1', () => resolve({ server, port: server.address().port })) + }) +} + function writeEnv(lines) { fs.writeFileSync(path.join(workspace, '.env'), `${lines.join('\n')}\n`) } @@ -45,7 +82,10 @@ function writeEnv(lines) { /** Async so an in-process stub server can still answer the child. */ function run(script, { env = {}, stripPhp = false } = {}) { return new Promise((resolve) => { - const childEnv = { ...process.env, ...env } + // PORT picks which branch of dev.mjs runs, so never inherit it. + const childEnv = { ...process.env } + delete childEnv.PORT + Object.assign(childEnv, env) if (stripPhp) { // Keep node reachable, drop everything that could provide php. childEnv.PATH = path.dirname(process.execPath) @@ -65,8 +105,8 @@ function run(script, { env = {}, stripPhp = false } = {}) { } describe('dev guards', () => { - it('refuses to start when the frontend port is taken', async () => { - const backend = await occupy() + it('refuses to start when PORT names a port that is taken', async () => { + const backend = await stubBackend() const frontend = await occupy() writeEnv([ `BASE_URL=http://127.0.0.1:${backend.port}`, @@ -82,6 +122,65 @@ describe('dev guards', () => { assert.match(output, /already in use/) // The consequence is the point: a random port breaks the OAuth callback. assert.match(output, /invalid_client/) + // A named port is the user's decision, so the way out is theirs too. + assert.match(output, /drop PORT/) + }) + + it('moves to the next registered port when the default one is taken', async () => { + // The whole 3000-3009 range has a registered OAuth callback, so a + // busy 3000 is no reason to refuse to start. This holds the real + // port 3000: a machine already using it satisfies the case anyway. + const backend = await stubBackend() + const held = await occupyPort(FRONTEND_PORTS[0]) + writeEnv([`BASE_URL=http://127.0.0.1:${backend.port}`, 'OAUTH_CLIENT_ID=test-client']) + + const { output } = await run('dev.mjs') + if (held) held.close() + backend.server.close() + + assert.match(output, /Port 3000 is in use - starting on 300[1-9] instead/) + // It carried on, and starts Nuxt on the port it announced. + assert.match(output, /Starting the Nuxt dev server -> http:\/\/localhost:300[1-9]/) + }) + + it('says so when every registered port is taken', async () => { + const backend = await stubBackend() + const held = await Promise.all(FRONTEND_PORTS.map((port) => occupyPort(port))) + writeEnv([`BASE_URL=http://127.0.0.1:${backend.port}`, 'OAUTH_CLIENT_ID=test-client']) + + const { code, output } = await run('dev.mjs') + for (const server of held) if (server) server.close() + backend.server.close() + + assert.equal(code, 1) + assert.match(output, /Ports 3000-3009 are all in use/) + assert.match(output, /invalid_client/) + }) + + it('refuses a PORT that nothing has registered a callback for', async () => { + // 4000 is outside 3000-3009 and no OAUTH_CALLBACK names it, so the + // browser would send a callback Drupal has never heard of. + const backend = await occupy() + writeEnv([`BASE_URL=http://127.0.0.1:${backend.port}`, 'OAUTH_CLIENT_ID=test-client']) + + const { code, output } = await run('dev.mjs', { env: { PORT: '4000' } }) + backend.server.close() + + assert.equal(code, 1) + assert.match(output, /no OAuth callback registered/) + assert.match(output, /invalid_client/) + }) + + it('leaves an external backend to police its own callbacks', async () => { + // Nothing here provisioned that consumer, so what it accepts is not + // this checkout's to assert. `.invalid` never resolves, so the run + // ends at the OAuth check without reaching the network. + writeEnv(['BASE_URL=http://druxt-nowhere.invalid', 'OAUTH_CLIENT_ID=test-client']) + + const { output } = await run('dev.mjs', { env: { PORT: '4000' } }) + + assert.match(output, /Backend \(external\)/) + assert.doesNotMatch(output, /no OAuth callback registered/) }) it('refuses to start when the callback names another port', async () => { diff --git a/test/lib.test.mjs b/test/lib.test.mjs index afbfc2f..6c9adf7 100644 --- a/test/lib.test.mjs +++ b/test/lib.test.mjs @@ -16,16 +16,20 @@ import { after, before, describe, it } from 'node:test' import { acquireSetupLock, backendInfo, + backendIsProvisionedHere, ddevProjectHost, DRUPAL_DIR, ensureOauthClientId, + firstFreePort, foreground, foregroundNpm, + FRONTEND_PORTS, isPortOpen, MINIMUM_PHP, miseAvailable, phpBelowMinimum, phpVersion, + portIsRegistered, printCommands, readEnv, releaseSetupLock, @@ -89,6 +93,26 @@ describe('backendInfo', () => { }) }) +describe('backendIsProvisionedHere', () => { + it('claims the backends this repo sets up', () => { + const ours = [ + 'http://127.0.0.1:8888', + 'https://quickstart-druxtsite.ddev.site', + 'https://druxt-quickstart.lndo.site', + ] + for (const url of ours) { + assert.equal(backendIsProvisionedHere(backendInfo({ BASE_URL: url })), true, url) + } + }) + + it('disclaims a backend set up somewhere else', () => { + // Its consumer was registered out of sight, so what callbacks it + // accepts is not this checkout's to assert. + const backend = backendInfo({ BASE_URL: 'https://demo-api.druxtjs.org' }) + assert.equal(backendIsProvisionedHere(backend), false) + }) +}) + describe('readEnv', () => { let dir let cwd @@ -154,6 +178,76 @@ describe('the setup lock', () => { }) }) +describe('the registered frontend ports', () => { + it('cover exactly the ports provisioning registers a callback for', () => { + // Drupal is the authority here: a port with no registered callback + // fails login with invalid_client, so the two lists have to agree. + const provision = fs.readFileSync(path.join(DRUPAL_DIR, '.devtools', 'provision'), 'utf8') + const [, first, last] = provision.match(/range\((\d+),\s*(\d+)\)/) + const expected = [] + for (let port = Number(first); port <= Number(last); port += 1) { + expected.push(port) + } + assert.deepEqual(FRONTEND_PORTS, expected) + }) + + it('are the only ports portIsRegistered accepts', () => { + for (const port of FRONTEND_PORTS) { + assert.equal(portIsRegistered(port), true, String(port)) + } + for (const port of [0, 80, 2999, 3010, 8080]) { + assert.equal(portIsRegistered(port), false, String(port)) + } + }) + + it('does not accept a port that is not a number', () => { + for (const port of ['3000', null, undefined, NaN]) { + assert.equal(portIsRegistered(port), false, String(port)) + } + }) +}) + +describe('firstFreePort', () => { + /** Listen on an ephemeral port, and report which one. */ + async function listen() { + const server = net.createServer() + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)) + return { server, port: server.address().port } + } + + /** A port nothing is listening on: take one, then give it back. */ + async function freePort() { + const { server, port } = await listen() + await new Promise((resolve) => server.close(resolve)) + return port + } + + it('skips a port in use and returns the next free one', async () => { + const busy = await listen() + const free = await freePort() + try { + assert.equal(await firstFreePort('127.0.0.1', [busy.port, free]), free) + } finally { + busy.server.close() + } + }) + + it('takes the first free port, not just any free one', async () => { + const first = await freePort() + const second = await freePort() + assert.equal(await firstFreePort('127.0.0.1', [first, second]), first) + }) + + it('is null when every port is taken', async () => { + const busy = await listen() + try { + assert.equal(await firstFreePort('127.0.0.1', [busy.port]), null) + } finally { + busy.server.close() + } + }) +}) + describe('isPortOpen', () => { it('reports a closed port as closed', async () => { // 1 is privileged and never listening in CI.