fix(giskard-checks): bound RegexMatching with regex timeout to mitigate ReDoS#2381
fix(giskard-checks): bound RegexMatching with regex timeout to mitigate ReDoS#2381kevinmessiaen merged 4 commits intomainfrom
Conversation
RegexMatching no longer maps regex.search timeouts to CheckResult.error; the engine's TimeoutError is raised instead. Update the ReDoS regression test to expect TimeoutError. Made-with: Cursor
There was a problem hiding this comment.
Code Review
This pull request replaces the standard 're' module with the 'regex' package in the RegexMatching check to introduce a configurable timeout, effectively mitigating potential ReDoS vulnerabilities. The feedback identifies a typo in the 'regex' dependency version within 'pyproject.toml' and recommends explicitly catching 'TimeoutError' in the 'run' method to return a structured failure result instead of allowing an unhandled exception. Additionally, the test suite should be updated to verify this failure status rather than expecting a raised exception.
| "giskard-core>=1.0.0a1", | ||
| "giskard-agents>=1.0.0a1", | ||
| "giskard-checks>=1.0.0a1", | ||
| "regex>=2026.1.15", |
There was a problem hiding this comment.
The version 2026.1.15 for the regex package appears to be a typo, as that version does not exist yet. It should likely be a current version (e.g., 2024.11.6).
| "regex>=2026.1.15", | |
| "regex>=2024.11.6", |
| try: | ||
| if re.search(pattern, text): | ||
| return CheckResult.success( | ||
| message=f"Text matches the regex pattern '{pattern}'.", | ||
| details=details, | ||
| ) | ||
| else: | ||
| return CheckResult.failure( | ||
| message=f"Text does not match the regex pattern '{pattern}'.", | ||
| details=details, | ||
| ) | ||
| except re.error as e: | ||
| matched = regex.search(pattern, text, timeout=self.match_timeout_seconds) | ||
| except regex.error as e: | ||
| return CheckResult.failure( | ||
| message=f"Invalid regex pattern '{pattern}': {str(e)}", | ||
| details=details, | ||
| ) |
There was a problem hiding this comment.
The regex.search function raises a TimeoutError when the specified timeout is exceeded. Currently, this exception is not handled, which will cause the check to fail with an unhandled exception. It is better to catch this error and return a CheckResult.failure to provide a cleaner error message to the user.
try:
matched = regex.search(pattern, text, timeout=self.match_timeout_seconds)
except TimeoutError:
return CheckResult.failure(
message=f"Regex matching timed out after {self.match_timeout_seconds}s.",
details=details,
)
except regex.error as e:
return CheckResult.failure(
message=f"Invalid regex pattern '{pattern}': {str(e)}",
details=details,
)| with pytest.raises(TimeoutError): | ||
| _ = await check.run(Trace()) |
There was a problem hiding this comment.
Since the run method should ideally handle the TimeoutError and return a failure result (as suggested), this test should be updated to verify the result status and message instead of expecting an exception to be raised.
| with pytest.raises(TimeoutError): | |
| _ = await check.run(Trace()) | |
| result = await check.run(Trace()) | |
| assert result.status == CheckStatus.FAIL | |
| assert "timed out" in result.message |
Description
Replace stdlib
re.searchwithregex.search(..., timeout=...).Add
match_timeout_secondsand a ReDoS regression test.