Escape DHL rate XML fields#328
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesDHL XML Escaping for Quote Requests
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/check-dhl-rate-xml-escaping.py (2)
7-25: ⚡ Quick winMake guardrails whitespace-tolerant to avoid brittle failures.
These checks are highly formatting-dependent (
\t, quote style, spacing), so harmless reformatting can break the test without changing safety. Prefer regex checks over exact string literals for semantic assertions.Proposed refactor
from pathlib import Path +import re @@ -missing = [snippet for snippet in required_snippets if snippet not in source] +required_patterns = [ + r"private function escapeXmlValue\(\$value\)", + r"htmlspecialchars\(\(string\)\s*\$value,\s*ENT_XML1\s*\|\s*ENT_QUOTES,\s*'UTF-8'\)", + r"<PaymentAccountNumber>\"\s*\.\s*\$this->escapeXmlValue\(\$dhlexpress_account\)\s*\.\s*\"</PaymentAccountNumber>", + r"\$this->escapeXmlValue\(\$fromCity\)", + r"\$this->escapeXmlValue\(\$fromPostcode\)", + r"\$this->escapeXmlValue\(\$to_city\)", + r"\$this->escapeXmlValue\(\$postcode\)", +] + +missing = [p for p in required_patterns if not re.search(p, source)] @@ -for unsafe_snippet in unsafe_snippets: - if unsafe_snippet in source: - raise SystemExit(f"DHL Express XML still writes raw value: {unsafe_snippet}") +unsafe_patterns = [ + r"<PaymentAccountNumber>\"\s*\.\s*\$dhlexpress_account\s*\.\s*\"</PaymentAccountNumber>", + r"ShippingHelper::get_postcode_city\(\$fromCountryCode,\s*\$fromCity,\s*\$fromPostcode\)", + r"ShippingHelper::get_postcode_city\(\$county_code_to,\s*\$to_city,\s*\$postcode\)", +] + +for unsafe_pattern in unsafe_patterns: + if re.search(unsafe_pattern, source): + raise SystemExit(f"DHL Express XML still writes raw value pattern: {unsafe_pattern}")Also applies to: 34-46
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/check-dhl-rate-xml-escaping.py` around lines 7 - 25, The test's required_snippets list is brittle because it matches exact whitespace and quoting; change the checks for symbols like escapeXmlValue, htmlspecialchars, PaymentAccountNumber, SiteID, Password, CountryCode, DeclaredCurrency, DeclaredValue, DimensionUnit, WeightUnit and others to use whitespace-tolerant regexes (e.g. patterns built with re.compile that allow \s* between tokens and accept either single/double-quote concatenation forms) and use re.search against the generated XML instead of direct string inclusion; update the required_snippets handling in tests/check-dhl-rate-xml-escaping.py to convert the current literal strings into regex patterns and apply the same fix to the other block referenced (lines 34-46).
15-16: ⚡ Quick winAssert escaped declared value/currency assignments directly.
The test currently verifies tag usage but not that
$declared_currencyand$declared_valueare created viaescapeXmlValue(). Add direct checks so this cannot regress silently.Proposed addition
required_snippets = [ @@ "<DeclaredCurrency>{$declared_currency}</DeclaredCurrency>", "<DeclaredValue>{$declared_value}</DeclaredValue>", + "$declared_currency = $this->escapeXmlValue($selected_currency);", + "$declared_value = $this->escapeXmlValue($total_value);", @@ ]Also applies to: 27-33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/check-dhl-rate-xml-escaping.py` around lines 15 - 16, Add direct assertions that the variables $declared_currency and $declared_value are produced by escapeXmlValue() instead of only checking tag presence: call escapeXmlValue(raw_currency_input) and escapeXmlValue(raw_value_input) and assert $declared_currency === that result and $declared_value === that result (or assert the generated XML strings for "<DeclaredCurrency>{$declared_currency}</DeclaredCurrency>" and "<DeclaredValue>{$declared_value}</DeclaredValue>" contain the escapeXmlValue(...) outputs). Repeat the same explicit assertions for the other declared-currency/value occurrences referenced in the 27-33 block so escaping cannot regress.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/check-dhl-rate-xml-escaping.py`:
- Around line 7-25: The test's required_snippets list is brittle because it
matches exact whitespace and quoting; change the checks for symbols like
escapeXmlValue, htmlspecialchars, PaymentAccountNumber, SiteID, Password,
CountryCode, DeclaredCurrency, DeclaredValue, DimensionUnit, WeightUnit and
others to use whitespace-tolerant regexes (e.g. patterns built with re.compile
that allow \s* between tokens and accept either single/double-quote
concatenation forms) and use re.search against the generated XML instead of
direct string inclusion; update the required_snippets handling in
tests/check-dhl-rate-xml-escaping.py to convert the current literal strings into
regex patterns and apply the same fix to the other block referenced (lines
34-46).
- Around line 15-16: Add direct assertions that the variables $declared_currency
and $declared_value are produced by escapeXmlValue() instead of only checking
tag presence: call escapeXmlValue(raw_currency_input) and
escapeXmlValue(raw_value_input) and assert $declared_currency === that result
and $declared_value === that result (or assert the generated XML strings for
"<DeclaredCurrency>{$declared_currency}</DeclaredCurrency>" and
"<DeclaredValue>{$declared_value}</DeclaredValue>" contain the
escapeXmlValue(...) outputs). Repeat the same explicit assertions for the other
declared-currency/value occurrences referenced in the 27-33 block so escaping
cannot regress.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3469ebca-5a6e-4445-894c-ba1b3e04c748
📒 Files selected for processing (2)
common/models/shipping/Dhlexpress.phptests/check-dhl-rate-xml-escaping.py
|
Follow-up after CodeRabbit review: I pushed What changed:
Validation run locally: python tests/check-dhl-rate-xml-escaping.py |
1 participant
/claim #59
Summary
Validation
python tests/check-dhl-rate-xml-escaping.pypython -m py_compile tests/check-dhl-rate-xml-escaping.pygit diff --checkNote: PHP is not installed in this environment, so
php -lcould not be run locally.Summary by CodeRabbit
Bug Fixes
Tests