Set a timeout on the rules fetch request - #43
Open
lbajsarowicz wants to merge 1 commit into
Open
Conversation
fetchRules() issued its GET without a timeout, so a stalled connection to the rules endpoint could hang the cron job or the sync-rules CLI command indefinitely and block the sansec cron group. Set a 30 second timeout, matching the pattern already used by Report::sendReport().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Model/Rules.php::fetchRules()does:Magento\Framework\HTTP\Client\Curlhas no default timeout, so a connection that stalls (network partition, a slow or hungshield.sansec.ioendpoint, a misconfigured proxy) blocks this call forever.fetchRules()is reached from two places:Cron\SyncRules, registered in thesanseccron group withuse_separate_process="1"and a 2-minuteschedule_lifetime. Magento's schedule-lifetime cleanup only prunes stale schedule rows after the fact; it doesn't kill the running process, so a hung fetch ties up the cron group's worker indefinitely rather than just failing one run.sansec:shield:sync-rulesCLI command, where the same hang blocks the invoking shell/process (e.g. a deploy script) with no bound.Model/Report.php:91already guards its own Curl call the same way this module needs here:Change
Added a private constant and a
setTimeout()call inModel/Rules.php:No new configuration option, matching how
Report.phphardcodes its own timeout rather than exposing it.Why 30 seconds
Report.php'ssetTimeout(5)fits a small JSON report POST where a slow response just means a dropped report — losing it is cheap.fetchRules()is different: it's the mechanism that keeps the WAF's ruleset current, its payload can run to a few hundred KB, and its caller is a cron job with a 2-minute schedule lifetime, not a request-path call. 30 seconds is generous enough to complete a normal fetch of that payload size over a slow connection, while still bounding the worst case to a small fraction of the cron schedule window and failing well within a typical deploy-script timeout, instead of hanging indefinitely.Testing
--fail-on-warningandxmllintonetc/*.xmlboth passed.Magento\Framework\HTTP\Client\CurlFactoryis generated code (produced bybin/magento setup:di:compile), not a source file shipped inmagento/framework, so a unit test forRules::fetchRules()/syncRules()would need an inline stub of that class to satisfy the constructor's type hint. Upstream CI installs onlymagento/framework, so that stub would be needed there too, for a two-line change that mirrors the existing, untested$curl->setTimeout(5)inReport.php. Given that mismatch between test cost and change size, the safer call is to skip the test rather than add a brittle one.