Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Event::json_data can construct an invalid SSE event by passing a serde_json::value::RawValue #2981

Closed
1 task done
Threated opened this issue Oct 12, 2024 · 2 comments · Fixed by #2992
Closed
1 task done

Comments

@Threated
Copy link
Contributor

Threated commented Oct 12, 2024

  • I have looked for existing issues (including closed) about this

Bug Report

When using Event::json_data with a serde_json::value::RawValue it is possible to construct an invalid SSE event.
This happens because RawValue will also capture newlines between object entries as it only validates that the bytes are valid json.
This means that the string "{\n\"foo\":\"bar\"}" when deserialized into a RawValue and serialized with Event::json_data will not be parsed correctly as internally it gets written to the buffer using serde_json::to_writer.

Reproduction

Looking at the output of dbg! of the bytes should be enough to see the issue but I thought I would verify it again using a SSE parsing lib.

//! ```cargo
//! [dependencies]
//! axum = "0.7"
//! eventsource-stream = "0.2"
//! serde_json = { version = "1", features = ["raw_value"]}
//! futures = "0.3"
//! ```

use axum::response::{sse::Event, IntoResponse, Sse};
use eventsource_stream::Eventsource;
use futures::TryStreamExt;
use serde_json::value::RawValue;
use std::error::Error;

fn main() -> Result<(), Box<dyn Error>> {
    let data = "{\n\"foo\":\"bar\"}";
    let var_name = serde_json::from_str::<Box<RawValue>>(data)?;
    let valid_event = Event::default().data(var_name.get());
    let invalid_event = Event::default().json_data(&var_name)?;
    let stream = Sse::new(futures::stream::iter([
        Ok::<_, std::convert::Infallible>(valid_event),
        Ok(invalid_event),
    ]))
    .into_response()
    .into_body()
    .into_data_stream()
    .inspect_ok(|b| {
        dbg!(String::from_utf8_lossy(b.as_ref()));
    })
    .eventsource();
    let events = futures::executor::block_on(stream.try_collect::<Vec<_>>())?;
    for e in events {
        assert_eq!(
            serde_json::from_str::<serde_json::Value>(&dbg!(e.data))?,
            serde_json::from_str::<serde_json::Value>(data)?
        );
    }
    Ok(())
}

Solutions

  1. Document that all use (also nested) of serde_json::value::RawValue could lead to invalid Events (easy)
  2. Internally use serde_json::to_string instead and than call to Event::data which will split the newlines correctly into multiple data fields. (Slow)
  3. Continue using serde_json::to_writer but wrap buffer in some custom writer which will skip newlines which should be fine as newlines are guaranteed to be escaped in json strings. (should be pretty fast still)

I would be interested in making a PR for 3. if that is the desired solution.

@jplatte jplatte changed the title Event::json_data can construct invalid an invalid SSE event by passing a serde_json::value::RawValue Event::json_data can construct an invalid SSE event by passing a serde_json::value::RawValue Oct 14, 2024
@jplatte
Copy link
Member

jplatte commented Oct 14, 2024

I don't really understand the second solution.
The third one sounds reasonable to me.

@SabrinaJewson do you have an opinion on this?

@SabrinaJewson
Copy link
Contributor

Yeah, third solution sounds reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants