fix(security): remove is_safe html from Twig serial helpers - #18
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe Twig serial-number function and filter no longer declare HTML-safe output. Tests verify the registrations, and security documentation describes auto-escaping and safe handling of unsanitized values. The Symfony 8 reference configuration enables strict typing. ChangesSerial helper escaping
Demo strict typing
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/SECURITY.md (1)
50-54: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSecurity Misconfiguration (CWE-16)
Make the auto-escaping precondition explicit.
Twig respects the configured auto-escaping setting; if it is disabled, removing
is_safedoes not escape these outputs. Reword the first sentence to describe this conditional behavior before the “keep auto-escaping enabled” guidance.🤖 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 `@docs/SECURITY.md` around lines 50 - 54, Update the first sentence in the serial_number and serial_number_mask guidance to state that their output is auto-escaped only when Twig auto-escaping is enabled; preserve the existing recommendation to keep auto-escaping enabled and avoid |raw for unsanitized values.Source: Path instructions
🤖 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.
Inline comments:
In `@docs/SECURITY.md`:
- Around line 50-54: Add an Unreleased security changelog note in
docs/CHANGELOG.md for the Twig serial_number and serial_number_mask XSS
guidance, using the existing 2.6/Unreleased Security section or the nearest
equivalent entry. Keep the note focused on the visible behavior that these Twig
outputs are not HTML-safe and remain auto-escaped by default, and do not add an
UPGRADING.md change unless the changelog entry implies an integration action.
---
Nitpick comments:
In `@docs/SECURITY.md`:
- Around line 50-54: Update the first sentence in the serial_number and
serial_number_mask guidance to state that their output is auto-escaped only when
Twig auto-escaping is enabled; preserve the existing recommendation to keep
auto-escaping enabled and avoid |raw for unsanitized values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 23c8e83a-651c-4556-9647-88dc18f82167
📒 Files selected for processing (3)
docs/SECURITY.mdsrc/Twig/SerialNumberTwigExtension.phptests/Unit/Twig/SerialNumberTwigExtensionTest.php
💤 Files with no reviewable changes (1)
- src/Twig/SerialNumberTwigExtension.php
| Twig function `serial_number` and filter `serial_number_mask` do **not** mark output as HTML-safe (`is_safe` is not set). Twig **auto-escapes** their results in HTML templates by default. | ||
|
|
||
| - **Recommendation:** Do not pass unsanitized user input (forms, query strings, etc.) directly into `serial_number()` or `serial_number_mask()` without validating/escaping the result for HTML. | ||
| - If serials or context values may contain user content: | ||
| - Escape in the template (e.g. `{{ serial|serial_number_mask(4)|e }}` when you need escaping for that value), or | ||
| - Ensure values are sanitized before they reach the bundle. | ||
|
|
||
| The bundle does not HTML-escape; the application must use serials safely in HTML context. | ||
| - Prefer application-controlled values (e.g. system-generated invoice numbers). | ||
| - If serials or context values may contain user content, keep auto-escaping enabled; do not pipe through `|raw` unless the value is already sanitized for HTML. | ||
| - Outside HTML contexts (e.g. attributes, JS), escape appropriately for that context. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
test -f docs/CHANGELOG.md
test -f docs/UPGRADING.md
rg -n -i 'unreleased|serial_number|serial_number_mask|html-safe|auto.?escap' \
docs/CHANGELOG.md docs/UPGRADING.mdRepository: nowo-tech/SerialNumberBundle
Length of output: 1067
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== CHANGELOG around Unreleased/security-related entries =="
sed -n '1,120p' docs/CHANGELOG.md
echo
echo "== CHANGELOG serial_number mentions around end =="
sed -n '180,215p' docs/CHANGELOG.md
echo
echo "== UPGRADING Twig section =="
sed -n '80,115p' docs/UPGRADING.md
echo
echo "== SECURITY section context =="
sed -n '35,65p' docs/SECURITY.mdRepository: nowo-tech/SerialNumberBundle
Length of output: 11206
Add the Unreleased changelog entry for Twig XSS guidance.
docs/SECURITY.md documents visible XSS guidance for serial_number / serial_number_mask, but docs/CHANGELOG.md has no Unreleased entry covering this security behavior. Add it under docs/CHANGELOG.md section 2.6 (or the appropriate Unreleased Security entry) so this user-visible mitigation is recorded. No UPGRADING.md change is needed unless integrators must change code/config.
🤖 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 `@docs/SECURITY.md` around lines 50 - 54, Add an Unreleased security changelog
note in docs/CHANGELOG.md for the Twig serial_number and serial_number_mask XSS
guidance, using the existing 2.6/Unreleased Security section or the nearest
equivalent entry. Keep the note focused on the visible behavior that these Twig
outputs are not HTML-safe and remain auto-escaped by default, and do not add an
UPGRADING.md change unless the changelog entry implies an integration action.
Source: Path instructions
HecFranco
changed the title
|
@coderabbitai review |
|
1 participant
Summary
is_safe => ['html']fromserial_number/serial_number_maskgetSafe()is nullFixes #17
Test plan
Made with Cursor
Summary by CodeRabbit
Bug Fixes
Documentation