Skip to content

Conversation

@JulienMalka
Copy link

@JulienMalka JulienMalka commented Jun 21, 2025

User description

Opening this PR for the sole purpose of hosting the patch (that has already been submitted by @infinisil in an issue).

Fixes #3

@qodo-code-review
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 Security concerns

Input validation weakening:
The change from BS16.decode to BS16.decodeLenient removes strict validation of hex input. While BS16.decode would fail on invalid hex characters, decodeLenient silently ignores them. This could potentially allow malformed input to pass through undetected, which might be exploitable depending on how the decoded data is used downstream.

⚡ Recommended focus areas for review

Error Handling

The migration from fst . BS16.decode to BS16.decodeLenient changes error handling behavior. The old version would fail on invalid hex input, while decodeLenient silently ignores invalid characters. This could mask input validation issues and lead to unexpected behavior with malformed hex strings.

toBinary (HexString bs) = B.decode . BSL.fromStrict . BS16.decodeLenient $ bs

-- | Reads a 'BS.ByteString' as raw bytes and converts to hex representation. We
--   cannot use the instance Binary of 'BS.ByteString' because it provides
--   a leading length, which is not what we want when dealing with raw bytes.
fromBytes :: BS.ByteString -> HexString
fromBytes = hexString . BS16.encode

-- | Access to the raw bytes in a 'BS.ByteString' format.
toBytes :: HexString -> BS.ByteString
toBytes (HexString bs) = BS16.decodeLenient bs

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add hex validation before decoding

The decodeLenient function may silently ignore invalid hex characters,
potentially leading to data corruption. Consider validating the hex string
before decoding to ensure data integrity.

src/Data/HexString.hs [55]

-toBinary (HexString bs) = B.decode . BSL.fromStrict . BS16.decodeLenient $ bs
+toBinary (HexString bs) = case BS16.decode bs of
+  Right decoded -> B.decode . BSL.fromStrict $ decoded
+  Left err -> error $ "Invalid hex string: " ++ err
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly points out that decodeLenient can silently ignore invalid characters, which could lead to data corruption. The proposed change to use the strict BS16.decode with explicit error handling is a significant improvement in robustness and safety over the PR's implementation.

Medium
Replace lenient with strict decoding

Using decodeLenient here may silently accept malformed hex data, which could
cause unexpected behavior. Consider using strict decoding with proper error
handling.

src/Data/HexString.hs [65]

-toBytes (HexString bs) = BS16.decodeLenient bs
+toBytes (HexString bs) = case BS16.decode bs of
+  Right decoded -> decoded
+  Left err -> error $ "Invalid hex string: " ++ err
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that using decodeLenient can hide malformed data issues. Switching to the strict BS16.decode and explicitly handling potential errors is a much safer approach, preventing unexpected behavior from invalid hex strings. This is a valuable improvement for data integrity.

Medium
  • More

@JulienMalka
Copy link
Author

I am a bit puzzled as to why there is a LLM bot that edits my own messages...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incompatible with base16-bytestring >=1.0

1 participant