Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.$status_report .= 'Check the error messages and <a href="' . request_uri() . '">try again</a>.';
The output of request_uri() cannot be used as is in HTML. It needs to be escaped.
Marking "critical" as it is a potential XSS bug, though quite hard to exploit.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | 680422.patch | 2.03 KB | grendzy |
| #4 | sdo_680422_follow_up_4_D7.patch | 1.68 KB | scor |
| check_url_request_uri_update.patch | 806 bytes | heine |
Issue fork drupal-680422
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
compare
Comments
Comment #1
c960657 commentedIsn't this equivalent to
<a href="">(i.e. with href set to an empty string)? Or doesn't that work in some older versions of IE? Anyway, this works and is easy to understand when reading the code.Comment #2
webchickGood catch. Committed to HEAD.
Comment #4
scor commentedThis patch fixes the remaining request_uri() in D7 which have been fixed in D6 as part of SA-CORE-2010-001 - Drupal core - Multiple vulnerabilities
Comment #5
scor commentedadding security.drupal.org tag
Comment #6
damien tournoud commentedLet's transform those
href="!url"into properhref="@url". We don't need to promote bad patterns in core.Comment #7
scor commentedarray('@url' => check_url(request_uri())is going to run check_plain() twice. Ok, it's in the installer, but if we're talking about patterns in core, they are places where check_plain() does not need to be run at all (like when we're not dealing with user input). I'm digressing here: if there is an appropriate issue for this topic, let me know.Comment #8
damien tournoud commented@scor: the standard (documented in t()) is:
$output .= '<p>'. t('Go to the <a href="@contact-page">contact page</a>.', array('@contact-page' => url('contact'))) .'</p>';Of course, we need to remove the check_plain() when using @ instead of !.
Comment #9
scor commentedincluding SA # in title
Comment #10
grendzy commentedComment #11
mr.baileysThe comment from #7 is still valid:
check_urlcallsfilter_xss_bad_protocolwhich performs acheck_plainbefore returning, so by using @url we are escaping the value twice...Comment #12
grendzy commentedyes, but isn't that what Damien was asking for? it seems check_url() is more correct than check_plain... so if the double-escaping is a problem then I think we should apply the patch in #4.
Comment #13
mr.baileysDouble-escaping is a problem (and a bug: #715142: [beta blocker blocker] Various URLs escaped twice, since check_url() resp. filter_xss_bad_protocol() calls check_plain()). I agree that using check_url is preferable to check_plain in this case, so I feel we should commit #4 and knock this one of the critical list.
Comment #14
grendzy commentedCool, thanks. This RTBC is for scor's patch #4.
Comment #15
dries commentedCommitted to CVS HEAD.