Make changes feed return bad request for invalid heartbeat values #2270

bessbd · 2019-10-23T15:13:25Z

Overview

Using a negative heartbeat value does not return a 400 bad request, instead getting just an empty response with no status code at all.

This commit adds extra checks so that negative and non-integer heartbeat values return 400 bad request responses.

Testing recommendations

The changes has been tested manually:

[donat:~/couchdb] changes-feed-input-validation 1 ± curl -XPUT http://127.0.0.1:15984/asd
{"ok":true}
[donat:~/couchdb] changes-feed-input-validation ± curl -v http://127.0.0.1:15984/asd/_changes?feed=continuous\&heartbeat=-1000
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to 127.0.0.1 (127.0.0.1) port 15984 (#0)
> GET /asd/_changes?feed=continuous&heartbeat=-1000 HTTP/1.1
> Host: 127.0.0.1:15984
> User-Agent: curl/7.64.1
> Accept: */*
>
< HTTP/1.1 400 Bad Request
< Cache-Control: must-revalidate
< Content-Length: 60
< Content-Type: application/json
< Date: Wed, 23 Oct 2019 15:10:52 GMT
< Server: CouchDB/3.0.0-1dd00d6ac (Erlang OTP/22)
< X-Couch-Request-ID: 61d1b854bd
< X-CouchDB-Body-Time: 0
<
{"error":"bad_request","reason":"Negative heartbeat value"}
* Connection #0 to host 127.0.0.1 left intact
* Closing connection 0
[donat:~/couchdb] changes-feed-input-validation ± curl -v http://127.0.0.1:15984/asd/_changes?feed=continuous\&heartbeat=a1000
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to 127.0.0.1 (127.0.0.1) port 15984 (#0)
> GET /asd/_changes?feed=continuous&heartbeat=a1000 HTTP/1.1
> Host: 127.0.0.1:15984
> User-Agent: curl/7.64.1
> Accept: */*
>
< HTTP/1.1 400 Bad Request
< Cache-Control: must-revalidate
< Content-Length: 59
< Content-Type: application/json
< Date: Wed, 23 Oct 2019 15:11:01 GMT
< Server: CouchDB/3.0.0-1dd00d6ac (Erlang OTP/22)
< X-Couch-Request-ID: 4e9ae14ede
< X-CouchDB-Body-Time: 0
<
{"error":"bad_request","reason":"Invalid heartbeat value"}
* Connection #0 to host 127.0.0.1 left intact
* Closing connection 0

Also, there are new test cases added for the new behavior.

Related Issues or Pull Requests

This fixes #2234

Checklist

Code is written and works correctly
Changes are covered by tests
Any new configurable parameters are documented in rel/overlay/etc/default.ini
A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation

iilyak

Thank you for your contribution @bessbd.
Would you mind writing a test case for it.
You can use any of the available test suites:

exunit : see examples
- here https://github.com/apache/couchdb/blob/master/src/couch/test/exunit/fabric_test.exs
- and here https://github.com/cloudant/couchdb/blob/opentracing/src/chttpd/test/exunit/ctrace_spans_test.exs
eunit :
- add it here https://github.com/apache/couchdb/blob/master/src/couch/test/eunit/couch_changes_tests.erl
exunit integration suite :
- see examples in the https://github.com/apache/couchdb/blob/master/test/elixir/test/ directory

iilyak · 2019-10-23T15:27:33Z

src/chttpd/src/chttpd_db.erl

-            Args#changes_args{heartbeat=list_to_integer(Value)};
+            try list_to_integer(Value) of
+                V when V > 0 ->
+                    Args#changes_args{heartbeat=heartbeat_integer};


I don't think heartbeat_integer is correct.

Args#changes_args{heartbeat=heartbeat_integer};

Good call! I've just pushed a commit to fix this.

iilyak · 2019-10-23T15:42:28Z

@iilyak Would you mind writing a test case for it.

I am so sorry I missed the fact that you have some tests

iilyak · 2019-10-23T16:03:27Z

src/chttpd/src/chttpd_db.erl

+                HeartbeatInteger when HeartbeatInteger > 0 ->
+                    Args#changes_args{heartbeat=HeartbeatInteger};
+                _ ->
+                    throw({bad_request, <<"Negative heartbeat value">>})


The "Negative heartbeat value" doesn't sound correct if user pass 0. Should we change the wording to something like:

The heartbeat value should be positive integer (in milliseconds)

iilyak · 2019-10-23T16:05:35Z

src/chttpd/src/chttpd_db.erl

+                _ ->
+                    throw({bad_request, <<"Negative heartbeat value">>})
+            catch error:badarg ->
+                throw({bad_request, <<"Invalid heartbeat value">>})


It would be better to give user a hint. Something like:

<<"Invalid heartbeat value. Expecting positive integer value (in milliseconds).">>

bessbd · 2019-10-23T16:18:17Z

@iilyak : thank you for the review! I've just pushed a commit (cabe4c4) to make the changes you've suggested.

iilyak

+1 conditional to CI pass

iilyak · 2019-10-23T16:21:21Z

@bessbd Could you squash commits.

Using a negative heartbeat value does not return a 400 bad request, instead getting just an empty response with no status code at all. This commit adds extra checks so that negative and non-integer heartbeat values return 400 bad request responses. This fixes apache#2234

bessbd · 2019-10-23T16:35:54Z

@bessbd Could you squash commits.

Done!

iilyak · 2019-10-23T18:33:39Z

Thank you for your contribution.

bessbd · 2019-10-24T12:00:14Z

Thank you for the review and the merge, @iilyak !

iilyak requested changes Oct 23, 2019

View reviewed changes

iilyak reviewed Oct 23, 2019

View reviewed changes

iilyak approved these changes Oct 23, 2019

View reviewed changes

bessbd force-pushed the changes-feed-input-validation branch from cabe4c4 to a47f0fa Compare October 23, 2019 16:35

iilyak merged commit be2364d into apache:master Oct 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make changes feed return bad request for invalid heartbeat values #2270

Make changes feed return bad request for invalid heartbeat values #2270

bessbd commented Oct 23, 2019

iilyak left a comment

iilyak Oct 23, 2019

bessbd Oct 23, 2019

iilyak commented Oct 23, 2019

iilyak Oct 23, 2019 •

edited

iilyak Oct 23, 2019

bessbd commented Oct 23, 2019

iilyak left a comment

iilyak commented Oct 23, 2019

bessbd commented Oct 23, 2019

iilyak commented Oct 23, 2019

bessbd commented Oct 24, 2019

Make changes feed return bad request for invalid heartbeat values #2270

Make changes feed return bad request for invalid heartbeat values #2270

Conversation

bessbd commented Oct 23, 2019

Overview

Testing recommendations

Related Issues or Pull Requests

Checklist

iilyak left a comment

Choose a reason for hiding this comment

iilyak Oct 23, 2019

Choose a reason for hiding this comment

bessbd Oct 23, 2019

Choose a reason for hiding this comment

iilyak commented Oct 23, 2019

iilyak Oct 23, 2019 • edited

Choose a reason for hiding this comment

iilyak Oct 23, 2019

Choose a reason for hiding this comment

bessbd commented Oct 23, 2019

iilyak left a comment

Choose a reason for hiding this comment

iilyak commented Oct 23, 2019

bessbd commented Oct 23, 2019

iilyak commented Oct 23, 2019

bessbd commented Oct 24, 2019

iilyak Oct 23, 2019 •

edited