Skip to content

Commit 7e83144

Browse files
committed
fix(cli): stop a refusal being swallowed, and gate example ids by name
A blank root flag was refused everywhere except `profiles`, where the catch that lets a broken profile still list absorbed it and the command exited 0 after printing the table. The refusal now carries its own error class, which is what the listing rethrows on — the two are distinguished by type rather than by matching message text, and a genuinely broken profile still lists. The unknown-profile message redacted the name the caller typed but not the suggestion or the list of configured names beside it, which come from the same file and are equally attacker-influenced once it has been hand-edited. Those are redacted now, as is every other message in these two files that quotes a name read out of the config, and the profile listing flattens the names it renders the way it already flattened the error column. The example-id audit judged a uuid by its digit texture, on the premise that a real one essentially never looks hand-authored. Measured against ten million generated ids, 0.81% of them do — one in 124, where this change alone replaced six. Requiring each digit exactly twice takes that to zero but rejects all fourteen placeholders now in the specs, so it is no cheaper than the alternative. The audit now holds the eighteen ids the specs actually use, which is one file rather than the twenty-seven a reserved format would touch, and a new id fails until someone lists it — which is the review the check exists to force.
1 parent f8ea72d commit 7e83144

5 files changed

Lines changed: 172 additions & 68 deletions

File tree

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

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ const mocks = vi.hoisted(() => ({
1010
listProfiles: vi.fn<() => string[]>(() => []),
1111
request: vi.fn(),
1212
readCredentialsProfile: vi.fn<() => Record<string, string>>(() => ({})),
13-
ProfileConfigError: class ProfileConfigError extends Error {},
1413
resolveAuthenticationProfileName: vi.fn((profile: string) => profile),
1514
pollForKey: vi.fn(async () => ({
1615
apiKey: 'sim-key',
@@ -49,10 +48,17 @@ vi.mock('../auth/device-flow', () => ({
4948
*/
5049
vi.mock('../config/index', async () => ({
5150
...(await import('../config/profile').then(
52-
({ FORBIDDEN_IN_VALUE, normalizeWorkspaceId, OUTPUT_FORMATS, validateProfileName }) => ({
51+
({
5352
FORBIDDEN_IN_VALUE,
5453
normalizeWorkspaceId,
5554
OUTPUT_FORMATS,
55+
ProfileConfigError,
56+
validateProfileName,
57+
}) => ({
58+
FORBIDDEN_IN_VALUE,
59+
normalizeWorkspaceId,
60+
OUTPUT_FORMATS,
61+
ProfileConfigError,
5662
validateProfileName,
5763
})
5864
)),
@@ -62,7 +68,6 @@ vi.mock('../config/index', async () => ({
6268
deleteProfile: mocks.deleteProfile,
6369
listAuthenticationDependents: mocks.listAuthenticationDependents,
6470
listProfiles: mocks.listProfiles,
65-
ProfileConfigError: mocks.ProfileConfigError,
6671
readCredentialsProfile: mocks.readCredentialsProfile,
6772
resolveAuthenticationProfileName: mocks.resolveAuthenticationProfileName,
6873
writeConfigProfile: mocks.writeConfigProfile,
@@ -74,6 +79,7 @@ vi.mock('../context', () => ({
7479
clientFrom: () => ({ client: { request: mocks.request }, profile: mocks.profileFrom() }),
7580
}))
7681

82+
import { ProfileConfigError, ProfileOverrideError } from '../config/profile'
7783
import { SimApiError } from '../http/client'
7884
import { loginCommand, logoutCommand, profilesCommand, whoamiCommand } from './auth'
7985

@@ -607,7 +613,7 @@ describe('profiles command', () => {
607613
// nothing, alone among the commands, because it never resolved at all.
608614
mocks.listProfiles.mockReturnValue(['default'])
609615
mocks.profileFrom.mockImplementation(() => {
610-
throw new mocks.ProfileConfigError('Unknown profile "bogus".')
616+
throw new ProfileConfigError('Unknown profile "bogus".')
611617
})
612618

613619
await expect(profiles('list', '--profile', 'bogus')).rejects.toThrow('Unknown profile "bogus".')
@@ -665,13 +671,11 @@ describe('profiles command', () => {
665671
// it throws for exactly the profile this command exists to show.
666672
mocks.listProfiles.mockReturnValue(['broken', 'default'])
667673
mocks.profileFrom.mockImplementation(() => {
668-
throw new mocks.ProfileConfigError('Profile "broken" references missing auth_profile "gone".')
674+
throw new ProfileConfigError('Profile "broken" references missing auth_profile "gone".')
669675
})
670676
mocks.resolveAuthenticationProfileName.mockImplementation((profile) => {
671677
if (profile === 'broken') {
672-
throw new mocks.ProfileConfigError(
673-
'Profile "broken" references missing auth_profile "gone".'
674-
)
678+
throw new ProfileConfigError('Profile "broken" references missing auth_profile "gone".')
675679
}
676680
return profile
677681
})
@@ -689,7 +693,7 @@ describe('profiles command', () => {
689693
// profiles` must fail exactly like `sim profiles --profile typo`.
690694
mocks.listProfiles.mockReturnValue(['default'])
691695
mocks.profileFrom.mockImplementation(() => {
692-
throw new mocks.ProfileConfigError('Unknown profile "bogus".')
696+
throw new ProfileConfigError('Unknown profile "bogus".')
693697
})
694698
process.env.SIM_PROFILE = 'bogus'
695699

@@ -706,7 +710,7 @@ describe('profiles command', () => {
706710
// exit 0. The catch exists to tolerate a broken *profile*, not a bad flag.
707711
mocks.listProfiles.mockReturnValue(['default'])
708712
mocks.profileFrom.mockImplementation(() => {
709-
throw new mocks.ProfileConfigError('Unknown output format "jsonl" from env.')
713+
throw new ProfileConfigError('Unknown output format "jsonl" from env.')
710714
})
711715
process.env.SIM_OUTPUT = 'jsonl'
712716

@@ -718,15 +722,28 @@ describe('profiles command', () => {
718722
}
719723
})
720724

725+
it('refuses a blank root flag rather than absorbing it into the broken-profile fallback', async () => {
726+
// The tolerance below exists for a profile that will not resolve. A blank
727+
// `--workspace` is the caller's own argument, and swallowing it listed
728+
// profiles and exited 0 where every other command exits 1.
729+
mocks.listProfiles.mockReturnValue(['default'])
730+
mocks.profileFrom.mockImplementation(() => {
731+
throw new ProfileOverrideError('--workspace requires a value.')
732+
})
733+
734+
await expect(profiles('list', '--workspace', '')).rejects.toThrow(
735+
'--workspace requires a value.'
736+
)
737+
expect(console.log).not.toHaveBeenCalled()
738+
})
739+
721740
it('marks a broken profile and still lists the rest', async () => {
722741
// `profiles` is the command someone runs *because* a profile is broken, and
723742
// one bad auth_profile used to abort the listing with nothing shown at all.
724743
mocks.listProfiles.mockReturnValue(['broken', 'default', 'dev'])
725744
mocks.resolveAuthenticationProfileName.mockImplementation((profile) => {
726745
if (profile === 'broken') {
727-
throw new mocks.ProfileConfigError(
728-
'Profile "broken" references missing auth_profile "gone".'
729-
)
746+
throw new ProfileConfigError('Profile "broken" references missing auth_profile "gone".')
730747
}
731748
return profile
732749
})

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

Lines changed: 29 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import {
2828
writeConfigProfile,
2929
writeCredentialsProfile,
3030
} from '../config/index'
31+
import { ProfileOverrideError, redact } from '../config/profile'
3132
import { clientFrom, globalsOf, profileFrom } from '../context'
3233
import {
3334
type GetMetaResponse,
@@ -91,15 +92,15 @@ function presentAuthentication(source: SettingSource): {
9192
async function confirmProfileOverwrite(profileName: string): Promise<boolean> {
9293
if (!process.stdin.isTTY) {
9394
throw new SimApiError(
94-
`Profile "${profileName}" already exists. Re-run with --yes to overwrite it.`,
95+
`Profile "${redact(profileName)}" already exists. Re-run with --yes to overwrite it.`,
9596
0
9697
)
9798
}
9899

99100
const prompt = createInterface({ input: process.stdin, output: process.stderr })
100101
try {
101102
const answer = await prompt.question(
102-
`Profile "${profileName}" already exists. Replace its API key and login defaults? (y/N) `
103+
`Profile "${redact(profileName)}" already exists. Replace its API key and login defaults? (y/N) `
103104
)
104105
return answer.trim().toLowerCase() === 'y' || answer.trim().toLowerCase() === 'yes'
105106
} finally {
@@ -117,7 +118,7 @@ function validateNewProfileName(profileName: string): void {
117118
validateProfileName(profileName)
118119
if (listProfiles().includes(profileName)) {
119120
throw new SimApiError(
120-
`Profile "${profileName}" already exists. Remove it first with: sim logout --all --profile ${profileName}`,
121+
`Profile "${redact(profileName)}" already exists. Remove it first with: sim logout --all --profile ${redact(profileName)}`,
121122
0
122123
)
123124
}
@@ -160,13 +161,13 @@ function requireStoredAuthentication(profile: ResolvedProfile): string {
160161
const storedKey = readCredentialsProfile(authProfile).api_key
161162
if (profile.sources.apiKey !== 'credentials' || !storedKey) {
162163
throw new SimApiError(
163-
`Cannot create a shared profile from "${profile.name}": the active API key is not stored. Run: sim login --profile ${authProfile}`,
164+
`Cannot create a shared profile from "${redact(profile.name)}": the active API key is not stored. Run: sim login --profile ${redact(authProfile)}`,
164165
0
165166
)
166167
}
167168
if (profile.sources.endpoint === 'flag' || profile.sources.endpoint === 'env') {
168169
throw new SimApiError(
169-
`Cannot create a shared profile from "${profile.name}": the active endpoint comes from ${profile.sources.endpoint}. Save it with: sim configure --profile ${authProfile} --set-endpoint ${profile.endpoint}`,
170+
`Cannot create a shared profile from "${redact(profile.name)}": the active endpoint comes from ${profile.sources.endpoint}. Save it with: sim configure --profile ${redact(authProfile)} --set-endpoint ${profile.endpoint}`,
170171
0
171172
)
172173
}
@@ -255,10 +256,10 @@ function addProfileCommand(): Command {
255256
workspace: normalizeWorkspaceId(workspace.id, 'the workspace response'),
256257
})
257258

258-
console.log(chalk.green(`✓ Added profile "${profileName}" in ${configPath()}`))
259+
console.log(chalk.green(`✓ Added profile "${safeOneLine(profileName)}" in ${configPath()}`))
259260
console.log(` Workspace: ${safeOneLine(workspace.name)} (${workspace.id})`)
260-
console.log(` Authentication: ${authProfile}`)
261-
console.log(chalk.dim(` Try: sim --profile ${profileName} whoami`))
261+
console.log(` Authentication: ${safeOneLine(authProfile)}`)
262+
console.log(chalk.dim(` Try: sim --profile ${safeOneLine(profileName)} whoami`))
262263
})
263264
}
264265

@@ -277,7 +278,7 @@ export function loginCommand(): Command {
277278

278279
if (authProfile !== profile.name) {
279280
throw new SimApiError(
280-
`Profile "${profile.name}" shares authentication with "${authProfile}". Run: sim login --profile ${authProfile}`,
281+
`Profile "${redact(profile.name)}" shares authentication with "${redact(authProfile)}". Run: sim login --profile ${redact(authProfile)}`,
281282
0
282283
)
283284
}
@@ -304,7 +305,7 @@ export function loginCommand(): Command {
304305
)
305306

306307
console.log(
307-
`Signing in to ${chalk.bold(profile.endpoint)} as profile ${chalk.bold(profile.name)}`
308+
`Signing in to ${chalk.bold(profile.endpoint)} as profile ${chalk.bold(safeOneLine(profile.name))}`
308309
)
309310
console.log(`\nPairing code: ${chalk.bold(auth.pairing)}`)
310311
console.log(
@@ -386,35 +387,37 @@ export function logoutCommand(): Command {
386387
const dependents = listAuthenticationDependents(profileName)
387388
if (dependents.length > 0) {
388389
throw new SimApiError(
389-
`Cannot remove authentication profile "${profileName}" because it is used by: ${dependents.join(', ')}. Remove those profiles first.`,
390+
`Cannot remove authentication profile "${redact(profileName)}" because it is used by: ${dependents.map(redact).join(', ')}. Remove those profiles first.`,
390391
0
391392
)
392393
}
393394
const removed = deleteProfile(profileName)
394395
if (!removed.config && !removed.credentials) {
395-
console.log(chalk.dim(`Nothing stored for profile "${profileName}".`))
396+
console.log(chalk.dim(`Nothing stored for profile "${safeOneLine(profileName)}".`))
396397
return
397398
}
398-
console.log(chalk.green(`✓ Removed profile "${profileName}".`))
399+
console.log(chalk.green(`✓ Removed profile "${safeOneLine(profileName)}".`))
399400
return
400401
}
401402

402403
const profile = profileFrom(command)
403404
const authProfile = resolveAuthenticationProfileName(profile.name)
404405
if (authProfile !== profile.name) {
405406
throw new SimApiError(
406-
`Profile "${profile.name}" shares authentication with "${authProfile}". Log out of the authentication profile instead: sim logout --profile ${authProfile}`,
407+
`Profile "${redact(profile.name)}" shares authentication with "${redact(authProfile)}". Log out of the authentication profile instead: sim logout --profile ${redact(authProfile)}`,
407408
0
408409
)
409410
}
410411

411412
if (!readCredentialsProfile(profile.name).api_key) {
412-
console.log(chalk.dim(`No stored key for profile "${profile.name}".`))
413+
console.log(chalk.dim(`No stored key for profile "${safeOneLine(profile.name)}".`))
413414
return
414415
}
415416

416417
writeCredentialsProfile(profile.name, null)
417-
console.log(chalk.green(`✓ Removed the stored key for profile "${profile.name}".`))
418+
console.log(
419+
chalk.green(`✓ Removed the stored key for profile "${safeOneLine(profile.name)}".`)
420+
)
418421
// The key still exists server-side; leaving that unsaid invites the
419422
// assumption that logging out revoked it.
420423
console.log(chalk.dim(' The key itself is still active — revoke it in Settings → API keys.'))
@@ -522,7 +525,7 @@ async function verifyProfile(
522525
status: 'unauthenticated',
523526
workspace: null,
524527
keyType: null,
525-
detail: `no API key — run: sim login --profile ${profile.name}`,
528+
detail: `no API key — run: sim login --profile ${safeOneLine(profile.name)}`,
526529
}
527530
}
528531

@@ -536,7 +539,7 @@ async function verifyProfile(
536539
status: 'no-workspace',
537540
workspace: null,
538541
keyType,
539-
detail: `no workspace to check against — run: sim configure --profile ${profile.name} --set-workspace <id>`,
542+
detail: `no workspace to check against — run: sim configure --profile ${safeOneLine(profile.name)} --set-workspace <id>`,
540543
}
541544
}
542545

@@ -663,9 +666,9 @@ interface ProfileRow {
663666

664667
const PROFILE_COLUMNS: Column<ProfileRow>[] = [
665668
{ header: '', value: (row) => (row.active ? chalk.green('*') : ' ') },
666-
{ header: 'profile', value: (row) => text(row.name) },
669+
{ header: 'profile', value: (row) => safeOneLine(row.name) },
667670
{ header: 'key', value: (row) => (row.error ? text(null) : row.hasKey ? 'yes' : 'no') },
668-
{ header: 'auth', value: (row) => text(row.authProfile) },
671+
{ header: 'auth', value: (row) => (row.authProfile ? safeOneLine(row.authProfile) : text(null)) },
669672
{ header: 'error', value: (row) => (row.error ? chalk.red(safeOneLine(row.error)) : text(null)) },
670673
]
671674

@@ -707,6 +710,12 @@ function profileListingContext(command: Command): { activeName: string; output:
707710
return { activeName: profile.name, output: profile.output }
708711
} catch (error) {
709712
if (!(error instanceof ProfileConfigError)) throw error
713+
// A blank `--profile`/`--endpoint`/`--workspace` is the caller's own
714+
// argument, not a broken profile. The tolerance below exists so a listing
715+
// still happens when the config is unreadable; letting it also absorb a
716+
// refused flag turned `sim --workspace "" profiles` into a successful
717+
// listing while every other command exits 1 on the same argv.
718+
if (error instanceof ProfileOverrideError) throw error
710719

711720
const globals = globalsOf(command)
712721
const named = globals.profile || process.env.SIM_PROFILE

packages/sim-cli/src/config/profile.test.ts

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {
1010
listAuthenticationDependents,
1111
listProfiles,
1212
OUTPUT_FORMATS,
13+
ProfileOverrideError,
1314
resolveAuthenticationProfileName,
1415
resolveProfile,
1516
validateProfileName,
@@ -525,6 +526,16 @@ describe('blank root flags', () => {
525526
expect(() => resolveProfile({ profile: '' })).toThrow(/--profile requires a value/)
526527
})
527528

529+
/**
530+
* `profiles` tolerates a profile that will not resolve, so the refusal has to
531+
* be tellable apart from a broken profile by something the wording cannot
532+
* break — otherwise a blank flag is absorbed and the listing exits 0.
533+
*/
534+
it('raises the refusal as its own error class', () => {
535+
expect(() => resolveProfile({ workspaceId: '' })).toThrow(ProfileOverrideError)
536+
expect(() => resolveProfile({ profile: 'unknown' })).not.toThrow(ProfileOverrideError)
537+
})
538+
528539
it('still reads an exported-but-empty environment variable as unset', () => {
529540
// The convention every profile-based CLI follows, and the reason the empty
530541
// string cannot simply become significant everywhere.
@@ -590,6 +601,32 @@ describe('redaction of rejected values', () => {
590601
expect(message).not.toMatch(FORBIDDEN_IN_VALUE)
591602
})
592603

604+
/**
605+
* The header line is split on `\n`, so a stored name cannot carry one — but
606+
* U+2028 is a line separator the reader keeps and `sanitize` does not strip,
607+
* which is why the same redaction the typed name already got has to cover the
608+
* two halves of this message that come out of the config file.
609+
*/
610+
it('redacts the suggestion and the configured list, not just the typed name', () => {
611+
writeFileSync(configPath(), '[profile st\u2028aging]\nworkspace = ws_1\n')
612+
613+
const message = messageOf(() => resolveProfile({ profile: 'st\u2028agng' }))
614+
expect(message).toBe(
615+
'Unknown profile "st agng". Did you mean "st aging"? Configured profiles: st aging.'
616+
)
617+
expect(message).not.toMatch(FORBIDDEN_IN_VALUE)
618+
})
619+
620+
it('redacts both names an auth_profile refusal quotes', () => {
621+
// ESC rather than U+2028 here: the value reader drops a line separator, so
622+
// the setting would never be seen at all.
623+
writeFileSync(configPath(), '[profile de\u001bv]\nauth_profile = mis\u001bsing\n')
624+
625+
const message = messageOf(() => resolveAuthenticationProfileName('de\u001bv'))
626+
expect(message).toBe('Profile "de v" references missing auth_profile "mis sing".')
627+
expect(message).not.toMatch(FORBIDDEN_IN_VALUE)
628+
})
629+
593630
it('redacts the format in the unknown-output-format refusal', () => {
594631
process.env.SIM_OUTPUT = 'ev\nil'
595632

0 commit comments

Comments
 (0)