Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Drupal\Core\Utility\LinkGenerator calls SafeMarkup::set() which is meant to be for internal use only.
Proposed resolution
- Remove the call by refactoring the code. COMPLETED
If refactoring is not possible, thoroughly document where the string is coming from and why it is safe, and why SafeMarkup::set() is required.
Remaining tasks
- Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123 - COMPLETED
- Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it. - New test added and existing tests updated
If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.
Manual testing steps (for XSS and double escaping)
Do these steps both with HEAD and with the patch applied:
- Clean install of Drupal 8.
- Create a new menu link item, perhaps with HTML in the link text or double quotes in the URL
- Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping (and proper single-escaping).
User interface changes
N/A
API changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#31 | increment.txt | 2.89 KB | pwolanin |
#31 | 2501705-31.patch | 5.65 KB | pwolanin |
#30 | increment.txt | 830 bytes | pwolanin |
#30 | 2501705-30.patch | 5.44 KB | pwolanin |
#25 | increment.txt | 1.42 KB | pwolanin |
Comments
Comment #1
tetranz CreditAttribution: tetranz commentedComment #2
tetranz CreditAttribution: tetranz as a volunteer commentedComment #3
tetranz CreditAttribution: tetranz as a volunteer commentedComment #4
tetranz CreditAttribution: tetranz as a volunteer commentedPatch #2 was calling checkPlain unnecessarily for URL.
Tidied up comments.
Comment #5
dawehner80 chars exceeded ... :(
Comment #6
star-szrThanks @tetranz!
SafeMarkup::format rather than SafeFormat::format.
Also this is now longer than 80 character so should be wrapped per https://www.drupal.org/node/1354#drupal.
Maybe you tried this but I don't think we can concatenate in the attributes like this. Can it be passed in as a @token as well?
Comment #7
star-szr@dawehner jinx :)
Comment #8
tetranz CreditAttribution: tetranz as a volunteer commentedFixed the comments.
Changed attributes to a @token for consistency although perhaps not strictly necessary.
Comment #9
tetranz CreditAttribution: tetranz as a volunteer commentedComment #10
tetranz CreditAttribution: tetranz as a volunteer commentedComment #11
star-szrGoing to review this again.
Comment #12
joelpittetI think we can do these
checkPlain
removals. It will likely still escape the value correctly under most conditions and the URL is already Url encoded.Example test:
Something like this likely needs to be added to a test in LinkGeneratorTest. To ensure we don't have " breaking the href attribute.
Comment #13
tetranz CreditAttribution: tetranz as a volunteer commentedThanks. That's a good point about the double quote in the URL. I think it might break it but I will do a test.
It will be a few days before I get back to this.
Comment #14
joelpittetThanks for letting us know. For now I'll unassign you until a few days and please grab it again if it's not been completed by then.
Comment #15
tetranz CreditAttribution: tetranz as a volunteer commentedI have tests which I will upload tomorrow night.
Comment #16
tetranz CreditAttribution: tetranz as a volunteer commentedThis is my first ever attempt at creating a test.
I'm not sure if I've done the right thing but I also added a test to UnroutedUrlAssemblerTest to show that the url encoding works correctly. I think I needed this to be sure that my mock for the assembler is valid in my new test in LinkGeneratorTest.
Comment #17
tetranz CreditAttribution: tetranz as a volunteer commentedI messed up the interdiff. I think #17 is better.
Comment #18
tetranz CreditAttribution: tetranz as a volunteer commentedComment #21
xjmI marked #2502035: Document SafeMarkup::set() in testGenerateWithHtml() as a duplicate of this issue, since I'm fairly certain that's test coverage for the lines we're changing here. Either we add a comment to the test to document why that call was appropriate, or we change the test so that it's testing the exact right thing for this issue.
Based on what the patch here is doing, I think that the intent of the test is still to ensure that the
$safe_text
doesn't get escaped a second time when it's already in the safe list, so I think theSafeMarkup::set()
call in the test should still be retained and documented as an intentional unit test for the interaction between SafeMarkup and the LinkGenerator. See my comment on #2502035: Document SafeMarkup::set() in testGenerateWithHtml() for more background info.Meanwhile, couple of nitpicks I noticed while scanning this patch to decide what to do with the test:
Minor: there should be parens on method names in comments.
Minor: We should have a blank line between the one-line summary and the trst of the docs.
Comment #22
pwolanin CreditAttribution: pwolanin at Acquia commentedLet me re-roll this with those fixes.
Comment #23
pwolanin CreditAttribution: pwolanin at Acquia commentedI'm really not seeing why the added test case adds value. Certainly setting the url-encoded value as safe won't have an effect.
Also, href is just an attribute, so I think we can just more consistently manage it through the Attribute class.
Comment #25
pwolanin CreditAttribution: pwolanin at Acquia commentedtweak the code so href attribute is first.
Comment #26
xjmStill need the added inline comment on the existing
SafeMarkup::set()
call inLinkGeneratorTest
.Comment #27
xjmComment #28
pwolanin CreditAttribution: pwolanin at Acquia commentedSo, I don't see why the patch was calling SafeMarkup::set() in LinkGeneratorTest. My last patch doesn't have it.
Comment #29
xjm@pwolanin was a bit confused at my comments; we sorted it out in IRC. The added scope is to do with the existing
SafeMarkup::set()
in HEAD in the test.Comment #30
pwolanin CreditAttribution: pwolanin at Acquia commentedadding the test change from: #2502035: Document SafeMarkup::set() in testGenerateWithHtml()
[edit] Oops - that wasy the wrong thing
Comment #31
pwolanin CreditAttribution: pwolanin at Acquia commentedFix comments in test and elsewhere.
Comment #34
pwolanin CreditAttribution: pwolanin at Acquia commentedComment #35
xjmYay, that! Thanks. +1.
Comment #36
akalata CreditAttribution: akalata as a volunteer commentedThis patch removes one instance SafeMarkup::set, replacing it with SafeMarkup::format as part of a refactoring of how link item attributes (including the href) are generated.
There are also two new tests (one for external URLs and one for local) to ensure that the URL generator correctly escapes characters, and an update to an existing test to ensure that previously-sanitized output (for example, the text within the ) is not affected by the change. The use of SafeMarkup::set in this test case is properly documented.
I've updated the issue summary, and manually tested with both a menu link with HTML in the link text and double quotes in the URL, noting no change between HEAD and this patch.
Comment #37
xjmIt's great that we are able to get rid of two
checkPlain()
and anescape()
in addition to theset()
. Given how frequently the LinkGenerator is used, this patch might actually make a slight dent in the callstack and memory overhead in addition to being security hardening.Thanks @akalata also for the excellent review.
I discussed this patch in IRC with @alexpott as well. I am somewhat cautious about the use of
SafeMarkup::format()
to assemble HTML tags since used incorrectly it can give the illusion of sanitization while actually generating markup that is unsafe, but in this case it is safe, and it is internal enough to the core API that I think there is a low risk of contrib misusing the pattern seen here. The tag can only be<a>
, the attributes are safely assembled with theAttribute
class, and the link content is also sanitized because it is used with an@
placeholder.As a required part of a critical issue, this patch can be committed any time during the beta phase. Committed and pushed to 8.0.x. Awesome work!