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

Handle malformed http requests #5041

Merged
merged 6 commits into from Sep 28, 2017

Conversation

n7olkachev
Copy link
Contributor

@n7olkachev n7olkachev commented Sep 26, 2017

Fixes #4878. ArgumentError is thrown while parsing, so it is better to catch it and return BadRequest.

@n7olkachev n7olkachev force-pushed the bug/4878-malformed-http-headers branch from 116894d to dae71c6 Compare Sep 26, 2017
return new method, resource, headers, body, http_version
begin
HTTP.parse_headers_and_body(io) do |headers, body|
return new method, resource, headers, body, http_version
Copy link
Contributor

@akzhan akzhan Sep 26, 2017

Choose a reason for hiding this comment

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

I'm unsure that you need to keep this code organization with added rescue block.

Why not something like

  begin
    HTTP.parse_headers_and_body(io) do |headers, body|
      new method, resource, headers, body, http_version
    end
  rescue ex : ArgumentError
    BadRequest.new
  end

Copy link
Contributor

@RX14 RX14 Sep 26, 2017

Choose a reason for hiding this comment

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

@akzhan that doesn't work, there's a case where parse_headers_and_body returns without yielding which isnt handled.

HTTP.parse_headers_and_body(io) do |headers, body|
return new method, resource, headers, body, http_version
end
rescue ex : ArgumentError
Copy link
Contributor

@RX14 RX14 Sep 26, 2017

Choose a reason for hiding this comment

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

I'm not entirely sure if this is the correct way to detect this. It feels to broad. I'm not sure what would be a better approach though.

Copy link
Contributor Author

@n7olkachev n7olkachev Sep 27, 2017

Choose a reason for hiding this comment

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

@RX14 To clarify: are you thinking about more exceptions which could be thrown and would be handled like this?

Copy link
Contributor

@RX14 RX14 Sep 27, 2017

Choose a reason for hiding this comment

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

Perhaps non-ArgumentError exceptions would be thrown, perhaps new would throw an exception. This PR is a great improvement but perhaps we could implement it a better way.

Copy link
Member

@jhass jhass Sep 27, 2017

Choose a reason for hiding this comment

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

The best way probably would be not rescuing the exception but handling the case that triggers it explicitly/differently.

Copy link
Contributor Author

@n7olkachev n7olkachev Sep 27, 2017

Choose a reason for hiding this comment

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

@jhass @RX14 updated, now it is handled explicitly in parse_headers_and_body while adding header. Ideas?

Copy link
Member

@asterite asterite Sep 27, 2017

Choose a reason for hiding this comment

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

Ideally no exception should be raised nor rescued. We should maybe have a method to add a header and return false or nil if it's not valid, instead of raising.

Copy link
Member

@jhass jhass Sep 27, 2017

Choose a reason for hiding this comment

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

Also some tests would be awesome :)

akzhan
akzhan approved these changes Sep 27, 2017
@n7olkachev
Copy link
Contributor Author

@n7olkachev n7olkachev commented Sep 27, 2017

Pushed next version. I've added valid_value? method to Headers, so we can handle malformed content without exceptions.

@@ -59,6 +59,7 @@ module HTTP
end

name, value = parse_header(line)
break unless headers.valid_value?(value)
headers.add(name, value)
Copy link
Contributor

@Sija Sija Sep 27, 2017

Choose a reason for hiding this comment

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

I'd suggest headers.add(name, value) if headers.valid_value?(value)

Copy link
Member

@asterite asterite Sep 27, 2017

Choose a reason for hiding this comment

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

This checks the validity twice. I suggest adding add? method that returns false if it's not valid, true otherwise.

@n7olkachev
Copy link
Contributor Author

@n7olkachev n7olkachev commented Sep 28, 2017

Added add? methods

self
end

def add?(key, value : String)
add?(key, [value])
Copy link
Member

@asterite asterite Sep 28, 2017

Choose a reason for hiding this comment

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

This allocates an array. The same for add(key, String). I prefer it not to allocate (like before, have different implementations of the method)

Copy link
Member

@jhass jhass Sep 28, 2017

Choose a reason for hiding this comment

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

Or maybe we can just allow to pass a tuple here?

Copy link
Contributor Author

@n7olkachev n7olkachev Sep 28, 2017

Choose a reason for hiding this comment

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

@asterite updated.
Any other suggestions?

RX14
RX14 approved these changes Sep 28, 2017
@RX14 RX14 added this to the Next milestone Sep 28, 2017
@RX14 RX14 merged commit b248857 into crystal-lang:master Sep 28, 2017
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants