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

multipart request: always skip boundary, fix #1220 #1221

Merged
merged 1 commit into from
Aug 13, 2015
Merged

multipart request: always skip boundary, fix #1220 #1221

merged 1 commit into from
Aug 13, 2015

Conversation

sigod
Copy link
Contributor

@sigod sigod commented Aug 13, 2015

s-ludwig added a commit that referenced this pull request Aug 13, 2015
@s-ludwig
Copy link
Member

I'd like to eventually add a validation that the readUntil actually reads 0 bytes until it hits the boundary. But that requires either adding a byte counter to readUntil, or using something else to consume the marker. I'll pull this in the meantime to see if it fixes the test mentioned above.

s-ludwig added a commit that referenced this pull request Aug 13, 2015
Always skip the boundary when parsing a multi-part form part. Fixes #1220.
@s-ludwig s-ludwig merged commit d1367fb into vibe-d:master Aug 13, 2015
@sigod sigod deleted the patch-1 branch August 13, 2015 20:48
@sigod
Copy link
Contributor Author

sigod commented Aug 13, 2015

I think it's a good idea.

Something like stream.consumeExpected(cast(ubyte[])boundary);?

@s-ludwig
Copy link
Member

I think I'd go with something like enforceHTTP(stream.trySkip(cast(ubyte)[])boundary), HTTPStatus.badRequest);, so that a proper HTTP status is generated. I'll add that and create an alpha tag for 0.7.25.

@sigod
Copy link
Contributor Author

sigod commented Aug 17, 2015

I think I'd go with something like enforceHTTP(stream.trySkip(cast(ubyte)[])boundary), HTTPStatus.badRequest);, so that a proper HTTP status is generated.

I thought about validation as in readUntil. But, yes, your version is much better. And more reusable, which is very important.

I'll add that and create an alpha tag for 0.7.25.

Why doesn't you use SemVer normally? Just increase patch number and release new version. Because I can't really get this bug fix. I'm stuck with 3 options: use old 0.7.23 or use alpha tag or edit local source of 0.7.24. I like neither of this options.

@s-ludwig
Copy link
Member

Hm, I do intend to tag a new release shortly, but right now it doesn't make sense yet to switch to full SemVer (>=1.0.0), because more or less every release still contains some breaking changes or new features. But the plan is to split up the library into multiple pieces and tag some of them with a stable version shortly afterwards.

But you can safely use the alpha tag for now, as there are no critical changes and I will not introduce any of those until 0.7.25.

@etcimon
Copy link
Contributor

etcimon commented Aug 22, 2015

I'm receiving a EOF before end stream marker on this using the MultiPartPart script with FileMultiPart:

https://github.com/rejectedsoftware/vibe.d/blob/master/source/vibe/inet/webform.d#L262

I don't add CRLF to the end boundary, so I'm wondering if the readLine is erroneous or if I should append CRLF.

@s-ludwig
Copy link
Member

According to RFC 1341 there is no CRLF required after the "--", so it looks like the correct thing to do would be to always read two bytes and require them to be either "--" or "\r\n" in case of "--", it should then nullSink.write(bodyReader); to drop the epilogue.

@etcimon
Copy link
Contributor

etcimon commented Aug 23, 2015

That's what I was assuming, I'll submit a pull request sometime today or tomorrow for this

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 this pull request may close these issues.

3 participants