diff --git a/CHANGELOG.md b/CHANGELOG.md index fd33e1c6..d6cfea0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +# Unreleased + +- ***(SECURITY)*** Prevent SSE injection in `Kemal::EventStream`: reject newlines in `event`/`id`, normalize CR/LF in `data`/`comment`. Thanks @hahwul for the report. Thanks @sdogruyol for the fix :pray: +- Fix URL params being decoded again on every request when route lookup results are cached. Thanks @hahwul for the report. Thanks @sdogruyol for the fix :pray: + # 1.12.0 (21-07-2026) - Crystal 1.21.0 support :tada: diff --git a/spec/context_spec.cr b/spec/context_spec.cr index f542f8d2..1dfb1e57 100644 --- a/spec/context_spec.cr +++ b/spec/context_spec.cr @@ -129,4 +129,19 @@ describe "Context" do context.params.url["id"].should eq "42" end end + + context "url params cache isolation" do + it "does not mutate cached radix params across repeated requests" do + get "/files/:path" do |env| + env.params.url["path"] + end + + path = "/%252e%252e%252fsecret.txt" + first = call_request_on_app(HTTP::Request.new("GET", "/files#{path}")) + second = call_request_on_app(HTTP::Request.new("GET", "/files#{path}")) + + first.body.should eq("%2e%2e%2fsecret.txt") + second.body.should eq("%2e%2e%2fsecret.txt") + end + end end diff --git a/spec/event_stream_spec.cr b/spec/event_stream_spec.cr index e2230419..d8195dfa 100644 --- a/spec/event_stream_spec.cr +++ b/spec/event_stream_spec.cr @@ -35,6 +35,47 @@ describe Kemal::EventStream do client_response.body.should eq("data: line one\ndata: line two\n\n") end + it "rejects LF, CR, and CRLF in event names" do + {"tick\ndata: x", "tick\rdata: x", "tick\r\ndata: x"}.each do |event| + io = IO::Memory.new + stream = Kemal::EventStream.new(HTTP::Server::Response.new(io)) + expect_raises(ArgumentError, "SSE event must not contain CR or LF") do + stream.send("hello", event: event) + end + end + end + + it "rejects LF, CR, and CRLF in ids" do + {"7\nevent: x", "7\revent: x", "7\r\nevent: x"}.each do |id| + io = IO::Memory.new + stream = Kemal::EventStream.new(HTTP::Server::Response.new(io)) + expect_raises(ArgumentError, "SSE id must not contain CR or LF") do + stream.send("hello", id: id) + end + end + end + + it "treats bare CR in data as a line break to prevent field smuggling" do + sse "/events" do |stream, _| + stream.send("safe\rdata: smuggled-via-CR") + end + + request = HTTP::Request.new("GET", "/events") + client_response = call_request_on_app(request) + client_response.body.should eq("data: safe\ndata: data: smuggled-via-CR\n\n") + client_response.body.should_not contain("\r") + end + + it "treats CRLF in data as a line break to prevent field smuggling" do + sse "/events" do |stream, _| + stream.send("safe\r\nevent: injected\r\ndata: owned") + end + + request = HTTP::Request.new("GET", "/events") + client_response = call_request_on_app(request) + client_response.body.should eq("data: safe\ndata: event: injected\ndata: data: owned\n\n") + end + it "sends keep-alive comments" do sse "/events" do |stream, _| stream.comment("ping") @@ -45,6 +86,16 @@ describe Kemal::EventStream do client_response.body.should eq(": ping\n\n") end + it "splits multi-line comments to prevent SSE injection" do + sse "/events" do |stream, _| + stream.comment("ping\nevent: injected\ndata: owned") + end + + request = HTTP::Request.new("GET", "/events") + client_response = call_request_on_app(request) + client_response.body.should eq(": ping\n: event: injected\n: data: owned\n\n") + end + it "supports url parameters" do sse "/events/:channel" do |stream, env| stream.send(env.params.url["channel"]) diff --git a/src/kemal/event_stream.cr b/src/kemal/event_stream.cr index 26a5c701..d510910f 100644 --- a/src/kemal/event_stream.cr +++ b/src/kemal/event_stream.cr @@ -3,6 +3,9 @@ module Kemal # # Sets the required headers and formats events according to the SSE spec. class EventStream + # SSE treats CR, LF, and CRLF as line terminators (WHATWG HTML). + private SSE_LINE_BREAK = /\r\n|\r|\n/ + def initialize(@response : HTTP::Server::Response) setup_headers end @@ -15,11 +18,20 @@ module Kemal end # Sends an SSE event. Multi-line *data* is split into separate `data:` fields. + # *event* and *id* must not contain CR/LF; newlines there raise `ArgumentError` + # so they cannot inject SSE fields. def send(data : String, *, event : String? = nil, id : String | Int? = nil, retry : Time::Span? = nil) : self - @response.puts "event: #{event}" if event - @response.puts "id: #{id}" if id + if event + validate_single_line!("event", event) + @response.puts "event: #{event}" + end + if id + id_value = id.to_s + validate_single_line!("id", id_value) + @response.puts "id: #{id_value}" + end @response.puts "retry: #{retry.total_milliseconds.to_i}" if retry - data.each_line(chomp: true) do |line| + each_sse_line(data) do |line| @response.puts "data: #{line}" end @response.puts @@ -29,7 +41,8 @@ module Kemal # Sends a keep-alive comment (ignored by clients, useful during idle periods). def comment(text : String) : self - @response.print ": #{text}\n\n" + each_sse_line(text) { |line| @response.print ": #{line}\n" } + @response.print "\n" flush self end @@ -42,6 +55,18 @@ module Kemal @response.close end + private def validate_single_line!(field : String, value : String) : Nil + if value.includes?('\n') || value.includes?('\r') + raise ArgumentError.new("SSE #{field} must not contain CR or LF") + end + end + + # Yields each SSE line. Normalizes CR/CRLF only when needed to avoid an extra alloc. + private def each_sse_line(value : String, & : String ->) : Nil + value = value.gsub(SSE_LINE_BREAK, "\n") if value.includes?('\r') + value.each_line(chomp: true) { |line| yield line } + end + private def setup_headers @response.content_type = "text/event-stream; charset=utf-8" @response.headers["Cache-Control"] = "no-cache" diff --git a/src/kemal/param_parser.cr b/src/kemal/param_parser.cr index 25a5017a..c9584a65 100644 --- a/src/kemal/param_parser.cr +++ b/src/kemal/param_parser.cr @@ -63,7 +63,9 @@ module Kemal alias AllParamTypes = String | Int64 | Float64 | Bool | Hash(String, JSON::Any) | Array(JSON::Any)? getter files, all_files - def initialize(@request : HTTP::Request, @url : Hash(String, String) = {} of String => String) + def initialize(@request : HTTP::Request, url : Hash(String, String) = {} of String => String) + # Own a copy so in-place URI decode cannot mutate a shared/cached Radix params hash. + @url = url.dup @query = HTTP::Params.new({} of String => Array(String)) @body = HTTP::Params.new({} of String => Array(String)) @json = {} of String => AllParamTypes @@ -111,7 +113,7 @@ module Kemal # Updates url params (e.g. after request method override). Used by Context#invalidate_route_cache. def update_url_params(new_url : Hash(String, String)) - @url = new_url + @url = new_url.dup @url_parsed = false end