diff --git a/frontend/src/lib/components/copilot/chat/global/core.test.ts b/frontend/src/lib/components/copilot/chat/global/core.test.ts index 824e37c1b3..8fc22a60e3 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.test.ts @@ -4930,7 +4930,7 @@ describe('global AI tools', () => { // A secret the model picked is not consent, and a result that echoed one back would let // it propose the same value again on the next call. - it('run_script opens password fields empty and hides secrets from the model', async () => { + it('run_script empties a proposed secret but keeps a variable reference', async () => { vi.mocked(ScriptService.getScriptByPath).mockResolvedValueOnce({ path: 'f/scripts/rotate', schema: { @@ -4969,10 +4969,13 @@ describe('global AI tools', () => { ) ) - expect(shown).toEqual({ nested: {}, name: 'ada' }) + // The literal is emptied; the variable reference is the model's to send and survives, + // since the secret stays in the variable and only its path travels. + expect(shown).toEqual({ nested: { inner: '$var:u/ada/prod_api_key' }, name: 'ada' }) // Named, or an emptied field reads as the user having deleted the value and the // next call proposes the same secret again. - expect(result).toContain('token, nested.inner') + expect(result).toContain('token') + expect(result).not.toContain('nested.inner') expect(result).not.toContain('hunter2') expect(result).not.toContain('secret_arg') expect(result).not.toContain('prod_api_key') diff --git a/frontend/src/lib/components/job_args.test.ts b/frontend/src/lib/components/job_args.test.ts index ca0e3e3b8b..a68dc54dbe 100644 --- a/frontend/src/lib/components/job_args.test.ts +++ b/frontend/src/lib/components/job_args.test.ts @@ -164,14 +164,27 @@ describe('secret args at every level the form nests', () => { either: { kind: 'a', key: 'k', other: 'o' } } - it('strips every one of them', () => { + it('strips every one of them, keeping a variable reference', () => { expect(stripSecretArgs(args, schema)).toEqual({ - obj: { keep: 1 }, + // Naming a workspace variable is how a secret is meant to reach a job, and the + // value never leaves it — so the reference is the caller's to send. + obj: { inner: '$var:u/ada/prod', keep: 1 }, list: [{ name: 'a' }, {}], either: { kind: 'a' } }) }) + // `$jsonvar:` paths are minted from what the user typed into the form, so a caller naming + // one is naming a secret it was never shown. A bare `$var:` names nothing. + it('keeps only a reference that names a workspace variable', () => { + const one = { properties: { token: { type: 'string', password: true } } } + const kept = (v: unknown) => stripSecretArgs({ token: v }, one).token + expect(kept('$var:f/team/api_token')).toBe('$var:f/team/api_token') + expect(kept('$jsonvar:u/ada/secret_arg/abc123')).toBeUndefined() + expect(kept('$var:')).toBeUndefined() + expect(kept('hunter2')).toBeUndefined() + }) + it('redacts every one of them', () => { const redacted = JSON.stringify(redactSecretArgs(args, schema)) for (const secret of ['hunter2', 'prod', 'one', 'two', '"k"', '"o"']) { diff --git a/frontend/src/lib/components/job_args.ts b/frontend/src/lib/components/job_args.ts index 7bcdc3f401..186d43ad02 100644 --- a/frontend/src/lib/components/job_args.ts +++ b/frontend/src/lib/components/job_args.ts @@ -186,6 +186,13 @@ function mapLeaves( const isSecretProp = (prop: any) => !!prop?.password +/** A reference to a workspace variable, which is how a secret is meant to reach a job: the + * value stays in the variable and the argument carries only its path, so it is safe to show + * and safe to store. `$jsonvar:` is deliberately not one of these — those are minted from + * what the user typed, so a caller naming one is naming a secret it was never shown. */ +const isVariableRef = (value: unknown) => + typeof value === 'string' && /^\$var:\S/.test(value) + const isFileProp = (prop: any) => prop?.contentEncoding === 'base64' || prop?.items?.contentEncoding === 'base64' @@ -209,11 +216,15 @@ function mapArgLeaves( } /** - * Drop every password-typed argument, so a caller cannot propose a secret on the user's - * behalf: the field falls back to whatever the script itself declares, as on any other - * run form, and the user fills in the rest. Appends the path of - * each one removed, so the caller can be told the field was emptied rather than left to - * read the absence as the user having deleted it. + * Drop every password-typed argument holding a secret of its own, so a caller cannot propose + * one on the user's behalf: the field falls back to whatever the script itself declares, as on + * any other run form, and the user fills in the rest. Appends the path of each one removed, so + * the caller can be told the field was emptied rather than left to read the absence as the user + * having deleted it. + * + * A `$var:` reference is kept: naming a workspace variable is how a secret is meant to reach a + * job, the caller can list those already, and the field's own widget treats an incoming + * reference as filled rather than minting over it. */ export function stripSecretArgs( args: Record, @@ -221,6 +232,7 @@ export function stripSecretArgs( strippedKeys?: string[] ): Record { return mapArgLeaves(args, schema, isSecretProp, (value, _prop, path) => { + if (isVariableRef(value)) return value if (value !== undefined) strippedKeys?.push(path) return undefined })