Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
The l() call does not correctly handle links when the protocol is anything but lower case. Calling
l('display text', 'HTTP://www.mywebsite.com')
Results in
http://drupalsite.org?q=HTTP://www.mywebsite.com
The problem is in filter.module, which is only checking for the lower case variant of protocol strings. A simple patch is attached.
Comment | File | Size | Author |
---|---|---|---|
#9 | filter.module.D5_0.patch | 733 bytes | scott.mclewin |
#6 | filter.module.D5.patch | 636 bytes | scott.mclewin |
filter.module_27.patch | 804 bytes | scott.mclewin | |
Comments
Comment #1
agentrickardComment #2
agentrickardThe HTTP specification does say that scheme checks should be case insensitive (http://www.ietf.org/rfc/rfc2616.txt).
If approved, this bug also applies to 5.x and HEAD. The affected function is filter_xss_bad_protocol().
Comment #3
RobRoy CreditAttribution: RobRoy commentedI'd recommend submitting a patch for 5.x-dev to be applied then backported to get more visibility. The (to be ported) status should only be used after a patch has been applied to a development version and it needs to be re-rolled to apply to previous versions.
Comment #4
RobRoy CreditAttribution: RobRoy commentedOops, finishing my title change. Not sure if this should be won't fixed or not, can someone find the appropriate quote from the RFC?
Comment #5
agentrickardFrom http://www.ietf.org/rfc/rfc2616.txt
This is the closest passage that I found regarding the use of http: or HTTP: The standard says to treat both as equals.
Comment #6
scott.mclewin CreditAttribution: scott.mclewin commentedI've rolled the patch for Drupal 5.
Comment #7
drummComment #8
Dries CreditAttribution: Dries commentedImo, this is worth documenting. Can we add a code comment referring to RFC 2616?
Comment #9
scott.mclewin CreditAttribution: scott.mclewin commentedDries - is this sufficient, or were you looking for a block comment with full details?
Comment #10
Dries CreditAttribution: Dries commentedThe code comment is sufficient, but this patch no longer applies against CVS HEAD.
Also, looking at the code some more, it's probably best to use
strripos
rather thanstrpos + strtolower
. If you insist on usingstrtolower
, it's probably best to lowercase the input string outside of the do-while-loop.We're almost there with this patch -- one more go? Thanks. :-)
Comment #11
Dries CreditAttribution: Dries commentedI obviously meant
stripos
(1 r) and notstrripos
(2 r's).Comment #12
scott.mclewin CreditAttribution: scott.mclewin commentedDries,
I factored out the extra "r". :) I knew what you meant.
In hindsight I have no clue why I didn't just use stripos() in the first place. I'm not bound to strtolower(). I think I must have bought a few extra strtolower() licenses and just needed to use them somewhere.
For what is now essentially a one letter change, we've certainly worked hard at it. :)
I'm not running Drupal HEAD anywhere and my test cases (which happen to be the wishlist.module I maintain) are on D5 rather than HEAD/D6. For such a simplistic change would you accept it against D5 and then port it into HEAD on my behalf? I'm not going to supply a patch that I've not tested - no matter how simple. Right now with a newborn child and a two year old at home, I get very little time to play with anything other than diapers, and I just don't see myself getting a HEAD + test case environment set up for so small a delta in the near term.
I'll roll this change for D5 no matter what the outcome on the D6/HEAD question.
Scott
Comment #13
scott.mclewin CreditAttribution: scott.mclewin commentedDries,
In settling in to remind myself of the details, I know why I used strtolower and not stripos.
$protocol is being used as an index into $allowed_protocols[] and itself needs to be lower case to be a valid index on that array, where all the keys are lower case.
I don't see a way to use stripos here that is an improvement over the existing logic, since the problem is not in finding the substring to extract, it is in keying the $allowed_protocols array. $protocol will still need to be converted to lower case. Does PHP have a case-insensitive array index operation?
On your other request, of doing the strtolower outside of the loop, I'll decline that one for the following reason: While the protocol component of the URI is supposed to be case insensitive, the rest of the URI is case sensitive. Because this function returns $string, I don't want to alter the full URI to lower case. I aimed to keep the conversion to just the protocol portion of the URI and to leave the rest of the string untouched. Without adding quite a bit of complexity, I don't see a clean way to achieve this without the strtolower() within the loop on $protocol.
Replacing soft hypens and running it through check_plain() are appropriate transformations. I don't think setting the full URI to lower case is an appropriate transformation because it would break the path portion of URIs when they use mixed case. IIS won't care, but Apache/*nix will.
If I am being dense please do articulate how, don't hold back. I want the right patch for Drupal, one that is effective, simple and as efficient as it can be. I think I've done that with this very minor patch, but I'm hardly as expert in PHP tricks as I am with C/C++ and may be overlooking a language trick.
Scott
Comment #14
RobRoy CreditAttribution: RobRoy commentedWe can't use stripos as it's PHP5 only...http://php.net/stripos. That's probably why you used strtolower().
Comment #15
scott.mclewin CreditAttribution: scott.mclewin commented@RobRoy - an excellent point that I'd not even considered.
Comment #16
Dries CreditAttribution: Dries commentedThanks for the further analysis. Looks like this is RTBC then. I'll do that later when I'm back home. :)
Comment #17
Steven CreditAttribution: Steven commentedThis code has been affected by a recent security update. In Drupal 5, the strtolower() call was already added correctly... I updated HEAD to match.
Comment #18
Steven CreditAttribution: Steven commentedWait no, my CVS lost its tag again *grmbl*. Committed to 5 too.
Comment #19
scott.mclewin CreditAttribution: scott.mclewin commentedThanks. This marks my first patch to core, I'm glad it made it in.
Comment #20
RobRoy CreditAttribution: RobRoy commented@Scott It's a good feeling isn't it!
/me pats Scott on the back. :D
Comment #21
(not verified) CreditAttribution: commented