fix(cli): stop a config value forging a section it was never meant to write

The config file is written by joining names and values into INI lines, and
nothing checked what was in them. A profile name carrying a newline and a
section header wrote a section that merged into a different profile and took
over its endpoint - and the next command sent that profile's stored API key
there. A workspace value could do the same from the other side, since only the
endpoint flag validated its input.

The refusal now lives at the writer, the single place untrusted text enters the
document, with the flag-level checks kept for the better message. Either alone
blocks the forgery; the pair is deliberate.

Rejecting rather than escaping, because the format has no escape syntax and
these files are hand-edited and read by other tools that would not decode one
we invented. The forbidden set covers control characters and the two Unicode
line separators, which the previous guard missed - those parse as an unreadable
line, so the key silently vanished on read and the next write appended a
duplicate while the command reported success.

Login also wrote the key before the settings, so a malformed response from the
deployment could leave a key on disk with no endpoint beside it, and the next
command would send it to the default host. Settings are written first, and the
response is checked before anything touches disk.

Name validation applies only when creating a profile, so a hand-written one
that predates the rule keeps working.

Author: waleedlatif1

packages/sim-cli/src/commands/auth.test.ts

@@ -41,7 +41,17 @@ vi.mock('../auth/device-flow', () => ({
   createAuthRequest: mocks.createAuthRequest,
   pollForKey: mocks.pollForKey,
 }))
-vi.mock('../config/index', () => ({
+/**
+ * The two validators come from the real module rather than a copy: a duplicated
+ * pattern here would keep passing if the shipped one were deleted, which is
+ * exactly the regression these tests exist to catch. `../config/profile` is not
+ * itself mocked, so this is the shipped implementation.
+ */
+vi.mock('../config/index', async () => ({
+  ...(await import('../config/profile').then(({ normalizeWorkspaceId, validateProfileName }) => ({
+    normalizeWorkspaceId,
+    validateProfileName,
+  }))),
   configPath: () => '/tmp/sim-config',
   credentialsPath: () => '/tmp/sim-credentials',
   DEFAULT_PROFILE: 'default',
@@ -232,6 +242,53 @@ describe('login command', () => {
     expect(mocks.createAuthRequest).toHaveBeenCalledOnce()
   })
 
+  it('writes the endpoint before the key, so a failed write cannot strand one', async () => {
+    // A key on disk with no endpoint beside it falls back to the default host
+    // on the next command, which would send a self-hosted key elsewhere.
+    setInteractive(false)
+    const order: string[] = []
+    mocks.writeConfigProfile.mockImplementation(() => {
+      order.push('config')
+    })
+    mocks.writeCredentialsProfile.mockImplementation(() => {
+      order.push('credentials')
+    })
+
+    await login()
+
+    expect(order).toEqual(['config', 'credentials'])
+  })
+
+  it('stores nothing when the server answers with an unstorable workspace id', async () => {
+    setInteractive(false)
+    mocks.pollForKey.mockResolvedValue({
+      apiKey: 'sim-key',
+      scope: 'platform',
+      workspaceBound: false,
+      workspaceId: 'ws_1\nendpoint = http://elsewhere.invalid',
+    })
+
+    await expect(login()).rejects.toThrow('Invalid workspace id')
+
+    expect(mocks.writeConfigProfile).not.toHaveBeenCalled()
+    expect(mocks.writeCredentialsProfile).not.toHaveBeenCalled()
+  })
+
+  it('stores nothing when the server answers with a malformed key', async () => {
+    setInteractive(false)
+    mocks.pollForKey.mockResolvedValue({
+      apiKey: '  ',
+      scope: 'platform',
+      workspaceBound: false,
+      workspaceId: 'ws_1',
+    })
+
+    await expect(login()).rejects.toThrow('malformed API key')
+
+    expect(mocks.writeConfigProfile).not.toHaveBeenCalled()
+    expect(mocks.writeCredentialsProfile).not.toHaveBeenCalled()
+  })
+
   it('clears a stale workspace default when none is selected during login', async () => {
     setInteractive(false)
     mocks.profileFrom.mockReturnValue({
@@ -449,6 +506,13 @@ describe('profiles command', () => {
     expect(mocks.writeConfigProfile).not.toHaveBeenCalled()
   })
 
+  it('refuses a new profile name that would forge a config section', async () => {
+    await expect(profiles('add', 'evil]\n[default', '--workspace', 'ws_acme')).rejects.toThrow(
+      'Invalid profile name'
+    )
+    expect(mocks.writeConfigProfile).not.toHaveBeenCalled()
+  })
+
   it('does not overwrite an existing profile', async () => {
     mocks.listProfiles.mockReturnValue(['acme'])
 
@@ -467,8 +531,65 @@ describe('profiles command', () => {
     await profiles('list')
 
     const output = vi.mocked(console.log).mock.calls.flat().join('\n')
-    expect(output).toContain('acme  (auth: default)')
-    expect(output).not.toContain('acme  (no key)')
+    expect(output).toMatch(/acme\s+yes\s+default/)
+  })
+
+  it('refuses an unknown profile like every other command', async () => {
+    // `profiles list --profile typo` used to exit 0 on a name that resolves to
+    // nothing, alone among the commands, because it never resolved at all.
+    mocks.listProfiles.mockReturnValue(['default'])
+    mocks.profileFrom.mockImplementation(() => {
+      throw new mocks.ProfileConfigError('Unknown profile "bogus".')
+    })
+
+    await expect(profiles('list', '--profile', 'bogus')).rejects.toThrow('Unknown profile "bogus".')
+    expect(console.log).not.toHaveBeenCalled()
+  })
+
+  it('renders the listing in the resolved output format', async () => {
+    mocks.listProfiles.mockReturnValue(['acme', 'default'])
+    mocks.profileFrom.mockReturnValue({
+      name: 'default',
+      endpoint: 'https://sim.ai',
+      apiKey: 'stored-key',
+      workspaceId: 'ws_default',
+      output: 'json',
+      sources: {
+        endpoint: 'config',
+        apiKey: 'credentials',
+        workspaceId: 'config',
+        output: 'config',
+      },
+    })
+
+    await profiles('list')
+
+    const output = vi.mocked(console.log).mock.calls.flat().join('\n')
+    expect(JSON.parse(output)).toEqual([
+      { name: 'acme', active: false, hasKey: true, authProfile: 'acme', error: null },
+      { name: 'default', active: true, hasKey: true, authProfile: 'default', error: null },
+    ])
+  })
+
+  it('answers a machine format with an empty list rather than prose', async () => {
+    mocks.listProfiles.mockReturnValue([])
+    mocks.profileFrom.mockReturnValue({
+      name: 'default',
+      endpoint: 'https://sim.ai',
+      apiKey: null,
+      workspaceId: null,
+      output: 'json',
+      sources: {
+        endpoint: 'default',
+        apiKey: 'unset',
+        workspaceId: 'unset',
+        output: 'config',
+      },
+    })
+
+    await profiles('list')
+
+    expect(JSON.parse(vi.mocked(console.log).mock.calls.flat().join('\n'))).toEqual([])
   })
 
   it('marks a broken profile and still lists the rest', async () => {

packages/sim-cli/src/commands/auth.ts

@@ -15,11 +15,13 @@ import {
   deleteProfile,
   listAuthenticationDependents,
   listProfiles,
+  normalizeWorkspaceId,
   ProfileConfigError,
   type ResolvedProfile,
   readCredentialsProfile,
   resolveAuthenticationProfileName,
   type SettingSource,
+  validateProfileName,
   writeConfigProfile,
   writeCredentialsProfile,
 } from '../config/index'
@@ -31,11 +33,10 @@ import {
   V2_OPERATIONS,
 } from '../generated/v2-api'
 import { requestAllPages, resolvePath, SimApiError, type SimClient } from '../http/client'
-import { printRecord, safeOneLine } from '../output/render'
+import { type Column, printList, printRecord, safeOneLine, text } from '../output/render'
 
 type SelectableWorkspace = ListWorkspacesResponse['data'][number]
 
-const PROFILE_NAME_PATTERN = /^[A-Za-z0-9][A-Za-z0-9._-]*$/
 const MAX_INTERACTIVE_WORKSPACES = 1000
 
 /**
@@ -108,15 +109,29 @@ function selectedProfileName(command: Command): string {
 }
 
 function validateNewProfileName(profileName: string): void {
-  if (!PROFILE_NAME_PATTERN.test(profileName)) {
+  // The shape rule lives with the config writer, so `profiles add`, `login
+  // --profile`, and `configure --profile` cannot drift into three answers.
+  validateProfileName(profileName)
+  if (listProfiles().includes(profileName)) {
     throw new SimApiError(
-      `Invalid profile name "${profileName}". Use letters, numbers, dots, underscores, or hyphens, starting with a letter or number.`,
+      `Profile "${profileName}" already exists. Remove it first with: sim logout --all --profile ${profileName}`,
       0
     )
   }
-  if (listProfiles().includes(profileName)) {
+}
+
+/**
+ * Refuses a minted key the credentials file could not represent.
+ *
+ * The poll response is remote input, and the deployment answering it is
+ * whatever the endpoint names. A key carrying a line break would be written
+ * verbatim into an escape-less format, so the writer refuses it — this refuses
+ * it one step earlier, before anything is on disk, and says which side is wrong.
+ */
+function requireStorableKey(apiKey: unknown): void {
+  if (typeof apiKey !== 'string' || !apiKey.trim() || /[\u0000-\u001f\u007f-\u009f]/.test(apiKey)) {
     throw new SimApiError(
-      `Profile "${profileName}" already exists. Remove it first with: sim logout --all --profile ${profileName}`,
+      'The server returned a malformed API key. Nothing was stored; check the endpoint.',
       0
     )
   }
@@ -290,17 +305,25 @@ export function loginCommand(): Command {
           )
         }
 
-        writeCredentialsProfile(profile.name, key.apiKey)
-
         // The workspace picked in the browser becomes the profile's default,
         // whether or not the key is scoped to it. The user chose it by name —
         // making them look up its id afterwards would waste the one moment the
-        // answer was already on screen.
+        // answer was already on screen. It arrives off the wire, so it is
+        // checked before either file is touched.
         const settings: Record<string, string | null> = {
           endpoint: profile.endpoint,
-          workspace: key.workspaceId ?? null,
+          workspace: key.workspaceId
+            ? normalizeWorkspaceId(key.workspaceId, 'the login response')
+            : null,
         }
+        requireStorableKey(key.apiKey)
+
+        // Config before credentials: the endpoint decides where the key is sent
+        // later. Storing the key first and then failing on the settings left a
+        // key on disk with no endpoint beside it, so the next command fell back
+        // to the default host — sending a self-hosted key somewhere else.
         writeConfigProfile(profile.name, settings)
+        writeCredentialsProfile(profile.name, key.apiKey)
 
         console.log(chalk.green(`\n✓ Logged in. Key stored in ${credentialsPath()}`))
         if (key.workspaceBound && key.workspaceId) {
@@ -597,38 +620,66 @@ export function whoamiCommand(): Command {
     })
 }
 
+interface ProfileRow {
+  name: string
+  active: boolean
+  hasKey: boolean
+  /** The profile whose stored login this one uses; itself unless it is an alias. */
+  authProfile: string | null
+  /** Why the row could not be resolved, for a profile with a broken `auth_profile`. */
+  error: string | null
+}
+
+const PROFILE_COLUMNS: Column<ProfileRow>[] = [
+  { header: '', value: (row) => (row.active ? chalk.green('*') : ' ') },
+  { header: 'profile', value: (row) => text(row.name) },
+  { header: 'key', value: (row) => (row.error ? text(null) : row.hasKey ? 'yes' : 'no') },
+  { header: 'auth', value: (row) => text(row.authProfile) },
+  { header: 'error', value: (row) => (row.error ? chalk.red(safeOneLine(row.error)) : text(null)) },
+]
+
+/**
+ * `profiles` is the command someone runs *because* a profile is broken, so a bad
+ * `auth_profile` marks its own row rather than aborting the listing and leaving
+ * them with nothing shown at all.
+ */
+function buildProfileRow(name: string, active: boolean): ProfileRow {
+  try {
+    const authProfile = resolveAuthenticationProfileName(name)
+    return {
+      name,
+      active,
+      hasKey: Boolean(readCredentialsProfile(authProfile).api_key),
+      authProfile,
+      error: null,
+    }
+  } catch (error) {
+    if (!(error instanceof ProfileConfigError)) throw error
+    return { name, active, hasKey: false, authProfile: null, error: error.message }
+  }
+}
+
 export function profilesCommand(): Command {
   const command = new Command('profiles')
     .alias('profile')
     .description('List profiles or add a workspace profile that shares a stored login')
 
   const printProfiles = (_options: unknown, actionCommand: Command): void => {
-    const profiles = listProfiles()
-    if (profiles.length === 0) {
-      console.log(chalk.dim('No profiles yet. Run: sim login'))
+    // Resolving is what makes `profiles --profile typo` fail like every other
+    // command instead of listing happily under a name that resolves to nothing,
+    // and it is also what supplies the output format the listing renders in.
+    const profile = profileFrom(actionCommand)
+    const rows = listProfiles().map((name) => buildProfileRow(name, name === profile.name))
+
+    if (rows.length === 0) {
+      // The prose belongs to the human formats; a script asking for json must
+      // get an empty list, not a sentence it cannot parse.
+      if (profile.output === 'table') console.log(chalk.dim('No profiles yet. Run: sim login'))
+      else printList(profile.output, rows, PROFILE_COLUMNS)
       return
     }
 
-    const active = selectedProfileName(actionCommand)
-    for (const name of profiles) {
-      const marker = name === active ? chalk.green('*') : ' '
-
-      // `profiles` is the command someone runs *because* a profile is broken,
-      // so one bad `auth_profile` must mark its own row rather than abort the
-      // listing and leave them with no profiles shown at all.
-      let authProfile: string
-      try {
-        authProfile = resolveAuthenticationProfileName(name)
-      } catch (error) {
-        if (!(error instanceof ProfileConfigError)) throw error
-        console.log(`${marker} ${name}${chalk.red(`  (${safeOneLine(error.message)})`)}`)
-        continue
-      }
-
-      const hasKey = Boolean(readCredentialsProfile(authProfile).api_key)
-      const authentication = authProfile === name ? '' : chalk.dim(`  (auth: ${authProfile})`)
-      console.log(`${marker} ${name}${hasKey ? '' : chalk.dim('  (no key)')}${authentication}`)
-    }
+    printList(profile.output, rows, PROFILE_COLUMNS)
   }
 
   command.action(printProfiles)