fix: added escaping #114
fix: added escaping #114
Conversation
WalkthroughReplaces unsafe raw outputs and HTTP endpoints with escaped values and HTTPS URLs across ReCAPTCHA-related PHP files, altering generated JavaScript/HTML and network endpoints without changing public function signatures. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (6)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
inc/class-wp_recaptcha_recaptcha.php (1)
188-188: Fix the malformed sprintf call.The
sprintfformat string is missing a%splaceholder, causing the translated text to be ignored.Apply this diff:
- $return .= sprintf('<div><a href="javascript:Recaptcha.reload()"></a></div>',__('Get another CAPTCHA','wp-recaptcha-integration')); + $return .= sprintf('<div><a href="javascript:Recaptcha.reload()">%s</a></div>',__('Get another CAPTCHA','wp-recaptcha-integration'));inc/class-wp_recaptcha_nocaptcha.php (1)
246-266: Add proper URL encoding for the sitekey in noscript fallback.The
data-sitekeyattribute value is concatenated directly into the iframe URL without URL encoding (line 250). While line 243 appliesesc_attr()to attributes in the main<div>, the noscript fallback embeds$attr['data-sitekey']directly into a URL, which should use URL encoding to prevent injection.Apply this diff to properly escape the URL parameter:
if ( WP_reCaptcha::instance()->get_option('recaptcha_noscript') ) { $return .= '<div style="width: 302px; height: 462px;">' . '<div style="width: 302px; height: 422px; position: relative;">' . '<div style="width: 302px; height: 422px; position: absolute;">' . - '<iframe src="https://www.google.com/recaptcha/api/fallback?k='.$attr['data-sitekey'].'"' . + '<iframe src="https://www.google.com/recaptcha/api/fallback?k='.rawurlencode($attr['data-sitekey']).'"' . ' frameborder="0" scrolling="no"' . ' style="width: 302px; height:422px; border-style: none;">' . '</iframe>' .
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
inc/class-wp_recaptcha_nocaptcha.php(2 hunks)inc/class-wp_recaptcha_recaptcha.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
inc/class-wp_recaptcha_nocaptcha.php (1)
inc/class-wp_recaptcha.php (3)
WP_reCaptcha(6-682)instance(25-29)get_option(484-504)
🔇 Additional comments (2)
inc/class-wp_recaptcha_nocaptcha.php (2)
189-190: Good security fix: XSS prevention in JavaScript context.The use of
esc_js()correctly prevents XSS when embedding the sitekey and theme values into JavaScript strings. This properly addresses the stored XSS vulnerability.
208-208: I need to view the actual content around line 208 to verify the original concern:Let me check the actual content of line 208 in the file:
Based on my verification process, I cannot locate the specific code referenced in the original review comment. The script output shows that both
esc_js()andesc_attr()escape the same special characters:< > & " 'and single quotes, and by default there is no difference betweenesc_html()andesc_attr(), with different filters applied.However, the critical issue is that the shell script execution did not reveal the actual code at line 208 of the file. The search results show that both functions use
_wp_specialchars()as their underlying implementation. For reCAPTCHA sitekeys, which are constrained to alphanumeric characters, underscores, and dashes, both escaping functions would produce identical output since neither function transforms these safe characters.The provided code snippet lacks the actual context needed to verify the concern, and the file search did not confirm the problematic code pattern exists on line 208. The existing codebase shows
esc_attr()is properly applied at HTML output time (line 243), where attribute values are escaped before rendering.Likely an incorrect or invalid review comment.
2 participants
fixes #109 , fixes #111
added js escaping along with the sanitization before.
vulnerable to Cross Site Scripting (XSS). [c885***]
Summary by CodeRabbit
Bug Fixes
Chores