fix: reject an --api-url that is not an http(s) URL, with exit 2

README.md

@@ -275,8 +275,8 @@ A timed-out `dataset ingest` may still have been processed: check `databox datas
 | Exit code | Meaning |
 |-----------|---------|
 | `0` | Success. Answering anything but `y` or `yes` to a confirmation prompt, at a terminal or piped, also exits 0 after printing `Aborted.`. |
-| `1` | The API returned an error (4xx or 5xx, including the rate limit, and `forbidden` when your role does not allow the command; see [Permissions](#permissions)). Also: no API key is configured, the stored config file is not valid JSON, the response was not JSON (usually a wrong `--api-url`), an update command was given no field to change, or `dataset ingest` was run at a terminal with no `--records` or `--file`. |
-| `2` | The request was never sent, or never reached the API: an unknown flag, a value outside a flag's options, a malformed ID or JSON value, an ingest over the limits, or a network failure, timeout or redirect (the CLI does not follow a redirect: it would resend your API key). Also a command that would prompt when stdin is not a terminal and nothing is piped: a delete, purge or clear without `--force`, or `auth login` without `--api-key` (see [Authentication](#authentication)). |
+| `1` | The API returned an error (4xx or 5xx, including the rate limit, and `forbidden` when your role does not allow the command; see [Permissions](#permissions)). Also: no API key is configured, the stored config file is not valid JSON or holds an API URL that is not `http://` or `https://`, the response was not JSON (usually a wrong `--api-url`), an update command was given no field to change, or `dataset ingest` was run at a terminal with no `--records` or `--file`. |
+| `2` | The request was never sent, or never reached the API: an unknown flag, a value outside a flag's options, a malformed ID, `--api-url` or JSON value, an ingest over the limits, or a network failure, timeout or redirect (the CLI does not follow a redirect: it would resend your API key). Also a command that would prompt when stdin is not a terminal and nothing is piped: a delete, purge or clear without `--force`, or `auth login` without `--api-key` (see [Authentication](#authentication)). |
 | `130` | A prompt (a confirmation, or the API key at `auth login`) was interrupted with Ctrl-C. |
 
 ## Scripts and AI Agents

src/base-command.ts

@@ -3,9 +3,9 @@ import {
 } from '@oclif/core'
 
 import {
-  ApiClient, ApiConnectionError, ApiRequestError, describeApiError, describeConnectionError,
+  ApiClient, ApiConnectionError, ApiRequestError, describeApiError, describeConnectionError, describeInvalidApiUrl, isApiUrl,
 } from './lib/api-client.js'
-import {loadConfig} from './lib/config.js'
+import {getConfigPath, loadConfig} from './lib/config.js'
 import {OUTPUT_FORMATS, OutputFormat, colorEnabled} from './lib/output.js'
 
 export type Flags<T extends typeof Command> = Interfaces.InferredFlags<T['flags'] & typeof BaseCommand['baseFlags']>
@@ -100,6 +100,12 @@ export abstract class BaseCommand<T extends typeof Command = typeof Command> ext
     const accountId = this.flags['account-id']
     if (accountId) this.requireNumericId(accountId, '--account-id')
 
+    // A base URL fetch cannot use would otherwise fail only once the request is built: see isApiUrl().
+    const apiUrl = this.flags['api-url']
+    if (apiUrl !== undefined && !isApiUrl(apiUrl)) {
+      this.error(describeInvalidApiUrl(apiUrl), {exit: 2})
+    }
+
     // Every command that extends BaseCommand calls the API, so a missing key or an unreadable
     // config fails here too, not after the prompt. auth login, which runs without a key,
     // extends Command.
@@ -109,9 +115,19 @@ export abstract class BaseCommand<T extends typeof Command = typeof Command> ext
       this.error('Not authenticated. Run "databox auth login" first.', {exit: 1})
     }
 
+    // A URL saved before `auth login` checked it, or edited in by hand. Like an unreadable
+    // config, it is the stored config at fault, so exit 1; the --api-url above overrides it.
+    if (apiUrl === undefined && config.apiUrl !== undefined && !isApiUrl(config.apiUrl)) {
+      this.error(
+        `The API URL saved in ${getConfigPath()} ("${config.apiUrl}") is not an http:// or https:// URL. `
+        + 'Run "databox auth login --api-url https://api.databox.com" to replace it.',
+        {exit: 1},
+      )
+    }
+
     this.apiClient = new ApiClient({
       apiKey,
-      baseUrl: this.flags['api-url'] ?? config.apiUrl,
+      baseUrl: apiUrl ?? config.apiUrl,
       trace: this.flags.verbose ? line => this.logToStderr(line) : undefined,
       version: this.config.version,
     })

src/commands/auth/login.ts

@@ -1,6 +1,6 @@
 import {Command, Flags} from '@oclif/core'
 
-import {ApiClient} from '../../lib/api-client.js'
+import {ApiClient, describeInvalidApiUrl, isApiUrl} from '../../lib/api-client.js'
 import {loadConfig, saveConfig} from '../../lib/config.js'
 import {prompt, readPipedLine} from '../../lib/prompt.js'
 
@@ -30,6 +30,12 @@ export default class Login extends Command {
     let apiKey = flags['api-key']
     const apiUrl = flags['api-url']
 
+    // Checked before the prompt and before anything is saved: a stored URL fetch cannot use
+    // would fail every later command, while this one still exited 0 with only a warning.
+    if (apiUrl !== undefined && !isApiUrl(apiUrl)) {
+      this.error(describeInvalidApiUrl(apiUrl), {exit: 2})
+    }
+
     // Off a terminal there is nobody to prompt, but the key can be piped in
     // (`pass show databox | databox auth login`). An empty first line or no input is refused.
     if (!apiKey && !process.stdin.isTTY) {

src/lib/api-client.ts

@@ -112,6 +112,26 @@ export function describeResponse(status: number, durationMs: number, requestId?:
   return lines
 }
 
+/**
+ * Whether a value can be the API base URL: absolute, over http or https. Anything else fails only
+ * once a request is built: `api.databox.com` as a bare "TypeError: Invalid URL", and
+ * `localhost:8080`, which parses with the scheme "localhost:", as a connection error that blames
+ * the network for a request that never left.
+ */
+export function isApiUrl(value: string): boolean {
+  try {
+    const {protocol} = new URL(value)
+    return protocol === 'https:' || protocol === 'http:'
+  } catch {
+    return false
+  }
+}
+
+/** The exit-2 message for an --api-url (or DATABOX_API_URL) that isApiUrl() refuses. */
+export function describeInvalidApiUrl(value: string): string {
+  return `--api-url must be an http:// or https:// URL, e.g. ${DEFAULT_BASE_URL}. Got "${value}" (from --api-url or DATABOX_API_URL).`
+}
+
 /** Whether fetch() rejected because of `redirect: 'error'`: Node reports it as a TypeError caused by "unexpected redirect". */
 export function isRedirectRefusal(error: unknown): boolean {
   return error instanceof TypeError && error.cause instanceof Error && error.cause.message === 'unexpected redirect'

test/base-command.test.ts

@@ -396,3 +396,99 @@ describe('base command: credentials', () => {
     expect(requests()).to.have.length(0)
   })
 })
+
+describe('base command: --api-url', () => {
+  const savedApiUrl = process.env.DATABOX_API_URL
+
+  beforeEach(() => {
+    setupTestConfig(KEY)
+    mockApi([{
+      method: 'GET',
+      path: '/v2/data-sources/10',
+      response: {data: DATA_SOURCE, requestId: 'test', status: 'success'},
+    }])
+  })
+
+  afterEach(() => {
+    if (savedApiUrl === undefined) delete process.env.DATABOX_API_URL
+    else process.env.DATABOX_API_URL = savedApiUrl
+    restoreApi()
+    cleanupTestConfig()
+  })
+
+  it('sends the request to the URL given', async () => {
+    const {error} = await runCommand(['data-source', 'get', '10', '--api-url', 'http://localhost:8080'], {root: process.cwd()})
+
+    expect(error).to.equal(undefined)
+    expect(requests()).to.have.length(1)
+  })
+
+  // Was a bare "TypeError: Invalid URL", exit 1, from inside the request.
+  it('rejects a URL without a scheme with exit 2, before any request', async () => {
+    const {error} = await runCommand(['data-source', 'get', '10', '--api-url', 'api.databox.com'], {root: process.cwd()})
+
+    expect(error?.oclif?.exit).to.equal(2)
+    expect(error?.message).to.contain('--api-url must be an http:// or https:// URL')
+    expect(error?.message).to.contain('Got "api.databox.com"')
+    expect(requests()).to.have.length(0)
+  })
+
+  // Was "Could not connect to API. Check your internet connection." with a request ID that never left.
+  it('rejects a host:port without a scheme, which parses as a URL fetch cannot use', async () => {
+    const {error} = await runCommand(['data-source', 'get', '10', '--api-url', 'localhost:8080'], {root: process.cwd()})
+
+    expect(error?.oclif?.exit).to.equal(2)
+    expect(error?.message).to.contain('Got "localhost:8080"')
+    expect(error?.message).to.not.contain('Request ID')
+    expect(requests()).to.have.length(0)
+  })
+
+  it('checks DATABOX_API_URL the same way', async () => {
+    process.env.DATABOX_API_URL = 'api.databox.com'
+
+    const {error} = await runCommand(['data-source', 'get', '10'], {root: process.cwd()})
+
+    expect(error?.oclif?.exit).to.equal(2)
+    expect(error?.message).to.contain('DATABOX_API_URL')
+    expect(requests()).to.have.length(0)
+  })
+
+  // No --force: the check has to fail before confirm() reads stdin.
+  it('rejects a bad URL before a destructive command prompts', async () => {
+    const {error} = await runCommand(['dataset', 'delete', '1', '--api-url', 'api.databox.com'], {root: process.cwd()})
+
+    expect(error?.oclif?.exit).to.equal(2)
+    expect(error?.message).to.contain('--api-url must be an http:// or https:// URL')
+    expect(requests()).to.have.length(0)
+  })
+
+  it('reports a bad stored URL by path, with the command that replaces it', async () => {
+    fs.writeFileSync(getConfigPath(), JSON.stringify({apiKey: KEY, apiUrl: 'api.databox.com'}))
+
+    const {error, stderr, stdout} = await runCommand(['data-source', 'get', '10'], {root: process.cwd()})
+
+    expect(error?.oclif?.exit).to.equal(1)
+    expect(error?.message).to.contain(`The API URL saved in ${getConfigPath()} ("api.databox.com")`)
+    expect(error?.message).to.contain('databox auth login --api-url https://api.databox.com')
+    expect(`${error?.message}${stderr}${stdout}`).to.not.contain(KEY)
+    expect(requests()).to.have.length(0)
+  })
+
+  it('lets --api-url override a bad stored URL', async () => {
+    fs.writeFileSync(getConfigPath(), JSON.stringify({apiKey: KEY, apiUrl: 'api.databox.com'}))
+
+    const {error} = await runCommand(['data-source', 'get', '10', '--api-url', 'https://api.databox.com'], {root: process.cwd()})
+
+    expect(error).to.equal(undefined)
+    expect(requests()).to.have.length(1)
+  })
+
+  it('still uses a good stored URL', async () => {
+    fs.writeFileSync(getConfigPath(), JSON.stringify({apiKey: KEY, apiUrl: 'https://staging.example.com'}))
+
+    const {error} = await runCommand(['data-source', 'get', '10'], {root: process.cwd()})
+
+    expect(error).to.equal(undefined)
+    expect(requests()).to.have.length(1)
+  })
+})

test/commands/auth/login.test.ts

@@ -52,6 +52,31 @@ describe('auth login', () => {
     expect(result.stderr).to.not.contain('Warning: Warning:')
   })
 
+  it('saves the --api-url it was given', async () => {
+    mockApi([{
+      method: 'GET',
+      path: '/v2/auth/validate-key',
+      response: {data: {}, requestId: 'test', status: 'success'},
+    }])
+
+    const {error} = await runCommand(['auth', 'login', '--api-key', 'my-test-key', '--api-url', 'http://localhost:8080'], {root: process.cwd()})
+
+    expect(error).to.equal(undefined)
+    expect(JSON.parse(fs.readFileSync(getConfigPath(), 'utf8')).apiUrl).to.equal('http://localhost:8080')
+  })
+
+  // Was saved, with only a warning and exit 0, and then failed every later command.
+  it('refuses an --api-url without a scheme with exit 2, and saves nothing', async () => {
+    mockApi([])
+
+    const {error} = await runCommand(['auth', 'login', '--api-key', 'my-test-key', '--api-url', 'api.databox.com'], {root: process.cwd()})
+
+    expect(error?.oclif?.exit).to.equal(2)
+    expect(error?.message).to.contain('--api-url must be an http:// or https:// URL')
+    expect(requests()).to.have.length(0)
+    expect(fs.existsSync(getConfigPath())).to.equal(false)
+  })
+
   describe('when stdin is not a terminal', () => {
     let restore: () => void
     const savedApiKey = process.env.DATABOX_API_KEY
@@ -107,6 +132,17 @@ describe('auth login', () => {
       expect(fs.existsSync(getConfigPath())).to.equal(false)
     })
 
+    it('refuses a bad --api-url before reading the piped key', async () => {
+      restore = pipeStdin('piped-test-key\n')
+      mockApi([])
+
+      const {error} = await runCommand(['auth', 'login', '--api-url', 'localhost:8080'], {root: process.cwd()})
+
+      expect(error?.oclif?.exit).to.equal(2)
+      expect(requests()).to.have.length(0)
+      expect(fs.existsSync(getConfigPath())).to.equal(false)
+    })
+
     it('uses --api-key without reading stdin', async () => {
       restore = pipeStdin('piped-test-key\n')
       mockApi([{

test/lib/api-client.test.ts

@@ -1,7 +1,7 @@
 import {expect} from 'chai'
 
 import {
-  ApiClient, ApiConnectionError, ApiRequestError, describeApiError, describeConnectionError,
+  ApiClient, ApiConnectionError, ApiRequestError, describeApiError, describeConnectionError, isApiUrl,
 } from '../../src/lib/api-client.js'
 import {mockApi, requests, restoreApi} from '../helpers.js'
 
@@ -310,6 +310,30 @@ describe('ApiClient response bodies', () => {
   })
 })
 
+describe('isApiUrl', () => {
+  it('accepts an absolute http or https URL', () => {
+    expect(isApiUrl('https://api.databox.com')).to.equal(true)
+    expect(isApiUrl('https://api.databox.com/')).to.equal(true)
+    expect(isApiUrl('http://localhost:8080')).to.equal(true)
+  })
+
+  it('refuses a host without a scheme, which new URL() cannot parse', () => {
+    expect(isApiUrl('api.databox.com')).to.equal(false)
+    expect(isApiUrl('')).to.equal(false)
+  })
+
+  // These parse, with the schemes "localhost:" and "api.databox.com:", so only fetch() would fail.
+  it('refuses a host:port without a scheme, which parses as some other scheme', () => {
+    expect(isApiUrl('localhost:8080')).to.equal(false)
+    expect(isApiUrl('api.databox.com:443')).to.equal(false)
+  })
+
+  it('refuses a scheme other than http and https', () => {
+    expect(isApiUrl('ftp://api.databox.com')).to.equal(false)
+    expect(isApiUrl('file:///etc/hosts')).to.equal(false)
+  })
+})
+
 describe('describeApiError', () => {
   it('renders code, message, field and request ID', () => {
     const error = new ApiRequestError('Title is required.', 400, [