From 44f0fc09b443c8282255bede29b601480eada0a3 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 28 Aug 2026 19:34:20 -0700 Subject: [PATCH] fix(jira): scope the user-field rewrite to declared fields and decode search rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review threads on #17090. The {accountId} marker is a shape, not a type, so createIssue rewrote any customFields value that merely looked like a user. The renderer already knows Jira's verdict — schema.type 'user', or an array of them — so it now sends that key list and the host rewrites only those keys. A key it was not told about is unknown, and unknown is left exactly as it arrived. normalizeJiraUserSearchResult cast each users entry to JiraUser without checking it, so a row with no accountId or displayName reached the picker, which keys and labels rows by both. Each entry is decoded now; one unreadable row fails the whole search into the unexpected-response path this PR already built, because a filtered list would look like the site's full answer. Moved the Jira IPC argument normalization into its own module to stay under the 300-line cap without a suppression. --- src/main/ipc/jira-ipc-arguments.ts | 47 ++++++++++++ src/main/ipc/jira.ts | 25 +------ .../jira/jira-create-reporter-payload.test.ts | 69 +++++++++++++++++- src/main/jira/jira-issue-mutations.ts | 10 ++- src/main/runtime/rpc/methods/jira.ts | 8 ++- src/preload/index.ts | 1 + .../task-page-jira-create-fields.test.ts | 21 ++++++ .../task-page-jira-create-fields.ts | 6 ++ .../hooks/use-task-page-create-jira-submit.ts | 9 ++- .../src/runtime/runtime-jira-client.test.ts | 72 ++++++++++++++++++- .../src/runtime/runtime-jira-user-search.ts | 68 +++++++++++++++--- src/shared/jira-types.ts | 4 ++ src/shared/jira-user-field-value.ts | 13 ++++ 13 files changed, 314 insertions(+), 39 deletions(-) create mode 100644 src/main/ipc/jira-ipc-arguments.ts diff --git a/src/main/ipc/jira-ipc-arguments.ts b/src/main/ipc/jira-ipc-arguments.ts new file mode 100644 index 00000000000..e94cfeac6db --- /dev/null +++ b/src/main/ipc/jira-ipc-arguments.ts @@ -0,0 +1,47 @@ +import type { JiraCreateIssueArgs } from '../../shared/jira-types' + +export function normalizeTrimmedArg(value: unknown): string | undefined { + return typeof value === 'string' && value.trim() ? value.trim() : undefined +} + +// IPC args are untrusted, so a malformed key list must read as "nothing declared" +// rather than reaching the resolver and widening what it rewrites. +function normalizeFieldKeyList(value: unknown): string[] | undefined { + if (!Array.isArray(value)) { + return undefined + } + const keys = value.filter( + (key): key is string => typeof key === 'string' && key.trim().length > 0 + ) + return keys.length > 0 ? keys : undefined +} + +export function normalizeJiraCreateIssueArgs( + args: JiraCreateIssueArgs +): { ok: true; args: JiraCreateIssueArgs } | { ok: false; error: string } { + const projectId = normalizeTrimmedArg(args?.projectId) + if (!projectId) { + return { ok: false, error: 'Project is required.' } + } + const issueTypeId = normalizeTrimmedArg(args.issueTypeId) + if (!issueTypeId) { + return { ok: false, error: 'Issue type is required.' } + } + const title = normalizeTrimmedArg(args.title) + if (!title) { + return { ok: false, error: 'Title is required.' } + } + return { + ok: true, + args: { + siteId: normalizeTrimmedArg(args.siteId), + projectId, + issueTypeId, + title, + description: args.description?.trim() || undefined, + customFields: + args.customFields && typeof args.customFields === 'object' ? args.customFields : undefined, + userFieldKeys: normalizeFieldKeyList(args.userFieldKeys) + } + } +} diff --git a/src/main/ipc/jira.ts b/src/main/ipc/jira.ts index 92b959ccb61..a59b4923d39 100644 --- a/src/main/ipc/jira.ts +++ b/src/main/ipc/jira.ts @@ -2,6 +2,7 @@ import { ipcMain } from 'electron' import { connect, disconnect, getStatus, selectSite, testConnection } from '../jira/client' import { _resetPreflightCache } from './preflight' import { JiraCancellableRequests } from './jira-cancellable-requests' +import { normalizeJiraCreateIssueArgs, normalizeTrimmedArg } from './jira-ipc-arguments' import { addIssueComment, createIssue, @@ -32,10 +33,6 @@ const VALID_FILTERS = new Set(['assigned', 'reported', 'all', ' const issueSummaryRequests = new JiraCancellableRequests() const searchRequests = new JiraCancellableRequests() -function normalizeTrimmedArg(value: unknown): string | undefined { - return typeof value === 'string' && value.trim() ? value.trim() : undefined -} - function normalizeSiteSelection(value: unknown): JiraSiteSelection | undefined { const siteId = normalizeTrimmedArg(value) return siteId as JiraSiteSelection | undefined @@ -191,24 +188,8 @@ export function registerJiraHandlers(): void { }) ipcMain.handle('jira:createIssue', async (_event, args: JiraCreateIssueArgs) => { - if (typeof args?.projectId !== 'string' || !args.projectId.trim()) { - return { ok: false, error: 'Project is required.' } - } - if (typeof args?.issueTypeId !== 'string' || !args.issueTypeId.trim()) { - return { ok: false, error: 'Issue type is required.' } - } - if (typeof args?.title !== 'string' || !args.title.trim()) { - return { ok: false, error: 'Title is required.' } - } - return createIssue({ - siteId: normalizeTrimmedArg(args.siteId), - projectId: args.projectId.trim(), - issueTypeId: args.issueTypeId.trim(), - title: args.title.trim(), - description: args.description?.trim() || undefined, - customFields: - args.customFields && typeof args.customFields === 'object' ? args.customFields : undefined - }) + const normalized = normalizeJiraCreateIssueArgs(args) + return normalized.ok ? createIssue(normalized.args) : normalized }) ipcMain.handle( diff --git a/src/main/jira/jira-create-reporter-payload.test.ts b/src/main/jira/jira-create-reporter-payload.test.ts index ac006e95ff5..c78ea5f5a48 100644 --- a/src/main/jira/jira-create-reporter-payload.test.ts +++ b/src/main/jira/jira-create-reporter-payload.test.ts @@ -59,7 +59,10 @@ async function createWithReporter( title: 'Broken login', // Exactly the marker the create dialog builds from a picked user; the // renderer half of that seam is pinned in task-page-jira-create-fields.test. - customFields: { reporter: buildJiraUserFieldValue(draft) } + customFields: { reporter: buildJiraUserFieldValue(draft) }, + // The dialog derives this from Jira's create metadata, which declares + // reporter as schema.type 'user'. + userFieldKeys: ['reporter'] }) expect(result.ok).toBe(true) return postedFields() @@ -108,3 +111,67 @@ describe('Jira create reporter payload', () => { expect(fields.customfield_2).toBe('free text') }) }) + +// Thread 1: the {accountId} marker is structural, so without Jira's own verdict on +// which keys are user fields any lookalike object would be rewritten on its way out. +describe('Jira create user-field scoping', () => { + beforeEach(() => { + vi.clearAllMocks() + isAuthErrorMock.mockReturnValue(false) + jiraRequestMock.mockReset() + }) + + async function createWithFields( + customFields: Record, + userFieldKeys?: string[], + authType?: 'cloud' | 'server' + ): Promise> { + getClientsMock.mockReturnValue([entry(authType)]) + jiraRequestMock.mockResolvedValue({ id: '1', key: 'ENG-1', self: 'https://example' }) + const { createIssue } = await import('./jira-issue-mutations') + const result = await createIssue({ + projectId: '100', + issueTypeId: '10001', + title: 'Broken login', + customFields, + userFieldKeys + }) + expect(result.ok).toBe(true) + return postedFields() + } + + it('leaves an accountId-shaped value alone on a field Jira did not declare as a user field', async () => { + const fields = await createWithFields( + { reporter: { accountId: '5abc' }, customfield_1: { accountId: 'not-a-user' } }, + ['reporter'] + ) + + expect(fields.reporter).toEqual({ id: '5abc' }) + expect(fields.customfield_1).toEqual({ accountId: 'not-a-user' }) + }) + + it('leaves an accountId-shaped value alone when no field types were declared at all', async () => { + const fields = await createWithFields({ customfield_1: { accountId: 'not-a-user' } }) + + expect(fields.customfield_1).toEqual({ accountId: 'not-a-user' }) + }) + + it('keeps resolving every entry of a declared array-of-users field', async () => { + const fields = await createWithFields( + { customfield_2: [{ accountId: '5abc' }, { accountId: '5def' }] }, + ['customfield_2'] + ) + + expect(fields.customfield_2).toEqual([{ id: '5abc' }, { id: '5def' }]) + }) + + it('keeps resolving a declared array-of-users field for Server/DC', async () => { + const fields = await createWithFields( + { customfield_2: [{ accountId: 'ada' }, { accountId: 'grace' }] }, + ['customfield_2'], + 'server' + ) + + expect(fields.customfield_2).toEqual([{ name: 'ada' }, { name: 'grace' }]) + }) +}) diff --git a/src/main/jira/jira-issue-mutations.ts b/src/main/jira/jira-issue-mutations.ts index 94c62b02704..ad27c061be9 100644 --- a/src/main/jira/jira-issue-mutations.ts +++ b/src/main/jira/jira-issue-mutations.ts @@ -7,7 +7,7 @@ import type { import { acquire, release } from './request-queue' import { apiBasePath, jiraRequest } from './authenticated-request' import { clearToken, getClients, isAuthError } from './client' -import { resolveJiraUserFieldValues } from '../../shared/jira-user-field-value' +import { resolveJiraCreateFieldValue } from '../../shared/jira-user-field-value' import { issueUrl, toBodyText } from './jira-issue-mapping' import type { JiraRecord } from './jira-record-pages' @@ -31,13 +31,19 @@ export async function createIssue(args: JiraCreateIssueArgs): Promise( entry, diff --git a/src/main/runtime/rpc/methods/jira.ts b/src/main/runtime/rpc/methods/jira.ts index 242231d2e90..39b47fb91ac 100644 --- a/src/main/runtime/rpc/methods/jira.ts +++ b/src/main/runtime/rpc/methods/jira.ts @@ -56,7 +56,10 @@ const CreateIssue = z.object({ issueTypeId: requiredString('Issue type is required'), title: requiredString('Title is required'), description: OptionalPlainString, - customFields: z.record(z.string(), z.unknown()).optional() + customFields: z.record(z.string(), z.unknown()).optional(), + // Optional so an older client that never sends it still decodes; the host then + // declares nothing a user field and rewrites nothing. + userFieldKeys: z.array(z.string()).optional() }) const IssueUpdate = z.object({ @@ -202,7 +205,8 @@ export const JIRA_METHODS: RpcAnyMethod[] = [ issueTypeId: params.issueTypeId.trim(), title: params.title.trim(), description: params.description?.trim() || undefined, - customFields: params.customFields + customFields: params.customFields, + userFieldKeys: params.userFieldKeys }) }), defineMethod({ diff --git a/src/preload/index.ts b/src/preload/index.ts index f9374319da5..20af538ac0b 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -2072,6 +2072,7 @@ const api = { title: string description?: string customFields?: Record + userFieldKeys?: string[] }): Promise< { ok: true; id: string; key: string; url: string } | { ok: false; error: string } > => ipcRenderer.invoke('jira:createIssue', args), diff --git a/src/renderer/src/components/task-page-jira-create-fields.test.ts b/src/renderer/src/components/task-page-jira-create-fields.test.ts index f54713c46de..d909e85fe72 100644 --- a/src/renderer/src/components/task-page-jira-create-fields.test.ts +++ b/src/renderer/src/components/task-page-jira-create-fields.test.ts @@ -6,6 +6,7 @@ import { findJiraCreateAllowedValue, getJiraCreateAllowedValueLabel, getJiraCreateOptionPayload, + getJiraUserCreateFieldKeys, isJiraUserCreateField, isVisibleJiraCreateField } from './task-page-jira-create-fields' @@ -233,3 +234,23 @@ describe('buildJiraCreateFieldValue for user fields', () => { }) }) }) + +// The host rewrites the {accountId} marker only for the keys named here, so this +// list is the whole reason a lookalike value on another field survives untouched. +describe('getJiraUserCreateFieldKeys', () => { + it('names only the fields Jira declares as users, single and array alike', () => { + expect( + getJiraUserCreateFieldKeys([ + field({ key: 'reporter', schema: { type: 'user' } }), + field({ key: 'customfield_watchers', schema: { type: 'array', items: 'user' } }), + field({ key: 'customfield_opt', schema: { type: 'option' } }), + field({ key: 'customfield_text', schema: { type: 'string' } }), + field({ key: 'customfield_untyped' }) + ]) + ).toEqual(['reporter', 'customfield_watchers']) + }) + + it('names nothing when the issue type has no user field', () => { + expect(getJiraUserCreateFieldKeys([field({ key: 'customfield_opt' })])).toEqual([]) + }) +}) diff --git a/src/renderer/src/components/task-page-jira-create-fields.ts b/src/renderer/src/components/task-page-jira-create-fields.ts index 3f173ae003c..74826b9c454 100644 --- a/src/renderer/src/components/task-page-jira-create-fields.ts +++ b/src/renderer/src/components/task-page-jira-create-fields.ts @@ -14,6 +14,12 @@ export function isJiraUserCreateField(field: JiraCreateField): boolean { return field.schema?.type === 'user' || field.schema?.items === 'user' } +// The host's {accountId} rewrite is shape-based, so it needs Jira's verdict on +// which keys are user fields rather than inferring it from the value. +export function getJiraUserCreateFieldKeys(fields: readonly JiraCreateField[]): string[] { + return fields.filter(isJiraUserCreateField).map((field) => field.key) +} + export function getJiraCreateAllowedValueLabel( value: NonNullable[number] ): string { diff --git a/src/renderer/src/components/task-page/hooks/use-task-page-create-jira-submit.ts b/src/renderer/src/components/task-page/hooks/use-task-page-create-jira-submit.ts index 865d98642c5..502c0003fe7 100644 --- a/src/renderer/src/components/task-page/hooks/use-task-page-create-jira-submit.ts +++ b/src/renderer/src/components/task-page/hooks/use-task-page-create-jira-submit.ts @@ -3,7 +3,10 @@ import { toast } from 'sonner' import { translate } from '@/i18n/i18n' import { jiraCreateIssue, jiraGetIssue } from '@/runtime/runtime-jira-client' -import { buildJiraCreateCustomFields } from '@/components/task-page-jira-create-fields' +import { + buildJiraCreateCustomFields, + getJiraUserCreateFieldKeys +} from '@/components/task-page-jira-create-fields' import type { GlobalSettings } from '../../../../../shared/global-settings-types' import type { JiraCreateField, @@ -72,6 +75,7 @@ export function useTaskPageCreateJiraSubmit({ visibleJiraCreateFields, newJiraIssueCustomFieldValues ) + const userFieldKeys = getJiraUserCreateFieldKeys(visibleJiraCreateFields) setNewJiraIssueSubmitting(true) const submitProviderRuntimeContextKey = providerRuntimeContextKey try { @@ -81,7 +85,8 @@ export function useTaskPageCreateJiraSubmit({ issueTypeId: newJiraIssueTargetType.id, title, description: newJiraIssueBody || undefined, - customFields + customFields, + userFieldKeys }) if (submitProviderRuntimeContextKey !== providerRuntimeContextKeyRef.current) { return diff --git a/src/renderer/src/runtime/runtime-jira-client.test.ts b/src/renderer/src/runtime/runtime-jira-client.test.ts index 173c514545d..5a10b908ea1 100644 --- a/src/renderer/src/runtime/runtime-jira-client.test.ts +++ b/src/renderer/src/runtime/runtime-jira-client.test.ts @@ -230,11 +230,14 @@ describe('runtime Jira client search bounds', () => { describe('jiraSearchUsers', () => { it('passes the project scope straight through on a local host', async () => { - jiraSearchUsersLocal.mockResolvedValue({ ok: true, users: [{ accountId: '5abc' }] }) + jiraSearchUsersLocal.mockResolvedValue({ + ok: true, + users: [{ accountId: '5abc', displayName: 'Alex Doe' }] + }) await expect(jiraSearchUsers(null, { projectIdOrKey: 'ENG', query: 'Alex' })).resolves.toEqual({ ok: true, - users: [{ accountId: '5abc' }] + users: [{ accountId: '5abc', displayName: 'Alex Doe' }] }) expect(jiraSearchUsersLocal).toHaveBeenCalledWith({ projectIdOrKey: 'ENG', query: 'Alex' }) }) @@ -269,6 +272,71 @@ describe('jiraSearchUsers', () => { } }) + // Thread 2: every entry is cast, not checked. The picker keys rows by accountId + // and labels them by displayName, so an entry missing either is an unusable row. + const malformedEntries: [string, unknown][] = [ + ['no accountId', { displayName: 'Alex Doe' }], + ['a blank accountId', { accountId: ' ', displayName: 'Alex Doe' }], + ['a non-string accountId', { accountId: 7, displayName: 'Alex Doe' }], + ['no displayName', { accountId: '5abc' }], + ['a blank displayName', { accountId: '5abc', displayName: ' ' }], + ['a non-string displayName', { accountId: '5abc', displayName: 7 }], + ['a non-string email', { accountId: '5abc', displayName: 'Alex Doe', email: 7 }], + ['a non-string avatarUrl', { accountId: '5abc', displayName: 'Alex Doe', avatarUrl: 7 }], + ['a non-object entry', 'Alex Doe'], + ['a null entry', null] + ] + for (const [label, entry] of malformedEntries) { + it(`fails the search when an entry has ${label}`, async () => { + jiraSearchUsersLocal.mockResolvedValue({ + ok: true, + users: [{ accountId: '5abc', displayName: 'Alex Doe' }, entry] + }) + + await expect(jiraSearchUsers(null, { projectIdOrKey: 'ENG' })).resolves.toEqual({ + ok: false, + error: 'Jira user search returned an unexpected response.' + }) + }) + } + + it('keeps a fully-formed list, including the optional fields Jira may omit or null out', async () => { + jiraSearchUsersLocal.mockResolvedValue({ + ok: true, + users: [ + { accountId: '5abc', displayName: 'Alex Doe', email: null }, + { + accountId: 'ada', + displayName: 'Ada L', + email: 'ada@x.test', + avatarUrl: 'https://a/x.png' + } + ] + }) + + await expect(jiraSearchUsers(null, { projectIdOrKey: 'ENG' })).resolves.toEqual({ + ok: true, + users: [ + { accountId: '5abc', displayName: 'Alex Doe', email: null }, + { + accountId: 'ada', + displayName: 'Ada L', + email: 'ada@x.test', + avatarUrl: 'https://a/x.png' + } + ] + }) + }) + + it('keeps an empty list reading as a successful empty search', async () => { + jiraSearchUsersLocal.mockResolvedValue({ ok: true, users: [] }) + + await expect(jiraSearchUsers(null, { projectIdOrKey: 'ENG' })).resolves.toEqual({ + ok: true, + users: [] + }) + }) + it('rejects an oversized query before it reaches the host', async () => { await expect( jiraSearchUsers( diff --git a/src/renderer/src/runtime/runtime-jira-user-search.ts b/src/renderer/src/runtime/runtime-jira-user-search.ts index 87b6a1cfe90..75a920438d6 100644 --- a/src/renderer/src/runtime/runtime-jira-user-search.ts +++ b/src/renderer/src/runtime/runtime-jira-user-search.ts @@ -4,6 +4,52 @@ import { callRuntimeRpc } from './runtime-rpc-client' import { isRuntimeProviderSearchQueryWithinLimit } from './runtime-provider-search-bounds' import { getJiraRuntimeTarget, type RuntimeJiraSettings } from './runtime-jira-target' +function unexpectedJiraUserSearchResponse(): JiraUserSearchResult { + return { + ok: false, + error: translate( + 'auto.runtime.runtime.jira.user.search.unexpectedResponse', + 'Jira user search returned an unexpected response.' + ) + } +} + +function nonBlankString(value: unknown): string | undefined { + return typeof value === 'string' && value.trim() ? value : undefined +} + +// The picker keys rows by accountId and labels them by displayName, so an entry +// missing either is a row that renders blank or selects nothing. The rows come +// from a Jira site we do not control, across Cloud and Server/DC and across +// versions, so each one is checked rather than cast. +function decodeJiraUser(value: unknown): JiraUser | undefined { + if (typeof value !== 'object' || value === null || Array.isArray(value)) { + return undefined + } + const candidate = value as { [K in keyof JiraUser]?: unknown } + const accountId = nonBlankString(candidate.accountId) + const displayName = nonBlankString(candidate.displayName) + if (!accountId || !displayName) { + return undefined + } + // Jira omits these or sends email as null when the directory hides it; any + // other shape means we are not reading the payload we think we are. + if (candidate.email !== undefined && candidate.email !== null) { + if (typeof candidate.email !== 'string') { + return undefined + } + } + if (candidate.avatarUrl !== undefined && typeof candidate.avatarUrl !== 'string') { + return undefined + } + return { + accountId, + displayName, + email: candidate.email as string | null | undefined, + avatarUrl: candidate.avatarUrl as string | undefined + } +} + // Runtime RPC results are cast, never decoded, so an unrecognized payload (an // older host, a shape change) must read as a failed search rather than an empty // one — an empty dropdown is what this picker exists to stop lying about. @@ -11,19 +57,25 @@ function normalizeJiraUserSearchResult(value: unknown): JiraUserSearchResult { if (typeof value === 'object' && value !== null) { const candidate = value as { ok?: unknown; users?: unknown; error?: unknown } if (candidate.ok === true && Array.isArray(candidate.users)) { - return { ok: true, users: candidate.users as JiraUser[] } + const users: JiraUser[] = [] + for (const entry of candidate.users) { + const user = decodeJiraUser(entry) + // One unreadable row fails the whole search. Filtering would hand back a + // short list that looks like the site's full answer, which is the same + // lie as the empty dropdown, and the error path still lets the user paste + // an account id the search never surfaced. + if (!user) { + return unexpectedJiraUserSearchResponse() + } + users.push(user) + } + return { ok: true, users } } if (candidate.ok === false && typeof candidate.error === 'string' && candidate.error) { return { ok: false, error: candidate.error } } } - return { - ok: false, - error: translate( - 'auto.runtime.runtime.jira.user.search.unexpectedResponse', - 'Jira user search returned an unexpected response.' - ) - } + return unexpectedJiraUserSearchResponse() } export async function jiraSearchUsers( diff --git a/src/shared/jira-types.ts b/src/shared/jira-types.ts index e29ebdd2e9f..602255c334c 100644 --- a/src/shared/jira-types.ts +++ b/src/shared/jira-types.ts @@ -164,6 +164,10 @@ export type JiraCreateIssueArgs = { title: string description?: string customFields?: Record + // Keys Jira's own create metadata declares as user fields (schema.type 'user', + // or an array of them). The {accountId} marker is only a shape, so the host + // rewrites these keys and nothing else; an undeclared key is left as it arrived. + userFieldKeys?: string[] } export type JiraCreateIssueResult = diff --git a/src/shared/jira-user-field-value.ts b/src/shared/jira-user-field-value.ts index d779b9d3155..f57606ba473 100644 --- a/src/shared/jira-user-field-value.ts +++ b/src/shared/jira-user-field-value.ts @@ -31,6 +31,19 @@ export function resolveJiraUserFieldValue( return authType === 'server' ? { name: accountId } : { id: accountId } } +// Only a field Jira declared as a user field is rewritten. The marker is a shape, +// not a type, so a lookalike object on an undeclared key would otherwise be +// silently retyped on its way to Jira; undeclared means unknown, and unknown is +// left exactly as it arrived. +export function resolveJiraCreateFieldValue( + fieldKey: string, + value: unknown, + userFieldKeys: ReadonlySet, + authType: JiraAuthType | undefined +): unknown { + return userFieldKeys.has(fieldKey) ? resolveJiraUserFieldValues(value, authType) : value +} + // Leaves every non-user value untouched so other providers' and Jira's own // option/number/text fields keep flowing through unchanged. export function resolveJiraUserFieldValues(