feat: fixed analyzer timeout issue + missing case fix #5878
Conversation
WalkthroughThe changes in this pull request involve modifications to two primary files. In Changes
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
pkg/fuzz/analyzers/time/time_delay.go (1)
Line range hint
4-4: Document the change in slope check removalThe comment block explaining the time delay analyzer should be updated to reflect the removal of slope proximity check and its implications for double time cases.
Add a note like:
// The approach is the one used in ZAP for timing based checks. +// Note: For better handling of double time cases (involving multiple queries), +// we only use correlation threshold without slope proximity check.pkg/protocols/http/httpclientpool/clientpool.go (2)
Line range hint
48-52: Consider thread safety for CustomMaxTimeoutWhile the cookiejar has proper mutex protection, the CustomMaxTimeout field in ConnectionConfiguration isn't protected against concurrent access. Although it's typically set once during configuration, it's better to be explicit about thread safety.
Consider adding CustomMaxTimeout to the mutex protection:
type ConnectionConfiguration struct { + mu sync.RWMutex DisableKeepAlive bool - CustomMaxTimeout time.Duration cookiejar *cookiejar.Jar - mu sync.RWMutex + customMaxTimeout time.Duration } +func (cc *ConnectionConfiguration) SetCustomMaxTimeout(timeout time.Duration) { + cc.mu.Lock() + defer cc.mu.Unlock() + cc.customMaxTimeout = timeout +} +func (cc *ConnectionConfiguration) GetCustomMaxTimeout() time.Duration { + cc.mu.RLock() + defer cc.mu.RUnlock() + return cc.customMaxTimeout +}
Line range hint
48-52: Document the precedence of CustomMaxTimeoutThe comment for CustomMaxTimeout should clearly document its precedence over other timeout settings.
Enhance the documentation:
// CustomMaxTimeout is the custom timeout for the connection -// This overrides all other timeouts and is used for accurate time based fuzzing. +// This overrides all other timeouts (including HttpTimeout and ResponseHeaderTimeout) +// and is specifically used for accurate time-based fuzzing where precise control +// over request timing is required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
pkg/fuzz/analyzers/time/time_delay.go(1 hunks)pkg/protocols/http/httpclientpool/clientpool.go(1 hunks)
🔇 Additional comments (1)
pkg/protocols/http/httpclientpool/clientpool.go (1)
114-114: LGTM: CustomMaxTimeout properly cloned
The CustomMaxTimeout field is correctly handled in the Clone method.
| return o.correlation > 1.0-correlationErrorRange | ||
| //math.Abs(expectedSlope-o.slope) < slopeErrorRange |
There was a problem hiding this comment.
💡 Codebase verification
Removing slope check could reduce detection accuracy
Based on the test cases and implementation analysis, removing the slope proximity check appears problematic:
-
The test file (
time_delay_test.go) shows that both correlation and slope are critical metrics:- Test cases verify both
slopeandcorrelationvalues - Constants define error ranges for both:
correlationErrorRange = 0.1andslopeErrorRange = 0.2
- Test cases verify both
-
The code uses
IsWithinConfidencein two contexts:- Main detection with
correlationErrorRange=0.3, slope=1.0, slopeErrorRange=0.5 - Regression analysis with custom thresholds
- Main detection with
Removing the slope check means we're only validating the correlation between input and response times, but not the actual rate of increase. This could lead to false positives where responses show correlated but disproportionate time increases.
🔗 Analysis chain
Verify the impact of removing slope proximity check
The removal of the slope proximity check (math.Abs(expectedSlope-o.slope) < slopeErrorRange) means we're now only relying on correlation for time-based detection. While this aligns with the PR objective to handle double time cases better, we should verify this doesn't increase false positives.
Consider:
- Adding test cases specifically for double time scenarios
- Documenting the threshold values and their significance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for test cases and usages to verify impact
# Look for test files and time-based detection patterns
rg -g "**/*test*.go" -A 5 "IsWithinConfidence"
# Look for SQL injection patterns to understand impact
ast-grep --pattern 'func $name($params) {
$$$
IsWithinConfidence($$$)
$$$
}'
Length of output: 131
Script:
#!/bin/bash
# Let's try a broader search to understand the usage context
# First, find all test files related to time delay analysis
fd "time.*test.*\.go$"
# Search for test cases mentioning correlation or slope
rg -g "**/*test*.go" "correlation|slope" -B 2 -A 2
# Look for the actual implementation details
rg "IsWithinConfidence" -A 5 -B 5
# Check commit history for the rationale
git log -p --grep="slope" -- "pkg/fuzz/analyzers/time/time_delay.go"
Length of output: 7457
Proposed changes
Checklist
Summary by CodeRabbit
New Features
CustomMaxTimeout.Bug Fixes
Documentation