Repository navigation
fix(workflow): replace NUL in webhook output and trigger fields - #9242
Draft
ThePumpingLemma wants to merge 1 commit into
Draft
ThePumpingLemma wants to merge 1 commit into
ThePumpingLemma wants to merge 1 commit into
Conversation
PostgreSQL jsonb rejects \u0000 ("unsupported Unicode escape sequence").
call_webhook decoded the response body with String::from_utf8_lossy,
which keeps NUL, and the body lands in the run's execution_trace. The
final status update then failed ("Failed to update run to Failed: ...")
and the run stayed `running` forever. Webhook-trigger and command
payload fields parsed from JSON ("\u0000" escapes) hit the same wall in
workflow_runs.trigger_context, failing run creation with a 500.
Replace U+0000 with U+FFFD at decode, the same substitution
from_utf8_lossy already makes for invalid UTF-8. Refusing the step
instead would fail a workflow over bytes a remote server chose to send,
and the body is already a lossy, best-effort rendering; keeping the run
going with a visible replacement character is consistent with that.
Trigger fields from the webhook endpoint and the workflow command
handler shared a duplicated flatten loop; both now go through
TriggerContext::extend_webhook_fields, which applies the same
replacement to keys and values.
Signed-off-by: David Grochowski <dgrochowski@squareup.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PostgreSQL
jsonbrejects\^@(unsupported Unicode escape sequence).call_webhookdecodes the response body withString::from_utf8_lossy, which keeps NUL, and the body goes into the run'sexecution_trace. When a webhook returns a NUL byte, the final status update fails (Failed to update run to Failed: … unsupported Unicode escape sequence) and the run staysrunningforever.Webhook-trigger bodies and workflow-command payloads have the same problem: JSON
"\^@"escapes parse into NUL inwebhook_fields, so writingworkflow_runs.trigger_contextfails and the webhook endpoint returns a 500.Change
call_webhooknow builds its output withwebhook_output, which decodes lossily and replaces U+0000 with U+FFFD. That's the same substitutionfrom_utf8_lossyalready makes for invalid UTF-8, and the run keeps going. I chose this over failing the step: the body is already a best-effort rendering, so failing a workflow over bytes a remote server sent would be inconsistent.api/bridge.rs) and the workflow command handler (handlers/command_executor.rs) had duplicate loops that flatten JSON into trigger fields. Both now callTriggerContext::extend_webhook_fields, which applies the same replacement to keys and values.Testing
Ran against an isolated
postgres:17-alpine(port 5481) with the schema applied bybin/pgschemaandreconcile-schema-after-pgschema.sql, plusredis:7-alpine(port 6391). I stopped both afterwards.Before the fix (with
storabletemporarily made the identity function):After the fix:
cargo test -p buzz-workflow --lib --features reqwest -- --include-ignored: 180 passedcargo test -p buzz-relay --lib -- --include-ignored workflow_webhook_postgres_tests: 2 passedcargo test -p buzz-relay --lib command_executor: passedcargo clippy --all-targets -- -D warningsforbuzz-workflow(with and withoutreqwest) andbuzz-relay: cleancargo fmt --all: cleanNot covered
add_reaction_implstill stores the relay's own reaction API response (resp.text(), which keeps NUL) in the trace. That response comes from our own relay, not from an arbitrary remote server.workflow_runs.error_messageisTEXT, which also rejects NUL. AWorkflowErrormessage carrying NUL would still fail theFailedupdate; for example, the AddReaction error embeds the relay's response body.build_trigger_context) aren't changed.events.contentisTEXT, so a NUL can't get past ingest.Review
Agent review, no blockers. Scores: minimalism 9, elegance 9, correctness 9.
webhook_fieldsareapi/bridge.rsandhandlers/command_executor.rs, and both now callTriggerContext::extend_webhook_fields. No copy of the old loop is left.create_workflow_run, or a run stuck inrunning. A condition can't have matched on NUL, because workflow definitions are stored asjsonband can't contain one.\u0000, so they're unchanged.storableis private toexecutor.rs, next to its two callers. Response headers aren't stored, andHeaderValuerejects NUL anyway."k\u0000"and"k\uFFFD"in the same body collapse to one key, and the last one in map order wins. The sender controls both keys, so there's no cross-tenant effect.call_webhook_implcall site has no test of its own. The SSRF guard blocks loopback, so the test goes throughwebhook_outputand the run below covers the call end to end.Exercised on a local relay
Isolated harness relay (
start-isolated-test-relay.sh --profile dev), workflows owned by a channel admin, each triggered withPOST /hooks/{id}:send_message 'trigger note: {{trigger.note}}'{"note":"a\u0000b"}completed;trigger_context.webhook_fields.note=a�b; message bytes…61 efbfbd 62call_webhook→httpbin.org/base64/YQBiIFdGTlVM(a\0b WFNUL), thensend_message{}completed; trace{"body":"a�b WFNUL","status":200}; messagehook said: a�b WFNULcall_webhook→ clean httpbin body, thensend_message{"note":"plain"}completed; bodyclean webhook body, message posted{"k\u0000":"nul","k\ufffd":"fffd"}completed; one keyk�=fffdPostgres-gated tests against the harness (
DATABASE_URL/REDIS_URLpointing at it):cargo test -p buzz-workflow --lib --features reqwest -- --include-ignored: 180 passedcargo test -p buzz-relay --lib -- --include-ignored workflow_webhook_postgres_tests: 2 passedAfterwards I tore the harness down with
docker compose -p buzz-harness -f docker-compose.harness.yml down -v.