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.

Sign up for GitHub

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

fix: Ignore everything after closing boundary when parsing multi form data #2862

Merged
merged 17 commits into from
Jun 12, 2024
Merged

fix: Ignore everything after closing boundary when parsing multi form data #2862

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
2c06be3
add working test case
seakayone May 22, 2024
f076ed5
break test
seakayone May 22, 2024
c9d7763
wip
seakayone May 22, 2024
0645065
fix bug
seakayone May 22, 2024
202e98b
Merge branch 'main' into fix/ignore-clrf-after-last-multipart-boundary
seakayone May 23, 2024
e7631a0
Merge branch 'main' into fix/ignore-clrf-after-last-multipart-boundary
seakayone May 28, 2024
de68225
Merge branch 'main' into fix/ignore-clrf-after-last-multipart-boundary
seakayone Jun 10, 2024
2c8b223
make boundaryMatches val
seakayone Jun 10, 2024
4fa5684
revert buffer to ChunkBuilder
seakayone Jun 10, 2024
10f288e
buffer is val
seakayone Jun 10, 2024
07edc59
fmt
seakayone Jun 10, 2024
995da2f
fmt
seakayone Jun 10, 2024
106ba2a
Update zio-http/shared/src/main/scala/zio/http/internal/FormState.scala
seakayone Jun 12, 2024
b338c6e
Extract closingBoundaryBytesSize and reset boundaryMatches with a new…
seakayone Jun 12, 2024
669f718
Merge branch 'main' into fix/ignore-clrf-after-last-multipart-boundary
seakayone Jun 12, 2024
736b504
Update zio-http/shared/src/main/scala/zio/http/internal/FormState.scala
seakayone Jun 12, 2024
f1f33ba
Replace Array[Boolean] with single boolean
seakayone Jun 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion zio-http/jvm/src/test/scala/zio/http/FormSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ object FormSpec extends ZIOHttpSpec {
form2 == form,
)
},
test("encoding with custom paramaters [charset]") {
test("encoding with custom parameters [charset]") {

val form = Form(
FormField.textField(
Expand Down Expand Up @@ -144,6 +144,30 @@ object FormSpec extends ZIOHttpSpec {
}

},
test("decoding no lf at the end") {
val body = Chunk.fromArray(
s"""|--(((AaB03x)))${CR}
|Content-Disposition: form-data; name="hocon-data"${CR}
|Content-Type: text/plain${CR}
|${CR}
|foos: []${CR}
|--(((AaB03x)))${CR}
|Content-Disposition: form-data; name="json-data"${CR}
|Content-Type: text/plain${CR}
|${CR}
|{ "bars": [] }${CR}
|--(((AaB03x)))--""".stripMargin.getBytes(),
)

val form = Form(
FormField.textField("hocon-data", "foos: []", MediaType.text.`plain`),
FormField.textField("json-data", """{ "bars": [] }""", MediaType.text.`plain`),
)

Form
.fromMultipartBytes(body)
.map(form2 => assertTrue(form2 == form))
},
)

val multiFormStreamingSuite: Spec[Any, Throwable] =
Expand Down
28 changes: 25 additions & 3 deletions zio-http/shared/src/main/scala/zio/http/internal/FormState.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,11 @@ private[http] object FormState {

final class FormStateBuffer(boundary: Boundary) extends FormState { self =>

private val tree0: ChunkBuilder[FormAST] = ChunkBuilder.make[FormAST]()
private val buffer: ChunkBuilder.Byte = new ChunkBuilder.Byte
private val tree0: ChunkBuilder[FormAST] = ChunkBuilder.make[FormAST]()
private val buffer: ChunkBuilder[Byte] = new ChunkBuilder.Byte
private var bufferSize: Int = 0
seakayone marked this conversation as resolved.
Show resolved Hide resolved
private val closingBoundaryBytesSize = boundary.closingBoundaryBytes.size
private var boundaryMatches: Array[Boolean] = new Array[Boolean](closingBoundaryBytesSize)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is something here that I don't understand / failing to see. What is the reason for using an array? Can't this be just a boolean and then L62-68 become this?

boundaryMatches &&= posInLine < closingBoundaryBytesSize && boundary.closingBoundaryBytes(posInLine) == byte

val boundaryDetected = boundaryMatches && posInLine == closingBoundaryBytesSize - 1

Copy link
Contributor Author

@seakayone seakayone Jun 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful, you are right we do not need to remember all checked bytes this way.

That works => f1f33ba

Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh no idea why i thought we need an array 🙈 so much better without - nice find @kyri-petrou 🎉


private var lastByte: OptionalByte = OptionalByte.None
private var isBufferEmpty = true
Expand All @@ -54,8 +57,20 @@ private[http] object FormState {

val crlf = byte == '\n' && !lastByte.isEmpty && lastByte.get == '\r'

var boundaryDetected = false;
val posInLine = bufferSize + (if (this.lastByte.isEmpty) 0 else 1)
if (posInLine < closingBoundaryBytesSize) {
boundaryMatches.update(posInLine, boundary.closingBoundaryBytes(posInLine) == byte)
}

if (posInLine == closingBoundaryBytesSize - 1) {
boundaryDetected = boundaryMatches.forall(_ == true)
}

def flush(ast: FormAST): Unit = {
buffer.clear()
bufferSize = 0
boundaryMatches = new Array[Boolean](closingBoundaryBytesSize)
lastByte = OptionalByte.None
if (crlf && isBufferEmpty && (phase eq Phase.Part1)) phase0 = Phase.Part2
if (ast.isContent && dropContents) () else addToTree(ast)
Expand All @@ -72,7 +87,11 @@ private[http] object FormState {
case ClosingBoundary(_) => BoundaryClosed(tree)
}

} else if (crlf && (phase eq Phase.Part2)) {
} else if ((crlf || boundaryDetected) && (phase eq Phase.Part2)) {
if (boundaryDetected) {
buffer += '-'
buffer += '-'
}
val ast = FormAST.makePart2(buffer.result(), boundary)

ast match {
Expand All @@ -87,6 +106,7 @@ private[http] object FormState {
if (!lastByte.isEmpty) {
if (isBufferEmpty) isBufferEmpty = false
buffer += lastByte.get
bufferSize += 1
}
lastByte = OptionalByte.Some(byte)
self
Expand All @@ -102,6 +122,8 @@ private[http] object FormState {
def reset(): Unit = {
tree0.clear()
buffer.clear()
bufferSize = 0
boundaryMatches = new Array[Boolean](closingBoundaryBytesSize)
isBufferEmpty = true
dropContents = false
phase0 = Phase.Part1
Expand Down
Loading