Validate request size against global or endpoint provided max request size #28

rabeyta · 2017-03-10T22:57:47Z

issue #27

This PR will add the functionality of validating each request against a globally configured request size.

By default, endpoints do not override and will default to the global.

You can override the global configuration inside each endpoint.

ProxyRouterEndpoints have this validation disabled by default

codecov-io · 2017-03-10T23:03:57Z

Codecov Report

Merging #28 into master will increase coverage by 0.01%.
The diff coverage is 95.45%.

@@             Coverage Diff             @@
##             master     #28      +/-   ##
===========================================
+ Coverage     89.79%   89.8%   +0.01%     
- Complexity     1823    1832       +9     
===========================================
  Files           137     137              
  Lines          5341    5357      +16     
  Branches        700     703       +3     
===========================================
+ Hits           4796    4811      +15     
  Misses          368     368              
- Partials        177     178       +1

Impacted Files	Coverage Δ	Complexity Δ
...java/com/nike/riposte/server/http/RequestInfo.java	`100% <ø> (ø)`	`7 <0> (ø)`	❌
...server/channelpipeline/HttpChannelInitializer.java	`100% <100%> (ø)`	`29 <0> (ø)`	❌
.../nike/riposte/server/http/ProxyRouterEndpoint.java	`82.75% <100%> (+0.61%)`	`7 <1> (+1)`	✅
...in/java/com/nike/riposte/server/http/Endpoint.java	`100% <100%> (ø)`	`9 <1> (+1)`	✅
...a/com/nike/riposte/server/config/ServerConfig.java	`95.34% <100%> (ø)`	`34 <1> (ø)`	❌
...nike/riposte/server/http/impl/RequestInfoImpl.java	`100% <100%> (ø)`	`64 <0> (ø)`	❌
...ckstopperRiposteFrameworkErrorHandlerListener.java	`100% <100%> (ø)`	`25 <0> (+1)`	✅
...poste/server/handler/RequestInfoSetterHandler.java	`88.88% <91.66%> (+12.41%)`	`13 <5> (+7)`	✅
.../com/nike/riposte/server/logging/AccessLogger.java	`94.85% <0%> (-1.48%)`	`46% <0%> (-1%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fe765a...dfef3e7. Read the comment docs.

nicmunroe · 2017-03-13T18:21:39Z

riposte-core/src/main/java/com/nike/riposte/server/handler/RequestInfoSetterHandler.java

@@ -21,10 +24,19 @@
 * This handler should come after {@link io.netty.handler.codec.http.HttpRequestDecoder} and {@link
 * SmartHttpContentCompressor} in the pipeline.
 *
+ * The request size is tracked and if it exceeds the configured global or a given endpoint's override, and exception


Minor typo - "and exception" instead of "an exception".

good eyes. fixed.

nicmunroe · 2017-03-13T18:26:45Z

riposte-core/src/main/java/com/nike/riposte/server/handler/RequestInfoSetterHandler.java

+
+    private int getConfiguredMaxRequestSize(HttpProcessingState state) {
+        int maxConfiguredRequestSize = maxRequestSizeInBytes;
+        Optional<Integer> endpointMaxRequestSizeOverride = state.getEndpointForExecution().maxRequestSizeInBytesOverride();


state.getEndpointForExecution() can return null depending on the state of the request. A null endpoint should be considered the same as an empty optional, i.e. use the global default.

Now that I think about it the endpoint could also return null. It would be a bad/broken endpoint but we still shouldn't NPE if someone does it. If null is returned from an endpoint's maxRequestSizeInBytesOverride() it should also be treated as an empty optional (i.e. use global default).

After some more thinking about all this, here are some general refactors that I think should be done:

The max request size in bytes (MRSB) was originally an int because once upon a time we used Netty's HttpObjectAggregator, and that handler did the MRSB check and required an int. We no longer have that restriction, so we should use a long instead to get rid of the 2GB ceiling on MRSB.

Since we have to check for null on the endpoint override anyway even with Optional, and the core stuff tends to use nulls rather than optionals to avoid the perf hit of optionals, the Endpoint.maxRequestSizeInBytesOverride() method signature should probably change to just return a Long object. Returning null from the override method would mean "use the global default", and returning anything non-null would mean "use this value instead of the global default".

thanks for the details. I'll update the logic where it handles the endpoint or the endpoint's override as null and will return the global default.

Never mind on moving stuff to longs. As you pointed out, we're ultimately limited by the fact that byte[], ByteBuf, etc lengths are ints. So without some major refactoring to allow for exposing payloads as InputStreams or something similar we're stuck with an int limit.

nicmunroe · 2017-03-13T18:27:17Z

riposte-core/src/main/java/com/nike/riposte/server/handler/RequestInfoSetterHandler.java

 * @author Nic Munroe
 */
 public class RequestInfoSetterHandler extends BaseInboundHandlerWithTracingAndMdcSupport {

+    private final int maxRequestSizeInBytes;


Suggest renaming to defaultMaxRequestSizeInBytes or something else that indicates it's the default/global.

will do: globalConfiguredMaxRequestSizeInBytes ?

nicmunroe · 2017-03-13T18:31:14Z

...est/java/com/nike/riposte/server/componenttest/VerifyRequestSizeValidationComponentTest.java

+                        .extract();
+
+        assertThat(response.statusCode()).isEqualTo(HttpResponseStatus.BAD_REQUEST.code());
+        assertThat(response.asString()).contains("Malformed request");


Also look for the metadata here (and other places where we're verifying that the error was thrown).

good call. i've added an assert for message and cause

nicmunroe · 2017-03-13T18:36:42Z

...est/java/com/nike/riposte/server/componenttest/VerifyRequestSizeValidationComponentTest.java

+                        .port(serverConfig.endpointsPort())
+                        .basePath(BasicEndpoint.MATCHING_PATH)
+                        .log().all()
+                        .body("{'key1':'value1'}")


I'd suggest that the global and endpoint-override max request size values be constants that can be referenced by these tests, and dynamically generate a request of the appropriate size in bytes relative to whatever constant you're testing against.

e.g. for this test:

.body(generatePayloadOfSizeInBytes(GLOBAL_MAX_REQUEST_SIZE + 1))

The dynamically generated payload doesn't have to be valid JSON - it can just be random characters, or all the same character, or whatever you want to do to make it easy - the only thing that matters is the size of the payload that's returned.

good suggestion. I've incorporated this pattern in here.

nicmunroe · 2017-03-13T18:48:44Z

riposte-core/src/test/java/com/nike/riposte/server/handler/RequestInfoSetterHandlerTest.java

+
+        // when
+        try {
+            handler.doChannelRead(ctxMock, httpContent);


AssertJ has a convenience method for all this:

Throwable thrownException = Assertions.catchThrowable(() -> handler.doChannelRead(ctxMock, httpContent));

Then you'd assert something on the exception which implicitly verifies the exception was thrown since if the assertion was executed on null it would also fail. The existing assertion in this test is sufficient, for example.

thanks for the tip. I've converted them over.

nicmunroe · 2017-03-13T18:51:39Z

riposte-core/src/test/java/com/nike/riposte/server/handler/RequestInfoSetterHandlerTest.java

+    @Test
+    public void doChannelRead_allows_endpoints_to_override_max_request_size_setting_lower() {
+        // given
+        //handler is configured for 10


Reference maxRequestSizeInBytes in the tests so you don't have to put these comments here and also removes the potential for accidental test breakage or worse, accidental regression in that the tests don't catch.

good call. I've moved this down in the tests so it is clearer the state of the test in the 'given'

nicmunroe · 2017-03-13T18:53:22Z

riposte-spi/src/main/java/com/nike/riposte/server/http/Endpoint.java

+    /**
+     * @return The max request size in bytes that you want to allow for this specific endpoint, or Optional.empty()
+     * if you want to use the app-wide default max request size
+     * returned by {@link ServerConfig#maxRequestSizeInBytes()} ()}.


Trailing ()} that needs to be deleted. Also, the javadoc should include something about returning 0 to disable request size validation entirely.

nicmunroe · 2017-03-13T21:29:15Z

...est/java/com/nike/riposte/server/componenttest/VerifyRequestSizeValidationComponentTest.java

+    private static String generatePayloadOfSizeInBytes(int length) {
+        StringBuilder sb = new StringBuilder(length);
+        for(int i = 0; i < length; i++) {
+            sb.append(i);


For anything i >= 11 you'll be getting a string larger than the requested length in bytes. You could do sb.append(i % 10).

nice catch.

nicmunroe · 2017-03-13T21:31:53Z

...est/java/com/nike/riposte/server/componenttest/VerifyRequestSizeValidationComponentTest.java

+                        .port(serverConfig.endpointsPort())
+                        .basePath(BasicEndpoint.MATCHING_PATH)
+                        .log().all()
+                        .body(generatePayloadOfSizeInBytes(GLOBAL_MAX_REQUEST_SIZE - 1))


Get rid of the - 1 I think?

good call. its unnecessary. removed!

nicmunroe · 2017-03-13T21:32:40Z

riposte-core/src/test/java/com/nike/riposte/server/handler/RequestInfoSetterHandlerTest.java

@@ -93,4 +119,102 @@ public void doChannelRead_checks_for_fully_send_responses_but_does_nothing_else_
        assertThat(result).isEqualTo(PipelineContinuationBehavior.CONTINUE);
    }

+    @Test
+    public void doChannelRead_returns_do_not_fire_when_null_state() {


Thanks for adding these missing tests!

nicmunroe · 2017-03-13T21:34:14Z

riposte-spi/src/main/java/com/nike/riposte/server/config/ServerConfig.java

@@ -390,12 +390,11 @@ default int maxOpenIncomingServerChannels() {
    /**
     * @return The maximum allowed request size in bytes. If netty receives a request larger than this then it will
     * throw a {@link io.netty.handler.codec.TooLongFrameException}.
-     * This value is an integer, so the max you can set it to is {@link Integer#MAX_VALUE}, which corresponds to 2^31-1,


I would leave the comment about max size in here (but since you're fixing the NOTE that line should obviously go).

i've added it back.

nicmunroe · 2017-03-13T21:34:48Z

riposte-spi/src/main/java/com/nike/riposte/server/http/Endpoint.java

@@ -6,6 +6,8 @@
 import com.fasterxml.jackson.core.type.TypeReference;
 import com.fasterxml.jackson.databind.ObjectMapper;

+import java.util.Optional;


Optional import no longer needed.

i found the other, but missed this one!! Sorry about that.

…able this to be disabled or overridden on an endpoint by endpoint basis

nicmunroe · 2017-03-13T22:02:47Z

Looks good, thank you for the PR!

rabeyta changed the title ~~Validate request against global or endpoint provided max request size~~ Validate request size against global or endpoint provided max request size Mar 10, 2017

rabeyta force-pushed the issue27 branch from bb50055 to 08bd4bd Compare March 11, 2017 16:46

nicmunroe reviewed Mar 13, 2017

View reviewed changes

rabeyta force-pushed the issue27 branch 2 times, most recently from 3f741cf to e9ae5ea Compare March 13, 2017 20:54

nicmunroe reviewed Mar 13, 2017

View reviewed changes

honor configured max request size inside RequestInfoSetterHandler. En…

dfef3e7

…able this to be disabled or overridden on an endpoint by endpoint basis

rabeyta force-pushed the issue27 branch from e9ae5ea to dfef3e7 Compare March 13, 2017 21:53

nicmunroe merged commit d89f801 into Nike-Inc:master Mar 13, 2017

rabeyta deleted the issue27 branch May 31, 2017 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate request size against global or endpoint provided max request size #28

Validate request size against global or endpoint provided max request size #28

rabeyta commented Mar 10, 2017

codecov-io commented Mar 10, 2017 •

edited

Loading

nicmunroe Mar 13, 2017

rabeyta Mar 13, 2017

nicmunroe Mar 13, 2017

nicmunroe Mar 13, 2017

nicmunroe Mar 13, 2017

rabeyta Mar 13, 2017

nicmunroe Mar 13, 2017

nicmunroe Mar 13, 2017

rabeyta Mar 13, 2017

nicmunroe Mar 13, 2017

nicmunroe Mar 13, 2017

rabeyta Mar 13, 2017

nicmunroe Mar 13, 2017 •

edited

Loading

rabeyta Mar 13, 2017

nicmunroe Mar 13, 2017 •

edited

Loading

rabeyta Mar 13, 2017

nicmunroe Mar 13, 2017 •

edited

Loading

rabeyta Mar 13, 2017

nicmunroe Mar 13, 2017

nicmunroe Mar 13, 2017 •

edited

Loading

rabeyta Mar 13, 2017

nicmunroe Mar 13, 2017

rabeyta Mar 13, 2017

nicmunroe Mar 13, 2017

nicmunroe Mar 13, 2017 •

edited

Loading

rabeyta Mar 13, 2017

nicmunroe Mar 13, 2017

rabeyta Mar 13, 2017

nicmunroe commented Mar 13, 2017

Validate request size against global or endpoint provided max request size #28

Validate request size against global or endpoint provided max request size #28

Conversation

rabeyta commented Mar 10, 2017

codecov-io commented Mar 10, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicmunroe Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicmunroe Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicmunroe Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicmunroe Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicmunroe Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicmunroe commented Mar 13, 2017

codecov-io commented Mar 10, 2017 •

edited

Loading

nicmunroe Mar 13, 2017 •

edited

Loading

nicmunroe Mar 13, 2017 •

edited

Loading

nicmunroe Mar 13, 2017 •

edited

Loading

nicmunroe Mar 13, 2017 •

edited

Loading

nicmunroe Mar 13, 2017 •

edited

Loading