Repository navigation
fix(relay): apply ingest's content checks to workflow SendMessage - #9229
Draft
ThePumpingLemma wants to merge 1 commit into
Draft
ThePumpingLemma wants to merge 1 commit into
ThePumpingLemma wants to merge 1 commit into
Conversation
Problem: ingest refuses event content over 256 KB and content or tags
containing a NUL byte (Postgres `text` cannot store `\0`). The workflow
sink's SendMessage builds and inserts a relay-signed kind:9 directly,
so it skipped both checks. Its text is rendered from templates that can
include a `call_webhook` response body (up to 1 MiB, decoded with
`String::from_utf8_lossy`, which keeps `\0`).
How it fails: a webhook body between 256 KB and 1 MiB is stored as a
message ingest would have refused; a body with a NUL byte fails at
insert and the run step fails with `ActionSinkError::Database` instead
of naming the bad input.
Change: the size limit and the content NUL check become shared
`pub(crate)` primitives in ingest (`MAX_EVENT_CONTENT_BYTES`,
`check_content_size`, `check_content_nul`, with a `StoredContentError`
whose Display omits the `invalid:` prefix). `ingest_event_inner` keeps
its ordering: the size check first, the NUL check (content and tags)
after auth, scope and the write restriction, before command routing.
Its rejection messages are unchanged. The sink runs both checks on the
rendered text before any database work and maps a failure to
`ActionSinkError::InvalidInput`. Tags in the sink are built from parsed
or stored values (UUID, pubkeys, resolved event ids), so the text is
the only value that needs checking there.
Why safe: ingest behaviour and messages are unchanged; the sink only
refuses text that ingest would refuse or that could not be stored.
Tests: `send_message_refuses_unstorable_text_before_touching_the_database`
drives `RelayActionSink::send_message` against an unreachable database.
Before the fix NUL text returned `Database("... pool timed out ...")`;
after it NUL and over-limit text return `InvalidInput`, and text at the
limit still reaches the database.
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.
Stacked on #9180 (which adds
reject_nul_bytes); retarget tomainonce it merges.Problem
Ingest refuses event content over 256 KB and content or tags with a NUL byte, because Postgres
textcannot store\0. The workflow sink's SendMessage builds a relay-signed kind:9 and inserts it directly, so neither check runs. Its text comes from templates, and a template can include acall_webhookresponse body. That body can be up to 1 MiB and is decoded withString::from_utf8_lossy, which keeps\0. As a result:ActionSinkError::Databaseinstead of an error that names the bad input.Change
handlers/ingest.rs: the size limit and the content NUL check are now sharedpub(crate)pieces:MAX_EVENT_CONTENT_BYTES,check_content_size,check_content_nul, and aStoredContentErrorwhoseDisplayleaves off theinvalid:prefix.ingest_event_innerkeeps its current order. The size check runs first. The NUL check (content and tags, viareject_nul_bytes) runs after auth, scope and the write restriction, before command routing.workflow_sink.rs:send_messageruns both checks on the rendered text before any database work. A failure becomesActionSinkError::InvalidInput. The failed run then records codewebhook_failedwith the messagewebhook error: invalid input: <reason>(every sink error already maps this way).Testing
workflow_sink::tests::send_message_refuses_unstorable_text_before_touching_the_database. It drivesRelayActionSink::send_messageagainst an unreachable database:InvalidInput.Database.cargo test -p buzz-relay --lib send_message_refuses_unstorable_textfailed withcargo test -p buzz-relay --lib -- workflow_sink nul_bytes: 23 passed, 5 ignored (Postgres).cargo test -p buzz-relay --lib -- handlers::ingest: 187 passed, 22 ignored (Postgres).cargo fmt --allandcargo clippy -p buzz-relay --all-targets -- -D warningsare clean.rust-testshook passed.Not covered
workflow_sinktests were not run (no local database).moderation_notices.rs: the notice body is relay-formatted text around an operator-authoredpublic_reason, which is already stored in Postgres, so no NUL can be in it. No webhook or other untrusted text reaches it. Left unchanged.side_effects.rs, git announcements, audio) build their content from relay or stored values, not caller-supplied text. Left unchanged.Review
Blockers: none. Checked the following:
event.contentbytes against the same constant as ingest.ActionSinkhas no other relay-signed send method.Open (optional):
call_webhookbody does not yet show up in the run record. That webhook step's output is already in the execution trace, and Postgres JSONB refuses NUL escapes, so writing the run's final status fails and the run staysrunning. This happened before this PR as well. For now the new NUL check is a defensive guard. Follow-up: make the webhook body storable when it is decoded (for example, replace\0the way invalid UTF-8 is already replaced) or refuse it at the webhook step.Exercised on a local relay
Commit tested:
f4dc6ddf3(stacked on #9180). Dev profile, isolated harness relay on :3030. The sender is the channel owner. Workflows were created withbuzz workflows create, each with amessage_postedtrigger, and triggered withbuzz messages send.A local HTTP server cannot be used.
call_webhookrunscheck_ssrf, which rejects loopback and private addresses, and there is no override. The workflow pointing athttp://127.0.0.1:8099failed withwebhook error: SSRF blocked: '127.0.0.1' resolved to private/reserved address 127.0.0.1. So the content cases used public httpbin endpoints:/base64/…returns the bodya\0b WFNUL/drip?numbytes=300000returns a 300,000-byte bodyIn each workflow the body was templated into
send_messageas{{steps.hook.output.body}}.workflow_runs)call_webhookbody over 256 KB →send_messagefailed,webhook_failed:webhook error: invalid input: content exceeds maximum size of 262144 bytes (got 300005)call_webhookbody with NUL →send_messageWorkflow run failed … webhook_failed), but the run row staysrunningwith an empty trace (see defect)call_webhookbody →send_messagecompletedhook said: clean webhook bodysend_messagecompletedplain workflow messagecall_webhookto 127.0.0.1failed(SSRF blocked)Defect (not introduced here): with
RUST_LOG=buzz_workflow=debug, the NUL run logs the sequence below.SendMessage → …: hook said: a\u0000b WFNULWorkflow run failed … webhook_failedFailed to update run to Failed: database error: … unsupported Unicode escape sequenceThe completed
hookstep's output, which is the webhook body containing\u0000, is written into theexecution_tracejsonb column, and Postgres jsonb rejects\u0000. So the failure is never recorded, the run staysrunningindefinitely, and the content error this PR adds is never visible on the run. The same would happen for any step output containing NUL, even without asend_message.Postgres-gated tests: I pointed
DATABASE_URL,BUZZ_TEST_DATABASE_URLandBUZZ_TEST_REDIS_URLat the harness.cargo test -p buzz-relay --lib -- --ignored --test-threads=1 workflow_sink nul_bytes: 5 passed, 0 failed.send_message_refuses_unstorable_text_before_touching_the_database: 1 passed.Not exercised: