Harden URL reads and XML serialization#343
Conversation
Reviewer's GuideThis PR hardens remote URL reading and XML serialization by enforcing public-only, size-bounded HTTP(S) reads, adding XML 1.0 character validation and secure namespace/type handling in both Python and Rust paths, gating Rust accelerators behind the same safety checks, and tightening dependency and documentation coverage around these behaviors. Sequence diagram for hardened readfromurl URL loadingsequenceDiagram
actor Caller
participant Utils as readfromurl
participant Validator as _validate_url
participant Client as _get_http_client
participant HTTP as urllib3_HTTPClient
Caller->>Utils: readfromurl(url, params, max_response_bytes, allow_private_networks)
Utils->>Utils: validate max_response_bytes
Utils->>Validator: _validate_url(url, allow_private_networks)
Validator-->>Utils: ok or URLReadError
Utils->>Client: _get_http_client()
Client-->>Utils: http, timeout
Utils->>HTTP: request("GET", url, fields=params, timeout, retries=False, redirect=False, preload_content=False)
HTTP-->>Utils: response
Utils->>Utils: check status, Content-Length, size limits
Utils->>HTTP: read(max_response_bytes + 1, decode_content=True)
HTTP-->>Utils: response_data
Utils->>Utils: json.loads(response_data.decode("utf-8"))
Utils-->>Caller: JSONValue or URLReadError
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #343 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 7 7
Lines 759 833 +74
=========================================
+ Hits 759 833 +74
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
Hey - I've found 1 issue
Fixed security issues:
- urllib3 (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/test_dict2xml.py" line_range="92-94" />
<code_context>
assert result == f'<key {attr_name}="value">payload</key>'.encode()
+ # @lat: [[tests#Conversion behavior#Custom type attributes use shared escaping]]
+ def test_custom_type_attribute_value_is_escaped(self) -> None:
+ """A caller-provided type attribute must not bypass value escaping."""
+ result = dicttoxml.dicttoxml(
+ {
+ "key": {
+ "@attrs": {"type": 'safe" injected="yes'},
+ "@val": "payload",
+ }
+ },
+ root=False,
+ attr_type=False,
+ )
+
+ assert result == (
+ b'<key type="safe" injected="yes">payload</key>'
+ )
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that invalid XML 1.0 characters in attribute values are rejected
The new tests cover XML 1.0–forbidden characters in element text and CDATA and verify escaping for custom `type` attributes, but they don’t directly assert that XML 1.0 validation applies to attribute values. Please add a test like:
```python
with pytest.raises(ValueError, match="not allowed in XML 1.0"):
dicttoxml.dicttoxml(
{
"key": {
"@attrs": {"type": "before\x00after"},
"@val": "payload",
}
},
root=False,
attr_type=False,
)
```
This ensures attributes cannot bypass the forbidden-character checks.
```suggestion
# @lat: [[tests#Conversion behavior#Custom type attributes are validated]]
def test_custom_type_attribute_value_forbidden_chars_rejected(self) -> None:
"""XML 1.0–forbidden characters in caller-provided type attributes must be rejected."""
with pytest.raises(ValueError, match="not allowed in XML 1.0"):
dicttoxml.dicttoxml(
{
"key": {
"@attrs": {"type": "before\x00after"},
"@val": "payload",
}
},
root=False,
attr_type=False,
)
# @lat: [[tests#Conversion behavior#Namespace metadata cannot inject attributes]]
def test_namespace_values_are_escaped_as_xml_attributes(self) -> None:
"""Namespace values must not be able to inject sibling attributes."""
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary
typeattributes, and reject XML 1.0-forbidden characters in both Python and Rust serializersurllib3requirements pin to 2.7.0 and add weekly Python Dependabot coverage for the root and docs manifestslat.mdarchitecture/test graphCompatibility note
readfromurl()now rejects private destinations by default. Trusted callers that intentionally read a private endpoint must passallow_private_networks=True; they can also setmax_response_bytesexplicitly.Validation
requirements.txtandrequirements.in: no known vulnerabilitieslat check: passSummary by Sourcery
Harden remote JSON reading and XML serialization by enforcing public-only, bounded URL requests, validating XML 1.0 character and namespace safety across Python and Rust backends, and updating dependencies, documentation, and tests accordingly.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests: