fix(jira): scope the user-field rewrite to declared fields and decode search rows

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.
This commit is contained in:
Brennan Benson
2026-08-29 18:57:56 -07:00
committed by Merge Sim
parent a849f43e22
commit 44f0fc09b4
13 changed files with 314 additions and 39 deletions
+47
View File
@@ -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)
}
}
}
+3 -22
View File
@@ -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<JiraIssueFilter>(['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(
@@ -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<string, unknown>,
userFieldKeys?: string[],
authType?: 'cloud' | 'server'
): Promise<Record<string, unknown>> {
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' }])
})
})
+8 -2
View File
@@ -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<JiraCreate
if (args.description?.trim()) {
fields.description = toBodyText(entry.site, args.description.trim())
}
const userFieldKeys = new Set(args.userFieldKeys ?? [])
for (const [fieldKey, value] of Object.entries(args.customFields ?? {})) {
if (!fieldKey || value === undefined || value === null || value === '') {
continue
}
// User fields arrive as the provider-neutral {accountId} marker; only the
// host knows whether this site wants Cloud {id} or Server/DC {name}.
fields[fieldKey] = resolveJiraUserFieldValues(value, entry.site.authType)
fields[fieldKey] = resolveJiraCreateFieldValue(
fieldKey,
value,
userFieldKeys,
entry.site.authType
)
}
const created = await jiraRequest<{ id: string; key: string; self: string }>(
entry,
+6 -2
View File
@@ -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({
+1
View File
@@ -2072,6 +2072,7 @@ const api = {
title: string
description?: string
customFields?: Record<string, unknown>
userFieldKeys?: string[]
}): Promise<
{ ok: true; id: string; key: string; url: string } | { ok: false; error: string }
> => ipcRenderer.invoke('jira:createIssue', args),
@@ -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([])
})
})
@@ -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<JiraCreateField['allowedValues']>[number]
): string {
@@ -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
@@ -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(
@@ -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(
+4
View File
@@ -164,6 +164,10 @@ export type JiraCreateIssueArgs = {
title: string
description?: string
customFields?: Record<string, unknown>
// 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 =
+13
View File
@@ -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<string>,
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(