Escape shipping postcode city XML fields#326
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)
📝 WalkthroughWalkthroughThis PR hardens XML output generation in shipping utilities by adding XML-escaping to the ChangesXML Escaping for Shipping Data
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/check-shipping-helper-xml-escaping.py (1)
7-27: ⚡ Quick winMake the guard resilient to formatting-only edits.
This test relies on exact source-string matches across the whole file, so harmless formatting/refactors can break it or unrelated text can satisfy it. Scope checks to
get_postcode_cityand match patterns instead of exact snippets.Proposed refactor
from pathlib import Path +import re @@ source = shipping_helper.read_text(encoding="utf-8") -required_snippets = [ - "htmlspecialchars((string) $postcode, ENT_XML1 | ENT_QUOTES, 'UTF-8')", - "htmlspecialchars((string) $city, ENT_XML1 | ENT_QUOTES, 'UTF-8')", - '"<Postalcode>{$escaped_postcode}</Postalcode>"', - '"<City>{$escaped_city}</City>"', -] +func_match = re.search( + r"function\s+get_postcode_city\s*\([^)]*\)\s*\{(?P<body>.*?)\n\s*\}", + source, + flags=re.S, +) +if not func_match: + raise SystemExit("Could not locate ShippingHelper::get_postcode_city") + +body = func_match.group("body") + +required_patterns = [ + r"htmlspecialchars\(\(string\)\s*\$postcode,\s*ENT_XML1\s*\|\s*ENT_QUOTES,\s*'UTF-8'\)", + r"htmlspecialchars\(\(string\)\s*\$city,\s*ENT_XML1\s*\|\s*ENT_QUOTES,\s*'UTF-8'\)", + r"<Postalcode>\{\$escaped_postcode\}</Postalcode>", + r"<City>\{\$escaped_city\}</City>", +] -missing = [snippet for snippet in required_snippets if snippet not in source] +missing = [p for p in required_patterns if not re.search(p, body)] if missing: raise SystemExit( - "ShippingHelper::get_postcode_city is missing XML escaping snippets: " + "ShippingHelper::get_postcode_city is missing XML escaping patterns: " + ", ".join(missing) ) @@ - if unsafe_snippet in source: + if unsafe_snippet in body: raise SystemExit( f"ShippingHelper::get_postcode_city still writes raw XML value: {unsafe_snippet}" )🤖 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-shipping-helper-xml-escaping.py` around lines 7 - 27, The test currently checks exact string snippets across the whole file which is brittle; update the check to isolate the ShippingHelper::get_postcode_city function body (look for the get_postcode_city definition) and then assert presence of XML-escaping patterns using regexes rather than exact literal snippets: verify that within get_postcode_city the postcode and city are passed to htmlspecialchars (or equivalent) with ENT_XML1 and ENT_QUOTES and that the escaped variables (e.g., $escaped_postcode/$escaped_city or the result of htmlspecialchars) are used inside the "<Postalcode>...</Postalcode>" and "<City>...</City>" tags; also detect and fail if raw "$postcode" or "$city" are inserted unescaped in those tags. Ensure the test searches only inside the function body and uses pattern matching so formatting changes don’t break it.
🤖 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-shipping-helper-xml-escaping.py`:
- Around line 7-27: The test currently checks exact string snippets across the
whole file which is brittle; update the check to isolate the
ShippingHelper::get_postcode_city function body (look for the get_postcode_city
definition) and then assert presence of XML-escaping patterns using regexes
rather than exact literal snippets: verify that within get_postcode_city the
postcode and city are passed to htmlspecialchars (or equivalent) with ENT_XML1
and ENT_QUOTES and that the escaped variables (e.g.,
$escaped_postcode/$escaped_city or the result of htmlspecialchars) are used
inside the "<Postalcode>...</Postalcode>" and "<City>...</City>" tags; also
detect and fail if raw "$postcode" or "$city" are inserted unescaped in those
tags. Ensure the test searches only inside the function body and uses pattern
matching so formatting changes don’t break it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be595ace-1338-4fa8-811a-3cc02e7f1e44
📒 Files selected for processing (2)
common/models/shipping/util/ShippingHelper.phptests/check-shipping-helper-xml-escaping.py
|
Follow-up after CodeRabbit review: I pushed What changed:
Validation run locally: python tests/check-shipping-helper-xml-escaping.py |
1 participant
Summary
ShippingHelper::get_postcode_city()builds DHL/Aramex address fragments/claim #59
Why this satisfies the bounty
Carrier rate requests are part of keeping the store platform running reliably at scale. The helper currently writes raw address values directly into XML, so customer/store cities or postcodes containing characters such as
&,<,>, quotes, or apostrophes can produce malformed DHL/Aramex request XML. Escaping the fields keeps the generated XML valid without changing the carrier flow.Existing PRs checked
Validation
C:\Users\Harada Hinata\.cache\codex-runtimes\codex-primary-runtime\dependencies\python\python.exe tests\check-shipping-helper-xml-escaping.pyC:\Users\Harada Hinata\.cache\codex-runtimes\codex-primary-runtime\dependencies\python\python.exe -m py_compile tests\check-shipping-helper-xml-escaping.pygit diff --cached --checkphp -l common/models/shipping/util/ShippingHelper.phpcould not be run locally because PHP is not installed in this environment.Summary by CodeRabbit
Bug Fixes
Tests