fix(audit): name the integration when a bulk sync pause or start targets nothing (NAN-6791) - #7304
Conversation
…ets nothing `POST /sync/pause` and `POST /sync/start` accept an empty `syncs` list to mean every sync, and the controller expands it after the audit resolver has run, so the event recorded no target at all. Fall back to the integration, which is the widest scope the request itself names. The same resolver also threw on a malformed member, discarding the valid targets beside it, and turned a non-string name into a fabricated `[object Object]::` id. Drop the invalid members instead. The MCP `syncs_set_state` tool emits the same two events off its own copy of the resolver with the same empty-list behaviour, so both now share one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 4 files
Confidence score: 4/5
- In
packages/server/lib/middleware/audit/sync.middleware.ts, malformed or empty requests with a numericprovider_config_keycan record an invalid integration target id such as"12345"; guard the raw value withnonEmptyStringbefore passing it tomakeTarget.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/server/lib/middleware/audit/sync.middleware.ts">
<violation number="1" location="packages/server/lib/middleware/audit/sync.middleware.ts:98">
P2: When an empty or malformed request supplies a numeric `provider_config_key`, this resolver records integration target id `"12345"` because `makeTarget` accepts numbers. Guard the raw value with `nonEmptyString` before the fallback so invalid requests are not attributed to a fabricated integration.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| .map(({ syncName, syncVariant }) => makeTarget('sync', syncTargetId(syncName, syncVariant))) | ||
| .filter((t): t is AuditTarget => Boolean(t)); | ||
| return targets.length > 0 ? targets : undefined; | ||
| return targets.length > 0 ? targets : makeTarget('integration', providerConfigKey); |
There was a problem hiding this comment.
P2: When an empty or malformed request supplies a numeric provider_config_key, this resolver records integration target id "12345" because makeTarget accepts numbers. Guard the raw value with nonEmptyString before the fallback so invalid requests are not attributed to a fabricated integration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/server/lib/middleware/audit/sync.middleware.ts, line 98:
<comment>When an empty or malformed request supplies a numeric `provider_config_key`, this resolver records integration target id `"12345"` because `makeTarget` accepts numbers. Guard the raw value with `nonEmptyString` before the fallback so invalid requests are not attributed to a fabricated integration.</comment>
<file context>
@@ -90,14 +90,32 @@ function syncBaseMeta(providerConfigKey: unknown, connectionId?: unknown): Recor
.map(({ syncName, syncVariant }) => makeTarget('sync', syncTargetId(syncName, syncVariant)))
.filter((t): t is AuditTarget => Boolean(t));
- return targets.length > 0 ? targets : undefined;
+ return targets.length > 0 ? targets : makeTarget('integration', providerConfigKey);
+}
+
</file context>
| return targets.length > 0 ? targets : makeTarget('integration', providerConfigKey); | |
| return targets.length > 0 ? targets : makeTarget('integration', nonEmptyString(providerConfigKey)); |
There was a problem hiding this comment.
Not applying this one.
An empty string is already handled — toAuditId returns undefined for '', so the suggestion changes nothing there. The only input it changes is a numeric key, and toAuditId coerces numbers to strings deliberately (several targets are numeric ids). On top of that, providerConfigKeySchema is z.string().regex(...), so {"provider_config_key": 12345} is a 400 and the event already records outcome: 'failure' — "12345" is verbatim what the caller sent, not a fabricated integration.
The convention is also one-sided: across the audit middleware there are 47 makeTarget( call sites and none wraps its value in nonEmptyString — ten pass a raw req.body/req.params value straight in (makeTarget('sync', req.body.scriptName), makeTarget('connection', req.params.connectionId)). toAuditId is the single place that decides what is a valid id, on purpose. Adding a guard here would make this the only exception.
The nonEmptyString four lines up isn't a general input guard either — it's there because a sync name gets concatenated into name::variant, where a non-string fabricates [object Object]::. A bare id has no such problem. I've reworded that comment to say so, since the asymmetry is a fair question to have.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The malformed-member test used a non-string name and a non-string variant at once, so it passed on the variant normalisation alone and stayed green when the name check was removed. Split it: one case per guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
POST /sync/pauseandPOST /sync/startaccept an emptysyncslist to mean "every sync", and the controller expands it only after the audit resolver has run, so those events recorded no target at all. They now fall back to the integration, which is the widest scope the request itself names. Measured in production: 4 of the 5 no-targetsync.startedevents in a 24h window were successful bulk calls, across 4 accounts.[object Object]::v2id. Invalid members are now dropped and the rest survive.syncs_set_statetool emits the same two events from its own copy of the resolver, with the same empty-list behaviour, so both now share one function.A
sync.pausedrow names the syncs when the caller named them and the integration when they did not, so the target type tells a reader which shape the call had. The connection is unchanged and stays in metadata, where both emitters already put it. Design and the reproduction matrix are on NAN-6791.Test plan
syncsfield, valid syncs surviving a malformed neighbour, a non-string name, a non-string variantts-build, oxlint and prettier clean; 284 audit + MCP unit tests passsync.paused/sync.startedrows drop to zero in ClickHouse