Raise ArgumentError on non-Hash/non-String patch_signals input#25
Closed
andriytyurnikov wants to merge 14 commits intostarfederation:mainfrom
Closed
Raise ArgumentError on non-Hash/non-String patch_signals input#25andriytyurnikov wants to merge 14 commits intostarfederation:mainfrom
andriytyurnikov wants to merge 14 commits intostarfederation:mainfrom
Conversation
Previously CI only exercised Ruby 4.0. Adds a build matrix so unit tests and the Datastar SDK conformance suite run on every supported Ruby. Also enables the workflow on pull_request events.
CI runs on ubuntu-latest. Without x86_64-linux locked, Bundler resolves platform-specific gems at install time on CI, which can cause version drift relative to local development. Locking the platform keeps CI reproducible.
bundle install fails on Ruby 3.1 because console 1.34.2 (transitive dep via async) requires ruby >= 3.2. The dependency tree's effective floor is 3.2.
Adds an optional generator_class: keyword to Dispatcher.new (defaulting to ServerSentEventGenerator) and routes the three internal generator instantiations — stream_one, stream_many's connection generator, and each per-stream generator inside spawned threads — through it. Lets downstream wrappers layer behavior (logging, metrics, input scrubbing, etc.) on top of the SDK by passing a subclass, instead of monkey-patching the SDK class.
Pre-fix #redirect interpolated the URL raw inside a single-quoted JS
string:
window.location = '#{url}'
A ' in url broke out of the literal, letting attacker-influenced
fragments execute as JS. The classic vulnerable pattern:
datastar.redirect("/page?ref=#{params[:ref]}")
is a Rails idiom that's safe under Rails' own redirect_to, so the
mismatch is a footgun.
Fix: encode the URL with JSON.generate(ascii_only: true,
escape_slash: true). The output is a properly-quoted JS string
literal that:
- escapes ", \, and control characters,
- escapes U+2028 / U+2029 (which terminate JS string literals
even when delimited by " or '),
- escapes / to \/ so a </script> substring in the URL can't
prematurely close the surrounding <script> tag during HTML
parsing (the parser does not recognize <\/script> as the end
tag, while \/ is a no-op inside a JS string literal).
Output format change: window.location is now wrapped in double
quotes with JSON-style escapes instead of single quotes. Behavior
at runtime is unchanged for safe URLs.
6 new specs cover single quotes, double quotes, backslashes,
</script> breakout, U+2028, and U+2029.
stream_one's lifecycle contract (handling_sync_errors) fires exactly one of on_server_disconnect / on_client_disconnect / on_error per stream — never a combination. stream_many violated this: its ensure block always fired on_server_disconnect, even when an error or client disconnect had already triggered the matching callback. Consumers writing observability/cleanup logic against these callbacks would see different behavior depending on whether they called .stream once or many times — a subtle source of bugs. Fix: track a completed_normally flag in stream_many's control thread, set it false when handle_streaming_error runs, and only fire on_server_disconnect in the ensure block when the flag is still true. 2 new shared examples (run for both ThreadExecutor and AsyncExecutor): - streamer raises → on_error fires, on_server_disconnect doesn't - client disconnects → on_client_disconnect fires, on_server_disconnect doesn't
Author
|
CI: run matrix against Ruby 3.2, 3.3, 3.4, 4.0
Lock x86_64-linux platform in Gemfile.lock
Allow Dispatcher to inject a custom generator_class
Make multi-stream disconnect callbacks mutually exclusive
Encode redirect URL as a safe JS string literal
Closes the SSE injection surface where attacker-controlled strings reaching element bodies, script bodies, attribute values, scalar option values, or array/hash option entries could forge SSE fields the browser then dispatches as legitimate events. The WHATWG SSE parser treats \r, \n, and \r\n all as line terminators. Two scrubbers, applied at the API boundary: - scrub_body: strip \r only (used for element/script bodies and String signal payloads). \n is preserved because the SDK splits bodies on \n to emit per-line `data:` fields. - scrub_option: recursively strip \r and \n from option values (Strings, Arrays, Hashes). Option values are written as single `data:` lines, so any embedded line terminator forges a field. Hash signals are unaffected — JSON.dump already escapes \r/\n in string values, so the serialized payload is always a single safe line. execute_script attribute values are scrubbed pre-tag-construction since they are interpolated raw and bypass patch_elements' splitter. 17 new specs covering element bodies, scalar/array/hash options, String and Hash signals, remove_elements, execute_script (body and attributes), redirect, and a top-level reproduction of the issue's attack vector.
Scrub CR/LF in SSE outputs to prevent field forging
The case/when in patch_signals had no else branch, so passing nil, an Array, an Integer, or any other non-Hash/non-String value silently produced an SSE event with only a header and no data: line — a malformed patch-signals event the browser would receive as effectively empty. The Dispatcher docstring already declares the contract: @param signals [Hash, String]. This makes the runtime match. Test coverage for nil, Array, Integer, Symbol.
andriytyurnikov
force-pushed
the
feature/patch-signals-validate-input
branch
from
18abff3 to
994c107
Compare
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
1 participant
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.
Summary
The
case/wheninServerSentEventGenerator#patch_signalshad noelse, so passingnil, anArray, anInteger, or any other non-Hash/non-String value silently produced a malformed event:— just the header, no
data: signalsline. The browser receives an effectively empty event and the caller has no idea anything went wrong.Fix
Add the missing
elsebranch — raiseArgumentError:The
Dispatcher#patch_signalsdocstring already declares this contract:# @param signals [Hash, String] signals to mergeThis makes the runtime match the documented contract.
Tests
4 new specs covering the failure modes:
nil,Array,Integer,Symbol. Full suite: 104 examples, 0 failures.Behavior change
Callers who were accidentally calling
patch_signals(nil)orpatch_signals(some_array)will now see anArgumentErrorimmediately instead of producing a silently-malformed wire payload. This is the better failure mode — surfaces caller bugs at the actual call site instead of producing a confused-browser at runtime.Test plan
patch_signalsHash and String paths still work