Remove trigger kwargs from the REST API response#67868
Conversation
| sensitive keys are masked here for consistency with how connection extras, variables | ||
| and rendered fields are already redacted. | ||
| """ | ||
| return str(redact(value)) |
There was a problem hiding this comment.
This will fundamentally break any trigger that gets caught by this. This is not merely display. It is changing the value that the Trigger class sees.
You say:
the triggerer still decrypts and
uses the real kwargs at runtime, so trigger execution is unaffected.
But how/where is that done?
There was a problem hiding this comment.
But how/where is that done?
The redact_kwargs is only called explicitly and only in one place - in the "TriggerResponse" that is passed as the regular API response (not TaskSDK).
We are running it in BeforeValidator, which means that this redaction takes place before the response is prepared to be wired to be sent as response of API call.
I looked where TriggerResponse - and seems that this class is only used in responses in regular REST API.
or am I wrong @ashb ?
There was a problem hiding this comment.
Ah no I was wrong.
So my other comment (about is this even worth returning stands) but if it does make sense to keep this:
This shouldn't have str() on it -- value is almost certainly a dict, which means this is relying on the stringification of a python dict, rather than keeping it as a json value.
|
Thinking about it, if this is part of the normal public API, not something used by the triggerer directly, perhaps we should just remove the trigger_kwargs from the response entirely? |
| # The sensitive value is masked; the key name and non-sensitive values are preserved. | ||
| assert "super-secret-value-123" not in kwargs | ||
| assert "'gemini_api_key': '***'" in kwargs | ||
| assert "'polling_interval': 30" in kwargs |
There was a problem hiding this comment.
| # The sensitive value is masked; the key name and non-sensitive values are preserved. | |
| assert "super-secret-value-123" not in kwargs | |
| assert "'gemini_api_key': '***'" in kwargs | |
| assert "'polling_interval': 30" in kwargs | |
| # The sensitive value is masked; the key name and non-sensitive values are preserved. | |
| assert kwargs == {"gemini_api_key": "***", "polling_interval": 30} |
c851984 to
bb487d3
Compare
|
Good call — dropped Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting |
| required: | ||
| - id | ||
| - classpath | ||
| - kwargs |
There was a problem hiding this comment.
Hmmmm, though removing this field means it's a breaking change on the API, and could possibly cause down stream consumers to fail with "no such property" type errors
There was a problem hiding this comment.
Yeah. Thought about it - what we could do instead is to leave it in but make it always empty. That is far less of a breaking change - the schema would remain and we could simply state in a documentation that it is here for historical reasons but kwargs are never populated.
I think this is. a bit of "Have cake and eat it too".
TriggerResponse.kwargs returned the decrypted trigger keyword arguments
verbatim (as a stringified Python dict). Those kwargs can contain
credentials a deferred operator hands to its trigger (an API key, a token),
so the field leaked secrets.
Rather than removing the field (a breaking schema change for API consumers),
keep it but always return it empty as "{}" -- the same value an empty-kwargs
trigger already produced under the previous str() serialization, so the field
type and OpenAPI schema are unchanged. The triggerer still decrypts and uses
the real kwargs at runtime; only the API representation is emptied.
bb487d3 to
974f0a9
Compare
|
Thanks @ashb — reworked based on your feedback. Rather than removing the field (which, as you noted, would be a breaking schema change for downstream consumers), Addressing your specific points:
Added a unit test asserting sensitive kwargs are redacted to Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting |
Why
TriggerResponse.kwargsreturned the decrypted trigger keyword arguments verbatim — and only as a stringified Pythondict, not a usable JSON value. Those kwargs can carry credentials a deferred operator hands to its trigger (an API key, a token, …), so the field both leaked secrets and wasn't machine-readable. It isn't consumed by the UI.What
Per review, rather than masking the value, the
kwargsfield is removed fromTriggerResponseentirely. The triggerer still decrypts and uses the real kwargs at runtime, so trigger execution is unaffected — only the API response no longer exposes them.trigger.kwargs).Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 4.8) following the guidelines