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

Conversation

Projects
None yet
3 participants
@sigod
Contributor

sigod commented Aug 13, 2015

s-ludwig added a commit that referenced this pull request Aug 13, 2015

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 13, 2015

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.

Member

s-ludwig commented Aug 13, 2015

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

Merge pull request #1221 from sigod/patch-1
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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sigod sigod deleted the sigod:patch-1 branch Aug 13, 2015

@sigod

This comment has been minimized.

Show comment
Hide comment
@sigod

sigod Aug 13, 2015

Contributor

I think it's a good idea.

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

Contributor

sigod commented Aug 13, 2015

I think it's a good idea.

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

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 15, 2015

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.

Member

s-ludwig commented Aug 15, 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'll add that and create an alpha tag for 0.7.25.

@sigod

This comment has been minimized.

Show comment
Hide comment
@sigod

sigod Aug 17, 2015

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 22, 2015

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.

Member

s-ludwig commented Aug 22, 2015

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

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Aug 22, 2015

Contributor

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.

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

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 23, 2015

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.

Member

s-ludwig commented Aug 23, 2015

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

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Aug 23, 2015

Contributor

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

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