Add the possibility to set the maximum number of header fields #549

marinelli · 2024-12-12T18:21:00Z

This PR adds the possibility to configure the maximum number of HTTP header fields in a message.
This is similar to what it was done for configuring the maximum size of an HTTP header field (#514).

snoyberg

Thanks! Two pieces of feedback:

Can you update the version number in the cabal file too?
It's slightly surprising that the new config value is a Maybe Int but it comes with a default of 100 and doesn't appear to have a way to set it to Nothing. Not a strong comment, but something more consistent might make sense.

marinelli · 2024-12-13T08:31:00Z

Thanks! Two pieces of feedback:

Thanks for having a look :)

Can you update the version number in the cabal file too?

done

It's slightly surprising that the new config value is a Maybe Int but it comes with a default of 100 and doesn't appear to have a way to set it to Nothing. Not a strong comment, but something more consistent might make sense.

I used the same strategy of managerMaxHeaderLength, and for what I understood is the following:

if you want to use set a maximum value, use something like managerMaxNumberHeaders 123 defaultManagerSettings
if you want to set it to Nothing, import Network.HTTP.Client.Internal, and do something like: defaultManagerSettings { managerMaxNumberHeaders = Nothing }.

I think we introduced some extra complexity, not really required.
The MaxHeaderLength and the new MaxNumberHeaders are used in predicates where we compare those max values with a counter, we can already use a negative number to obtain the same behavior of configuring those parameters to Nothing, ie: managerMaxNumberHeaders (- 1) defaultManagerSettings.

The main purpose of configuring those parameters to Nothing would be for testing and debugging (I think), so we could just remove the Maybe, and have something like this: marinelli/http-client@set-max-headers...marinelli:http-client:use-ints.

If we would like to continue to use a Maybe value for those max settings in ManagerSettings, well, I would change it to a Maybe Word.

cc: @tfausak

snoyberg · 2024-12-13T10:00:29Z

I don't remember the reasons behind the max header length decisions. I'm fine with any approach. Even if we went over to Word, using 0 to mean "unlimited" would be reasonable too (since disallowing all headers wouldn't make sense).

@tfausak any opinion from you on which way to go?

tfausak · 2024-12-13T14:32:19Z

I like the idea of removing Maybe. I also think that switching to Word makes sense, but that may require some annoying refactoring.

There may be no need for a special "unlimited" value. You could just set it to maxBound which is effectively unlimited. But also treating 0 as "unlimited" makes sense to me. You can't really do HTTP without any headers, so setting the limit to 0 would be useless.

marinelli · 2024-12-13T14:58:51Z

I like the idea of removing Maybe. I also think that switching to Word makes sense, but that may require some annoying refactoring.

There may be no need for a special "unlimited" value. You could just set it to maxBound which is effectively unlimited. But also treating 0 as "unlimited" makes sense to me. You can't really do HTTP without any headers, so setting the limit to 0 would be useless.

I had a look here https://www.rfc-editor.org/rfc/rfc2616#page-31, and it seems that header fields can also be omitted, so 0 might be a proper value. Yeah, we could use maxBound, but I think also negative numbers are a valid option.
@tfausak I already refactored the code here [1], and I do not think that switching to Words will be so complicated.

We could also use data MaxValue = Unlimited | MaxValue Word :) but I would merge [1] instead of increasing complexity.

[1] marinelli/http-client@set-max-headers...marinelli:http-client:use-ints

tfausak · 2024-12-13T15:03:38Z

Technically the RFC allows zero headers, but practically the Host header is always present for HTTP/1.1 requests. There's also this bit: https://www.rfc-editor.org/rfc/rfc2616#section-9

The Host request-header field (section 14.23) MUST accompany all HTTP/1.1 requests.

marinelli · 2024-12-13T16:10:54Z

Technically the RFC allows zero headers, but practically the Host header is always present for HTTP/1.1 requests. There's also this bit: https://www.rfc-editor.org/rfc/rfc2616#section-9

The Host request-header field (section 14.23) MUST accompany all HTTP/1.1 requests.

I don't know, it's a bit confusing to me to assign to 0 the meaning of unlimited value, I could think to use 0 to test server implementations. Anyway, if we will find that using 0 with that meaning might be confusing, we can just change it in an other PR.

marinelli · 2024-12-13T16:27:54Z

Technically the RFC allows zero headers, but practically the Host header is always present for HTTP/1.1 requests. There's also this bit: https://www.rfc-editor.org/rfc/rfc2616#section-9

The Host request-header field (section 14.23) MUST accompany all HTTP/1.1 requests.

I don't know, it's a bit confusing to me to assign to 0 the meaning of unlimited value, I could think to use 0 to test server implementations. Anyway, if we will find that using 0 with that meaning might be confusing, we can just change it in an other PR.

~~Here it is marinelli#1. It's based on this PR, and I can merge it here.~~ I updated this PR. Let me know what you think.

tfausak

Looks great! I like removing the Maybe from both fields.

tfausak · 2024-12-13T22:14:06Z

http-client/Network/HTTP/Client/Connection.hs

-                case fmap unMaxHeaderLength mhl of
-                    Nothing -> pure ()
-                    Just n -> when (total' > n) $ throwHttp OverlongHeaders
+                when (total >= unMaxHeaderLength mhl && total /= 0) $ do


This needs to compare against the new total, not the old one. And it should be checking to see if the MaxHeaderLength is 0, not the total.

Suggested change

when (total >= unMaxHeaderLength mhl && total /= 0) $ do

let n = unMaxHeaderLength mhl

total' = total + fromIntegral (S.length bs)

when (total' >= n && n /= 0) $ do

I don't know why I made this change :)

See #549 (comment)

tfausak · 2024-12-13T22:14:54Z

http-client/Network/HTTP/Client/Headers.hs

-    parseHeaders 100 _ = throwHttp OverlongHeaders
+    guardMaxNumberHeaders :: Word -> IO ()
+    guardMaxNumberHeaders count =
+        when (count >= unMaxNumberHeaders mnh && count /= 0) $ do


Same deal here:

Suggested change

when (count >= unMaxNumberHeaders mnh && count /= 0) $ do

let n = unMaxNumberHeaders mnh

in when (count >= n && n /= 0) $ do

See #549 (comment)

tfausak · 2024-12-13T22:17:42Z

http-client/Network/HTTP/Client/Types.hs

-    , managerMaxHeaderLength :: Maybe MaxHeaderLength
+    , managerMaxHeaderLength :: MaxHeaderLength
+    -- ^ Configure the maximum size, in bytes, of an HTTP header field.
+    -- Set it to 0 to remove this limit (eg: for debugging purposes).


I don't think it's necessary to use 0 as a sentinel value. maxBound :: Word32 is more than 4 GB, and maxBound :: Word64 is unfathomably large. In other words, using maxBound would effectively disable these checks anyway.

I agree, and this is why I just removed the n /= 0 condition from the previous predicates.

See #549 (comment)

snoyberg

Let's avoid any backwards-incompatible changes to the library, including to the internal modules. We can implement new features using the preferred approach though.

snoyberg · 2024-12-16T06:10:25Z

http-client/Network/HTTP/Client.hs

@@ -322,9 +323,14 @@ managerSetProxy :: ProxyOverride -> ManagerSettings -> ManagerSettings
 managerSetProxy po = managerSetInsecureProxy po . managerSetSecureProxy po

 -- @since 0.7.17
-managerSetMaxHeaderLength :: Int -> ManagerSettings -> ManagerSettings
+managerSetMaxHeaderLength :: Word -> ManagerSettings -> ManagerSettings


This is a breaking change, I do not want to include it. It's not worth the ecosystem churn for a small improvement in type correctness.

You are right, I actually thought about it, and I didn't change it.

But... are you talking about managerSetMaxHeaderLength only? I would just revert to use Word for both managerSetMaxHeaderLength and managerSetMaxNumberHeaders, and also the newtypes used internally for distinguishing those values.

Ah, I missed your main comment above. Personally I would avoid using Int in one place and Word in an other, it would be confusing to me: why one max value is an int an an other a word?
If we want to refine a bit the types of the interface, let's do that in a new version like 0.8.0, we could put there multiple breaking changes.

I'm not in favor of a breaking change for this topic. I'll leave the choice to you: inconsistency in the API, or using Int in both places. My only stance is avoiding breaking changes.

At the end I reverted to the first original implementation, in this way we have similar settings for managerMaxHeaderLength and managerMaxNumberHeaders (we could also rename it to managerMaxNumberHeaderFields). I prefer to have a more uniform interface, it improves readability.

marinelli · 2024-12-16T06:25:55Z

http-client/Network/HTTP/Client/Types.hs

                   --
                   -- @since 0.5.0
+                   | TooManyHeaders


The introduction of this error might be considered a breaking change, but it also simplified debugging the code.

This is acceptable, we've documented that adding extra constructors to error types is something end users should expect.

I renamed it to TooManyHeaderFields, it's more clear.

marinelli · 2024-12-16T06:27:35Z

http-conduit/test/main.hs

@@ -261,6 +262,18 @@ main = do
            let Just req1 = parseUrlThrow $ "http://127.0.0.1:" ++ show port
            _ <- httpLbs req1 manager
            return ()
+        it "too many header fields" $ tooManyHeaderFields $ \port -> do


I just wrote those tests here, but we could move the entire DOS protection tests in the http-client package, we do not really need a data stream to raise the OverlongHeaders exception.

I moved the tests for TooManyHeaderFields in the http-client package. I tried to move also the ones for OverlongHeaders, without success. I didn't find the correct way to simulate it (something similar to what I did for TooManyHeaderFields).

snoyberg · 2024-12-16T09:49:22Z

http-client/Network/HTTP/Client.hs

@@ -324,7 +325,12 @@ managerSetProxy po = managerSetInsecureProxy po . managerSetSecureProxy po
 -- @since 0.7.17
 managerSetMaxHeaderLength :: Int -> ManagerSettings -> ManagerSettings
 managerSetMaxHeaderLength l manager = manager
-    { managerMaxHeaderLength = Just $ MaxHeaderLength l }
+    { managerMaxHeaderLength = MaxHeaderLength l }


This should also not change, yes it's an internal module, but there's no need to break end-user code for this.

snoyberg · 2024-12-16T09:50:26Z

http-client/Network/HTTP/Client/Types.hs

                   --
                   -- @since 0.5.0
+                   | TooManyHeaders


This is acceptable, we've documented that adding extra constructors to error types is something end users should expect.

This reverts commit c3312b4.

snoyberg requested changes Dec 13, 2024

View reviewed changes

marinelli requested a review from snoyberg December 13, 2024 17:48

tfausak reviewed Dec 13, 2024

View reviewed changes

snoyberg requested changes Dec 16, 2024

View reviewed changes

marinelli added 8 commits December 16, 2024 07:24

Add the possibility to set the maximum number of headers

2812a6c

Improve comments and update changelog

1411cf0

Fix comments

74d5048

Bump http-client version

4916257

Use Word in MaxHeaderLength and MaxNumberHeaders

c3312b4

Update http-client changelog

d6644ac

Revert some changes; add TooManyHeaders exception; add tests

b2e496a

Switch back to Int

a104b38

marinelli commented Dec 16, 2024

View reviewed changes

marinelli force-pushed the set-max-headers branch from f57e83d to a104b38 Compare December 16, 2024 06:30

Update http-client changelog

aa8930e

snoyberg requested changes Dec 16, 2024

View reviewed changes

marinelli added 2 commits December 16, 2024 19:05

Revert "Use Word in MaxHeaderLength and MaxNumberHeaders"

749ed45

This reverts commit c3312b4.

Move tests and rename error

1e82860

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the possibility to set the maximum number of header fields #549

Add the possibility to set the maximum number of header fields #549

marinelli commented Dec 12, 2024

snoyberg left a comment

marinelli commented Dec 13, 2024

snoyberg commented Dec 13, 2024

tfausak commented Dec 13, 2024 •

edited

Loading

marinelli commented Dec 13, 2024 •

edited

Loading

tfausak commented Dec 13, 2024

marinelli commented Dec 13, 2024 •

edited

Loading

marinelli commented Dec 13, 2024 •

edited

Loading

tfausak left a comment

tfausak Dec 13, 2024

marinelli Dec 16, 2024

marinelli Dec 18, 2024 •

edited

Loading

tfausak Dec 13, 2024

marinelli Dec 18, 2024 •

edited

Loading

tfausak Dec 13, 2024

marinelli Dec 16, 2024

marinelli Dec 18, 2024

snoyberg left a comment

snoyberg Dec 16, 2024

marinelli Dec 16, 2024

marinelli Dec 16, 2024

marinelli Dec 16, 2024

marinelli Dec 16, 2024

snoyberg Dec 16, 2024

marinelli Dec 18, 2024

marinelli Dec 16, 2024

snoyberg Dec 16, 2024

marinelli Dec 18, 2024

marinelli Dec 16, 2024 •

edited

Loading

marinelli Dec 18, 2024

snoyberg Dec 16, 2024

snoyberg Dec 16, 2024

-                when (total >= unMaxHeaderLength mhl && total /= 0) $ do
+                let n = unMaxHeaderLength mhl
+                    total' = total + fromIntegral (S.length bs)
+                when (total' >= n && n /= 0) $ do

	when (count >= unMaxNumberHeaders mnh && count /= 0) $ do
	let n = unMaxNumberHeaders mnh
	in when (count >= n && n /= 0) $ do

Add the possibility to set the maximum number of header fields #549

Are you sure you want to change the base?

Add the possibility to set the maximum number of header fields #549

Conversation

marinelli commented Dec 12, 2024

snoyberg left a comment

Choose a reason for hiding this comment

marinelli commented Dec 13, 2024

snoyberg commented Dec 13, 2024

tfausak commented Dec 13, 2024 • edited Loading

marinelli commented Dec 13, 2024 • edited Loading

tfausak commented Dec 13, 2024

marinelli commented Dec 13, 2024 • edited Loading

marinelli commented Dec 13, 2024 • edited Loading

tfausak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marinelli Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marinelli Dec 18, 2024 • 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

snoyberg left a 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

marinelli Dec 16, 2024 • 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

tfausak commented Dec 13, 2024 •

edited

Loading

marinelli commented Dec 13, 2024 •

edited

Loading

marinelli commented Dec 13, 2024 •

edited

Loading

marinelli commented Dec 13, 2024 •

edited

Loading

marinelli Dec 18, 2024 •

edited

Loading

marinelli Dec 18, 2024 •

edited

Loading

marinelli Dec 16, 2024 •

edited

Loading