raboof commented on code in PR #744:
URL: https://github.com/apache/pekko-http/pull/744#discussion_r2362547116
##########
http/src/main/scala/org/apache/pekko/http/scaladsl/unmarshalling/sse/ServerSentEventParser.scala:
##########
@@ -110,18 +120,57 @@ private final class ServerSentEventParser(
import shape._
private val builder = new Builder()
+ private lazy val log = Logging(materializer.system,
classOf[ServerSentEventParser])
+ @volatile private var shouldSkipUntilEventEnd = false
setHandlers(in, out, this)
- override def onPush() = {
+ override def onPush(): Unit = {
val line = grab(in)
- if (line == "") { // An event is terminated with a new line
- if (builder.hasData) // Events without data are ignored according to
the spec
- push(out, builder.build())
- else
- pull(in)
+ if (shouldSkipUntilEventEnd) { // Max event size was previously
reached. Skip successive lines until event ends
+ if (line.isEmpty) shouldSkipUntilEventEnd = false // Stop skipping
when end of event (empty line) is reached
+ pull(in) // Already reported oversized event (below). Drop and
continue to next line.
+ } else if (maxEventSize > 0 && builder.size + line.length >
maxEventSize) { // Next line exceeds the size limit
+ shouldSkipUntilEventEnd = true
+ oversizedStrategy match {
+ case OversizedSseStrategy.FailStream =>
+ builder.appendData(line)
+ val event = builder.build()
+ failStage(new IllegalStateException(
+ s"Oversized SSE Event ${event.id.fold("") { id => s"at ID: $id
" }}" +
Review Comment:
The scenario would be an application that connects to 'untrusted' SSE
servers and exposes the result somewhere, including showing an error based on
the exception. As a 'defense in depth' mechanism, it would be good to avoid
including attacker-controllable input in that error, but it would be good for
diagnostics if it's possible to have it in the logs. For that reason we have
exceptions in the `ExceptionWithErrorInfo` hierarchy, which allow splitting the
message into `summary` and 'details', where only the 'summary' is exposed by
default, and 'details' can contain additional (possibly attacker-controlled)
details. This is briefly documented at
https://github.com/apache/pekko-http/blob/main/http-core/src/main/scala/org/apache/pekko/http/scaladsl/model/ErrorInfo.scala#L25
and
https://pekko.apache.org/docs/pekko-http/current/routing-dsl/exception-handling.html#including-sensitive-data-in-exceptions
> The choice of failing the stage with an `IllegalStateException` is
actually just preserving what the code used to do previously and keeps the
default behavior (and exception type) unless the user opts into one of the new
choices. I'm happy to change it though if there's clear guidance.
That makes sense (though the original exception didn't include the id...)
I guess perhaps it's somewhat far-fetched in this case, I'm ok with a
'traditional' exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
