Fix _hashlib.compare_digest to reject non-ASCII strings#7280
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/stdlib/src/hashlib.rs`:
- Around line 738-745: The mixed-type error message currently uses
b.as_object().class().name() which reports the wrong type; update the match arms
handling ArgStrOrBytesLike variants (e.g., the (ArgStrOrBytesLike::Buf(a),
ArgStrOrBytesLike::Buf(b)) branch and the fallback) so that when one operand is
Str and the other is Buf you return vm.new_type_error with the incompatible type
set to "str" (or dynamically detect which of the two is ArgStrOrBytesLike::Str
and use "str") instead of using b.as_object().class().name(); keep the
constant_time_eq branch unchanged and only change the error construction for
mixed Str/Buf cases.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockLib/test/test_hmac.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/stdlib/Cargo.tomlcrates/stdlib/src/hashlib.rs
| (ArgStrOrBytesLike::Buf(a), ArgStrOrBytesLike::Buf(b)) => { | ||
| Ok(a.with_ref(|a| b.with_ref(|b| constant_time_eq(a, b)))) | ||
| } | ||
| _ => Err(vm.new_type_error(format!( | ||
| "a bytes-like object is required, not '{}'", | ||
| b.as_object().class().name() | ||
| ))); | ||
| ))), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the hashlib.rs file to understand the context
find . -type f -name "hashlib.rs" | head -5Repository: RustPython/RustPython
Length of output: 95
🏁 Script executed:
# Get the file and examine lines around 738-745
if [ -f "crates/stdlib/src/hashlib.rs" ]; then
wc -l "crates/stdlib/src/hashlib.rs"
fiRepository: RustPython/RustPython
Length of output: 97
🏁 Script executed:
# Read the relevant section with context
sed -n '700,760p' crates/stdlib/src/hashlib.rs | cat -nRepository: RustPython/RustPython
Length of output: 2418
🏁 Script executed:
# Search for the function containing this code and understand ArgStrOrBytesLike
rg -n "ArgStrOrBytesLike" crates/stdlib/src/hashlib.rs -B5 -A5 | head -60Repository: RustPython/RustPython
Length of output: 1651
🏁 Script executed:
# Search for similar error messages or patterns with mixed types in RustPython
rg -n "bytes-like object is required" . --type rust -B5 -A2Repository: RustPython/RustPython
Length of output: 2034
🏁 Script executed:
# Also check what ArgStrOrBytesLike::Buf actually is
rg -n "enum ArgStrOrBytesLike\|struct.*Buf" . --type rust -A10 | head -40Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Check if there are tests for compare_digest with mixed types
rg -n "compare_digest" . --type python -B3 -A3 | head -80Repository: RustPython/RustPython
Length of output: 95
🏁 Script executed:
# Let me check if there are Python test files for compare_digest
find . -type f -name "*.py" | xargs grep -l "compare_digest" 2>/dev/null | head -5Repository: RustPython/RustPython
Length of output: 197
🏁 Script executed:
# Look at the definition of ArgStrOrBytesLike to understand what Buf is
fd -e rs | xargs grep -l "ArgStrOrBytesLike" | head -3Repository: RustPython/RustPython
Length of output: 149
🏁 Script executed:
# Check the function.rs file to understand ArgStrOrBytesLike definition
if [ -f "vm/src/function.rs" ]; then
rg -n "enum ArgStrOrBytesLike\|impl.*ArgStrOrBytesLike" vm/src/function.rs -A15 | head -60
elif [ -f "crates/vm/src/function.rs" ]; then
rg -n "enum ArgStrOrBytesLike\|impl.*ArgStrOrBytesLike" crates/vm/src/function.rs -A15 | head -60
fiRepository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Search more broadly for ArgStrOrBytesLike definition
rg -n "enum ArgStrOrBytesLike" --type rust -A10Repository: RustPython/RustPython
Length of output: 730
🏁 Script executed:
# Also check the test file for compare_digest
if [ -f "Lib/test/test_hmac.py" ]; then
rg -n "compare_digest" Lib/test/test_hmac.py -B2 -A5
fiRepository: RustPython/RustPython
Length of output: 2968
🏁 Script executed:
# Check the hashlib tests or docstrings for compare_digest behavior
find . -type f -name "*.py" -exec grep -l "compare_digest" {} \; | xargs grep -l "bytes\|str" | head -3Repository: RustPython/RustPython
Length of output: 131
🏁 Script executed:
# Let's check the actual Python test to understand what error CPython produces
grep -n "str.*bytes\|bytes.*str" Lib/test/test_hmac.py -B2 -A2 | head -30Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Let's write a Python script to test CPython's behavior
python3 << 'EOF'
import hmac
import sys
# Test compare_digest with mixed types
test_cases = [
("string", b"bytes"),
(b"bytes", "string"),
]
for a, b in test_cases:
try:
result = hmac.compare_digest(a, b)
except TypeError as e:
print(f"compare_digest({type(a).__name__}, {type(b).__name__}): {e}")
EOFRepository: RustPython/RustPython
Length of output: 206
🏁 Script executed:
# Also check what the current RustPython implementation actually produces
# by looking at how the error case is structured in more detail
sed -n '721,746p' crates/stdlib/src/hashlib.rs | cat -nRepository: RustPython/RustPython
Length of output: 1205
🏁 Script executed:
# Let me verify the exact behavior by looking at what happens in each case
# The key insight: ArgStrOrBytesLike::Str means string, Buf means bytes-like
# Looking at the match logic:
# Case 1: (Str, Str) -> OK (both strings)
# Case 2: (Buf, Buf) -> OK (both bytes-like)
# Case 3: everything else -> ERROR using b's type
# So if a=Str and b=Buf:
# - Goes to case 3 (neither case 1 nor case 2 matches)
# - Error uses b.as_object().class().name() which is bytes
# - Message: "a bytes-like object is required, not 'bytes'" <- WRONG
# If a=Buf and b=Str:
# - Goes to case 3
# - Error uses b.as_object().class().name() which is str
# - Message: "a bytes-like object is required, not 'str'" <- matches CPython
python3 << 'EOF'
# Verify CPython's behavior more thoroughly
import hmac
test_cases = [
("string", b"bytes", "str first"),
(b"bytes", "string", "bytes first"),
("s1", "s2", "both str"),
(b"b1", b"b2", "both bytes"),
]
for a, b, desc in test_cases:
try:
result = hmac.compare_digest(a, b)
print(f"{desc}: OK - {result}")
except TypeError as e:
print(f"{desc}: {e}")
EOFRepository: RustPython/RustPython
Length of output: 218
Fix error message to report 'str' instead of the second argument's type.
The error message in the mixed-type case uses b.as_object().class().name(), which is incorrect. When a is a string and b is bytes, this produces "a bytes-like object is required, not 'bytes'" which is misleading. CPython always reports "not 'str'" for mixed-type comparisons (both str+bytes and bytes+str cases), because str is the incompatible type. Change the error to report 'str' in all mixed-type cases, or dynamically determine which argument is Str and report that type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/stdlib/src/hashlib.rs` around lines 738 - 745, The mixed-type error
message currently uses b.as_object().class().name() which reports the wrong
type; update the match arms handling ArgStrOrBytesLike variants (e.g., the
(ArgStrOrBytesLike::Buf(a), ArgStrOrBytesLike::Buf(b)) branch and the fallback)
so that when one operand is Str and the other is Buf you return
vm.new_type_error with the incompatible type set to "str" (or dynamically detect
which of the two is ArgStrOrBytesLike::Str and use "str") instead of using
b.as_object().class().name(); keep the constant_time_eq branch unchanged and
only change the error construction for mixed Str/Buf cases.
Lib/test/test_hmac.py
Outdated
|
|
||
| @unittest.expectedFailure # TODO: RUSTPYTHON; AssertionError: TypeError not raised by compare_digest | ||
| def test_exceptions(self): | ||
| return super().test_exceptions() |
There was a problem hiding this comment.
can you please remove the entire test?
Lib/test/test_hmac.py
Outdated
|
|
||
| @unittest.expectedFailure # TODO: RUSTPYTHON; AssertionError: TypeError not raised by compare_digest | ||
| def test_exceptions(self): | ||
| return super().test_exceptions() |
Add non-ASCII string check to _hashlib.compare_digest, matching the behavior of _operator._compare_digest. When both arguments are strings, non-ASCII characters now correctly raise TypeError. Also replace the non-constant-time == comparison with constant_time_eq for proper timing-attack resistance, and return PyResult<bool> instead of PyResult<PyObjectRef>.
959cb04 to
c100b7f
Compare
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: Error: 'frozenset' object has no attribute 'discard' Legend:
|
Add non-ASCII string check to _hashlib.compare_digest, matching the behavior of _operator._compare_digest. When both arguments are strings, non-ASCII characters now correctly raise TypeError.
Also replace the non-constant-time == comparison with constant_time_eq for proper timing-attack resistance, and return PyResult instead of PyResult.
Summary by CodeRabbit