-
Notifications
You must be signed in to change notification settings - Fork 6
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
[#295] fix failing with OverlongHeaders #315
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,12 +103,14 @@ addExclusionOptions ExclusionConfig{..} (ExclusionOptions ignore) = | |
|
||
data NetworkingOptions = NetworkingOptions | ||
{ noMaxRetries :: Maybe Int | ||
, noMaxHeaderLength :: Maybe Int | ||
} | ||
|
||
addNetworkingOptions :: NetworkingConfig -> NetworkingOptions -> NetworkingConfig | ||
addNetworkingOptions NetworkingConfig{..} (NetworkingOptions maxRetries) = | ||
addNetworkingOptions NetworkingConfig{..} (NetworkingOptions maxRetries maxHeaderLength) = | ||
NetworkingConfig | ||
{ ncMaxRetries = fromMaybe ncMaxRetries maxRetries | ||
, ncMaxHeaderLength = fromMaybe ncMaxHeaderLength maxHeaderLength | ||
, .. | ||
} | ||
|
||
|
@@ -228,6 +230,13 @@ networkingOptionsParser = do | |
value Nothing <> | ||
help "How many attempts to retry an external link after getting \ | ||
\a \"429 Too Many Requests\" response." | ||
|
||
noMaxHeaderLength <- option (Just <$> auto) $ | ||
long "header-limit" <> | ||
metavar "INT" <> | ||
value Nothing <> | ||
help "The maximum allowed total size of HTTP headers (in bytes) \ | ||
\ that can be returned by the server." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: you insert two spaces between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, let's strip extending CLI options. The code is good, the reason is that, in this project, CLI arguments serve to tune how verification is run, and config serves to tune which set of errors should be covered. If I run So having header length limit specified in config should suffice. |
||
return NetworkingOptions{..} | ||
|
||
dumpConfigOptions :: Parser Command | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,18 +7,22 @@ module Xrefcheck.Command | |
( defaultAction | ||
) where | ||
|
||
import Universum | ||
import Universum hiding ((.~)) | ||
|
||
import Control.Lens ((.~)) | ||
|
||
import Data.Reflection (Given, give) | ||
import Data.Yaml (decodeFileEither, prettyPrintParseException) | ||
import Fmt (build, fmt, fmtLn) | ||
import System.Console.Pretty (supportsPretty) | ||
import System.Directory (doesFileExist) | ||
import Text.Interpolation.Nyan | ||
import Network.HTTP.Client (newManager, managerSetMaxHeaderLength) | ||
import Network.HTTP.Client.TLS (tlsManagerSettings) | ||
|
||
import Xrefcheck.CLI (Options (..), addExclusionOptions, addNetworkingOptions, defaultConfigPaths) | ||
import Xrefcheck.Config | ||
(Config, Config' (..), ScannersConfig, ScannersConfig' (..), defConfig, overrideConfig) | ||
(Config, Config' (..), NetworkingConfig' (..), ScannersConfig, ScannersConfig' (..), defConfig, overrideConfig, cNetworkingL, ncHttpManagerL) | ||
import Xrefcheck.Core (Flavor (..)) | ||
import Xrefcheck.Progress (allowRewrite) | ||
import Xrefcheck.Scan | ||
|
@@ -87,8 +91,12 @@ defaultAction Options{..} = do | |
whenJust (nonEmpty $ sortBy (compare `on` seFile) scanErrs) reportScanErrs | ||
|
||
verifyRes <- allowRewrite showProgressBar $ \rw -> do | ||
let fullConfig = config | ||
let parsedConfig = config | ||
{ cNetworking = addNetworkingOptions (cNetworking config) oNetworkingOptions } | ||
|
||
mgr <- newManager $ managerSetMaxHeaderLength (ncMaxHeaderLength (cNetworking parsedConfig)) tlsManagerSettings | ||
let fullConfig = parsedConfig & cNetworkingL . ncHttpManagerL .~ Just mgr | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's better not to mix config and things created at runtime, and propagate the manager as separate argument. Shouldn't require too much chances as far as I can tell. |
||
|
||
verifyRepo rw fullConfig oMode repoInfo | ||
|
||
case verifyErrors verifyRes of | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
{- SPDX-FileCopyrightText: 2021 Serokell <https://serokell.io> | ||
- | ||
- SPDX-License-Identifier: MPL-2.0 | ||
-} | ||
|
||
module Test.Xrefcheck.MaxHeaderLengthSpec where | ||
|
||
import Universum hiding ((.~)) | ||
|
||
import Control.Lens ((.~)) | ||
import Data.Set qualified as S | ||
import Network.HTTP.Client (newManager, managerSetMaxHeaderLength, defaultManagerSettings) | ||
import Network.HTTP.Types (ok200) | ||
import Network.Wai qualified as Web | ||
import Test.Tasty (TestTree, testGroup) | ||
import Test.Tasty.HUnit (testCase) | ||
import Web.Scotty qualified as Web | ||
import qualified Data.Text as T | ||
import qualified Data.Text.Lazy as TL | ||
|
||
import Test.Xrefcheck.UtilRequests | ||
import Xrefcheck.Config | ||
import Xrefcheck.Progress | ||
import Xrefcheck.Verify | ||
|
||
mockHeader :: Int -> IO Web.Application | ||
mockHeader size = Web.scottyApp $ do | ||
Web.matchAny "/header" $ do | ||
Web.setHeader "X-header" (TL.fromStrict $ T.replicate size "x") | ||
Web.status ok200 | ||
|
||
test_maxHeaderLength :: TestTree | ||
test_maxHeaderLength = testGroup "MaxHeaderLength tests" | ||
[ testCase "Succeeds with small header" $ do | ||
setRef <- newIORef S.empty | ||
mgr <- newManager $ managerSetMaxHeaderLength mhl defaultManagerSettings | ||
checkMultipleLinksWithServer | ||
(5001, mockHeader (mhl `div` 2)) | ||
setRef | ||
[ VerifyLinkTestEntry | ||
{ vlteConfigModifier = \c -> c | ||
& cNetworkingL . ncMaxHeaderLengthL .~ mhl | ||
& cNetworkingL . ncHttpManagerL .~ Just mgr | ||
, vlteLink = "http://127.0.0.1:5001/header" | ||
, vlteExpectedProgress = mkProgressWithOneTask True | ||
, vlteExpectationErrors = VerifyResult [] | ||
} | ||
] | ||
|
||
, testCase "Fails with MaxHeaderLengthError" $ do | ||
setRef <- newIORef S.empty | ||
mgr <- newManager $ managerSetMaxHeaderLength mhl defaultManagerSettings | ||
checkMultipleLinksWithServer | ||
(5002, mockHeader (mhl*2)) | ||
setRef | ||
[ VerifyLinkTestEntry | ||
{ vlteConfigModifier = \c -> c | ||
& cNetworkingL . ncMaxHeaderLengthL .~ mhl | ||
& cNetworkingL . ncHttpManagerL .~ Just mgr | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If manager will be not part of config, its creation will likely be somewhere inside |
||
, vlteLink = "http://127.0.0.1:5002/header" | ||
, vlteExpectedProgress = mkProgressWithOneTask False | ||
, vlteExpectationErrors = VerifyResult [MaxHeaderLengthError mhl] | ||
} | ||
] | ||
] | ||
where | ||
mhl = 4096 | ||
|
||
mkProgressWithOneTask shouldSucceed = report "" $ initProgress 1 | ||
where | ||
report = | ||
if shouldSucceed | ||
then reportSuccess | ||
else reportError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just global comment: Make sure commit names start with uppercase, and use infinitive.
Other than that, looks good, thanks for using Problem/Solution template for commit description 👍