fix(runner): report a response time value that does not fit a duration - #2655
CalvinTjoaquinn wants to merge 1 commit into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughFilterOperator.Parse now returns an invalid-value error when a numeric duration cannot be parsed after interpreting it as seconds. Tests cover valid operators and duration formats, whitespace, and rejected inputs. ChangesDuration parsing
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Oversized response-time filter values are now rejected instead of silently matching every host. The change is small and covered by tests, so it looks safe to merge. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change rejects an oversized response-time value instead of allowing it to act as a zero-duration filter. The identified callers already handle parsing errors before comparing response times. No broader boundary change was identified, though external exposure is not fully established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
I’m a rabbit, pleased to see Comment |
FilterOperator.Parse reads a bare number as seconds by retrying time.ParseDuration with an "s" appended, and that retry discarded its error. A number too large to hold as a duration left value at zero with nothing reported, so -mrt '>=10000000000' became a filter matching every host rather than a rejected flag. The function had no tests. The new ones cover each operator, the bare-number path, trimming, and the inputs that have to be rejected. Only the overflow case changes: the other fifteen pass before and after.
39e2416 to
9ec2c1d
Compare
1 participant
Proposed changes
FilterOperator.Parsereads a bare number as seconds by retryingtime.ParseDurationwith ansappended, and that retry discarded its error:A number that parses fine on its own but overflows once seconds are added takes the first branch, so
valuestays at zero and nothing is reported:>= 0sis true for every response, so the flag silently matches every host instead of being rejected. The flags affected are-mrt, -match-response-timeand-frt, -filter-response-time.Larger values happen to be caught already, because they fail the first parse with "invalid duration" rather than "missing unit" and fall through to the error branch:
So the gap is the range where the bare number is a valid float but the duration overflows, which starts a little above 9223372036 seconds.
Proof
Parsehad no tests, so the new file covers the whole contract: each of the six operators, the bare-number path, space trimming, and the inputs that have to be rejected.Against the current code only the overflow case fails:
The other fifteen subtests pass before and after, so the change is limited to the value that used to be swallowed.
go test ./runner/passes,go vetis clean, andgofmtreports nothing.Checklist
Summary by CodeRabbit