Problem/Motivation
Drupal\Core\Database\Install\Tasks::runTasks() calls SafeMarkup::set() which is meant to be for internal use only.
Proposed resolution
Remove the call to SafeMarkup::set()
by refactoring the code.
The installer uses TaskException to communicate errors to the settings form. It's adding translated messages to the exception and then it trying to list multiple messages. We should not be translating exception messages and all the text should be translatable (it's not currently). The latest patch refactors the installer db tests to not use TaskException since it is completely unnecessary and adds complexity where it is not needed.
Remaining tasks
Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123Identify 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.Note: The task messages are fixed strings in code, not user inputIf 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:
- In core/lib/Drupal/Core/Database/Install/Tasks.php on line 42 change
'CREATE TABLE {drupal_install_test} (id int NULL)',
to a SQL comment'-- CREATE TABLE {drupal_install_test} (id int NULL)',
- Attempt a clean install of Drupal 8 via the UI installer.
- Compare the output of the error messages in HEAD and with the patch applied. Confirm that there is no double-escaping in the error messages and that all the $messages (there should be 4 errors in addition to the intro text) are displaying.
User interface changes
Database fail error on head
Database fail error with patch
API changes
Remove TaskException
Remove db_run_tasks()
Beta phase evaluation
Issue category | Bug because the installer is using translated messages in exceptions and SafeMarkup::set() |
---|---|
Issue priority | Major because it is a child of the SafeMarkup critical |
Prioritized changes | The main goal of this issue is security as SafeMarkup::set() should not be used. |
Disruption | Potential disruption for alternate database drivers (there are very very few). @chx has confirmed no disruption for Mongodb. |
Comment | File | Size | Author |
---|---|---|---|
#72 | 2501835.72.patch | 13.15 KB | alexpott |
#72 | 65-72-interdiff.txt | 395 bytes | alexpott |
#65 | 2501835.65.patch | 13 KB | alexpott |
#65 | 62-65-interdiff.txt | 10.92 KB | alexpott |
#63 | 2501835-62.patch | 6.75 KB | stefan.r |
Comments
Comment #1
star-szrWorking on this at NH Dev Days 2.
Comment #2
star-szrRenaming
$message
to$failure_message
for the initial usage makes this easier for my brain to understand, at least right now.Comment #3
joelpittetThis looks good. There is an extra space in the message and removed the unused $message = ''; initialization.
Comment #4
alexpottThis code looks weird as it will only report one error when there are several... The d7 version of this code looks like this:
I think we should fix this here too ... which means this is a bug. Hmmm that said this was discussed in #2374847: Only last one of the database settings form errors is being printed and decided that this behaviour is correct - I'm not sure I'm convinced by the argument there. What is for sure is that we're missing a space between after the fullstop...
hosting provider.@failure_message
. I'm not entirely sure that removing the paragraph with the error class was correct either.Comment #5
kgoel CreditAttribution: kgoel at Forum One commentedworking on this
Comment #6
kgoel CreditAttribution: kgoel at Forum One commentedI went ahead and replaced the code how it has been done in D7
I am not sure why $message was rename to $failure_message so reverted that in my patch but if $failure_message makes more sense in readability then sure.
I am not sure how to test this - I tried to drop db and installed site from ui and command line but I don't think thats enough to trigger any db error.
Comment #7
seantwalshWorking on testing the latest patch.
Comment #8
seantwalshBelow are the steps to reproduce testing, as well as the various screenshots documenting the changes.
'CREATE TABLE {drupal_install_test} (id int NULL)',
to a SQL comment'-- CREATE TABLE {drupal_install_test} (id int NULL)',
HEAD:
Only prints the last $message. Uses SafeMarkup::set() & SafeMarkup::isSafe().
PATCH #6:
Prints all $messages but valid markup is escaped. Uses SafeMarkup::format() instead of SafeMarkup::set() and removes SafeMarkup::isSafe().
UPDATED PATCH:
Prints all $messages and properly renders the markup. Removes SafeMarkup::format() from previous patch and replaces it with Xss:filterAdmin().
Comment #9
seantwalshUpdated issue summary to reflect Manual testing steps. Also, removed the "If there is any user or calling code input in the string, submit
alert('XSS');and ensure that it is sanitized." step since the output is all coming from Tasks.php and not user or calling code input.
Comment #10
seantwalshComment #11
pwolanin CreditAttribution: pwolanin at Acquia commentedComment #12
seantwalshSilly typos and me uploading the wrong patch!!! Here's the patch that works.
Comment #13
akalata CreditAttribution: akalata as a volunteer commentedI've applied the patch and was able to confirm that HEAD does not display more than one error message on that step of the install, and that #11 displays all the error messages without double-escaping.
One nit-pick I have is that the multiple error messages run together without even a space after the previous period (see below). Personally I would prefer more of a break than that, but I don't know what the standards are when displaying multiple error messages.
Comment #14
seantwalshThis version fixes the running together adding each message as a new paragraph for better readability.
Comment #15
seantwalshSorry had to fix an error I left in. :(
Comment #17
akalata CreditAttribution: akalata as a volunteer commentedI have applied this patch and tested using the manual testing steps described in the issue summary. A review of the patch itself notes only the necessary changes.
Comment #18
xjmSo @alexpott and I discussed this and we agreed that Xss::filterAdmin() is actually not the best approach here. In HEAD, the unsafe message text is
checkPlain()
ed. This patch loosens that sanitization. It also adds things to the call stack for error handling which is a risk.This patch is now a case where we're joining an indeterminate number of messages. I was going to suggest an item list, but @alexpott said that would be problematic in this case. Postponing on #2501975: Determine how to update code that currently joins strings in SafeMarkup::set() for now.
Comment #19
alexpottI've just tried to prove why I had concerns about the item list idea - I thought this would cause nesting in a very small space but I'm not sure I'm right.
Comment #20
xjmAh so, re: #19, if someone can get it working with an item list render array that'd be great. But if not it can stay postponed.
Comment #21
pwolanin CreditAttribution: pwolanin at Acquia commented@xjm - I don't know that the render system is available if during these early errors? If you want a lit, I think we'd just hard-code the markup.
Comment #22
kgoel CreditAttribution: kgoel at Forum One commentedYesCT and I worked on it together. In the head in case of database fail during site install, it was displaying only the last error. This patch fixes that problem and it will display all the errors if database fails. There is no interdiff because this is a new approach per @xjm in #20
Database fail error on head
Database fail error with patch
Comment #23
xjmHuh, interesting that that does work. And the code is definitely far cleaner and more readable this way. Plus the screenshots look great.
The only concern to discuss is whether it's a good idea to add the dependency to the renderer service here:
...because I have no idea how the installer normally renders things. Per @alexpott, the installer uses the maintenance theme (which is a stripped down version of everything)... This is different from #2505497: Support render arrays for drupal_set_message() because there the render service will eventually be used normally when the page is rendered with your typical
drupal_set_message()
. But in general the installer builds a lot of things manually.Meanwhile, since this is a bug (with HEAD discarding most of the messages), we should add test coverage for the multiple messages.
Comment #24
YesCT CreditAttribution: YesCT commentedyep, helped code up #22. :)
Comment #25
xjm(Updating the issue credit.)
Comment #26
xjmOh, can we also get an automated test for sanitization? Because I'm still not sure Twig autoescape is sanitizing the way it does during normal site operation.
Comment #27
lauriiiComment #28
kgoel CreditAttribution: kgoel at Forum One commentedI am working on it.
Comment #29
kgoel CreditAttribution: kgoel at Forum One commentedTwig autoescape sanitizes the stuff but in this case I am not sure if twig sanitize on install. I think we don't need sanitization on install.php page since "we" can assume that the person installing the site has admin permissions. I don't think I need to do any work on #26but reading #23 now.
Does anyone have opinion on my comment above?
Comment #30
kgoel CreditAttribution: kgoel at Forum One commentedI have looked into /core/tests/Drupal/Tests/Core/Database/ConnectionTest.php but I am not entirely sure if that is the correct test file where i should be writing test for displaying multiple database error messages. /core/tests/Drupal/Tests/Core/Database/ConnectionTest.php seems relevant for core/lib/Drupal/Core/Database/Connection.php class.
I have created a follow up issue for adding test coverage since we discovered bug in HEAD while fixing SafeMarkUp issue so seems like a good idea to fix the test in the follow-up issue.
Follow up - #2539902: Add test coverage for Tasks.php
Comment #31
davidhernandezIt would seem not necessary to test for sanitization during the installer since there is no user entered text. Only test supplied by code or entered by the admin running the installer.
Whether a test to count the correct number of database error messages should be included with this issue is a different question. I'm inclined to say if it was fixed here the test should be added here.
Comment #32
kgoel CreditAttribution: kgoel at Forum One commenteddawehner said \Drupal\system\Tests\Installer\InstallerExistingDatabaseSettingsTest is an example which deals with posting to that form and asked to add a new test class extending \Drupal\simpletest\InstallerTestBase. The new class InstallerNotValidDatabaseSettingstest should live - Drupal\system\Tests\Installer\ and in the setupSettings() method just pass in random db usernames and passwords.
Comment #33
kgoel CreditAttribution: kgoel at Forum One commentedAdded empty test class InstallerNotValidDatabaseSettingstest.php
Comment #35
kgoel CreditAttribution: kgoel at Forum One commentedI tried to run test locally but I am not sure if it's caught in loop as it never completes.
Comment #37
kgoel CreditAttribution: kgoel at Forum One commentedRenamed old test class to new test class and fixed the dependency.
Comment #39
davidhernandezWith patch applied I get an exception.
Comment #40
davidhernandezI don't know why but it definitely seems to be the render line.
Comment #41
kgoel CreditAttribution: kgoel at Forum One commentedI have used renderPlain() but I am still getting the messages with HTML output.
Comment #42
davidhernandezIf renderRoot or renderPlain are used the problem goes away but the resultant markup is escaped.
So this thing - #2450993: Rendered Cache Metadata created during the main controller request gets lost
I think we need to do one of these maybe?
Even when I set this up, and get it to work with render(), the markup is still escaped.
Comment #43
kgoel CreditAttribution: kgoel at Forum One commentedI am not sure why to use executeInRenderContext() since renderPlain() or renderRoot() gets rid of the error below.
Although, none of the calls renderPlain(), renderRoot() or executeInRenderContext() resolves safe markup issue on the database error page. When I worked on this patch last month #22 then database fail messages were not escaped so I am wondering what else has changed in HEAD beside using \Drupal::service('renderer')->renderRoot() or \Drupal::service('renderer')->renderPlain() instead of \Drupal::service('renderer')->render() .
Anyone has any idea?
Comment #44
davidhernandezRender context is empty, because render() was called outside of a renderRoot() or renderPlain() call
That is what the error is. It doesn't have context. renderRoot and renderPlain work because the way is providing context.
Comment #45
kgoel CreditAttribution: kgoel at Forum One commentedI have used rednerPlain() after getting error which is in #43comment but now I am getting escaped database error markup. I am going to look into escaping issue now and try to figure it out why renderPlain() is causing escaped markup.
Screenshot with escaped markup after using renderPlain()
Comment #47
kgoel CreditAttribution: kgoel at Forum One commentedI reproduced the database error on my local. Followed the following steps -
1. Applied #45 patch on local branch
2. sudo rm -r sites/default
3. git checkout -- sites/default
4. SQL comment on line 42 Drupal\Core\Database\Install\Tasks.php
'-- CREATE TABLE {drupal_install_test} (id int NULL)',
5. localhost/install.php, chose language, standard install
6. On database configuration form added correct database username and password
7. Noticed all the database errors but they are all escaped in the screenshot below
-------------------------------------------------------------------------------------------------------------------------------------------------
In my test class, I am passing 'password' => $this->randomMachineName(), and I tried to enter random password on database configuration form but I get different error than with changed behavior in the patch which makes me think that the test is not correct.
Screenshot of database error with random database password
Comment #48
YesCT CreditAttribution: YesCT commentedok. so that means that the test that we have been working on, with the random database password, is not adding coverage for the change in this patch.
we should
a) add that test coverage elsewhere
b) change the test here to test for the change from head: additional error messages
--- but. maybe because of the way we are testing this, by adding a -- comment in front of the database create command, it is not realistic that any actual user would get multiple errors, and we dont need the change in the patch to show multiple errors and dont need the additional test coverage for that here.
I think we need to try and get a way through the UI for a user to make one error message in head, which would have multiple with the patch. and if we cannot do that, we dont need the test coverage, or the change to show multiple.
Comment #49
davidhernandezThe test isn't working because kgoel didn't finish writing it. The main problem is the functional change is not correct. renderPlain() will escape the markup in the error message, which we don't want.
Comment #50
kgoel CreditAttribution: kgoel at Forum One commentedIts really confusing. I tried using renderRoot but that also escaped the markup so I am not sure what to use. I don't think we want to use executeInRenderContext() but I could be wrong.
I didn't finish working on because database errors are escaped now and also, test case doesn't seem the right place as was suggested in #32
Comment #51
xjmMinor: The name of this variable is misleading as it's actually a plural collection of errors from the results.
Something is not quite right here... >renderPlain() does not do anything with that second argument.
That wouldn't have anything to do with the failure and double-escaping, but it is worth noting.
Comment #52
joelpittetHaven't added anything to the tests but this should clean things up a bit on the fix end. I opted to just use the t() but render out the generated markup with the renderPlain().
@kgoel maybe you can ping me on your intentions on the test and how I can help you get it there.
Comment #53
stefan.r CreditAttribution: stefan.r commented#52 addresses the points mentioned in #51, I manually tested it and it seems to display the errors correctly.
It just needs tests now, I'll see if I can update the test scenario.
Comment #55
alexpottSee how to test a broken installer in #2156401: Write install_profile value to configuration and only to settings.php if it is writeable. We need to add an empty
setUpSite()
method and a test method to assert the output.Comment #56
stefan.r CreditAttribution: stefan.r commentedTest added, I would have liked to test for multiple errors but I would need to modify the list of tasks for that, which I'm not sure we want to do.
Right now the task run considers failure of the table creation task fatal (as it should) so it doesn't go on to the next tasks. And there's no easy way to make the insert/update/delete/drop fail consistently on mysql, pgsql and sqlite without first creating a table.
For what it's worth, we may have an actual use case for the multiple error messages in #2529188: Provide better error handling for MySQL client and server utf8mb4 incompatibility, but for now we may have to follow the manual testing steps to confirm this actually displays multiple error messages.
Comment #57
stefan.r CreditAttribution: stefan.r commentedThe actual test was missing there.
A bunch of empty methods might have also worked, instead I added a flag to make setUp() do an early return.
Comment #58
joelpittetYup fails with tests only and that seems to be a nice way to cleanup the tests to make them easier to write for this circumstance. Thanks @stefan.r
Comment #59
alexpottI don't think this is necessary. Just override the methods as necessary. You could be testing errors at any point of the installer. The empty methods in the concrete implementation nicely document the expectation of not reaching these steps.
We should add a comment saying that we are creating a table here to force an error in the installer because it will try and create the drupal_install_test table as this is part of the standard db tests performed by the installer in Drupal\Core\Database\Install\Tasks.
Move the assert to the test method as assertTrue(TRUE) is completely not descriptive.
Comment #60
stefan.r CreditAttribution: stefan.r commentedComment #62
stefan.r CreditAttribution: stefan.r commentedComment #63
stefan.r CreditAttribution: stefan.r commentedComment #64
joelpittet@stefan.r I don't think those two fails are this patches fault, I was trying to determine as much and I ran them on an earlier patch in #52.
To me the only one that can let us know is @alexpott on this one, so I'm RTBCing because the comments were addressed regarding the tests.
Comment #65
alexpottI don't really think this way of appending the errors into a translatable string is really correct. Reviewing what the installer is doing here I've realised that the whole TaskException is quite bogus we just need to return the errors to the caller. And in the caller we can add the overall message and the list of errors together.
I've added tests to prove that translating these strings works as expected and reworked the changes to InstallerTestBase to do less in setUpSite().
Given the db drivers are extremely rare I think removing TaskException should be ok but I'll ask @chx to give this a look over in case a db driver maintainer has a different perspective.
Comment #66
joelpittetWhy this change? Gabor was in past issues had some concerns with the http://localize.drupal.org/ being able to parse or having to parse another syntax inline.
May need to get Gabor to review.
Comment #67
alexpott@joelpittet inline templates are supported by l.d.o this has been discussed before. Dumping a whole item list on the end of a t() is way worse for translators because they have no idea what
@errors
is in'Resolve all issues below to continue the installation. For help configuring your database server, see the <a href="https://www.drupal.org/getting-started/install">installation handbook</a>, or contact your hosting provider.@errors'
Comment #68
joelpittet@alexpott that is good to know, I thought that was an outstanding issue that never got resolved regarding l.d.o. Fewf.
Looking again at where the errors placed... it's outside the
trans
block (read it as the same as the t() implementation the first time). So I'm good with that, thanks for the quick reply.That's quite the scope change on this patch, just because testing installer is weird. It's a bit beyond me at this point but the inline_template solution is ok with me here. Like you mentioned let's get db maintainer to have a peek.
Comment #69
chx CreditAttribution: chx commentedWhy, yes, MongoDB is a DB driver and the only class that actually does something is the install Tasks class. However, it does not throw TaskException so we are good there.
If you'd like a more generic answer, I absolutely have no problems on a task runner needing to catch its exceptions, it's very early, you can't expect anything from the system at this point.
Comment #70
alexpottFilled out a beta evaluation and the issue summary because the scope has expended.
Comment #71
stefan.r CreditAttribution: stefan.r commentedChanges in #65 look good to me, maybe @Crell can have another look at this as it's similar to what we did earlier in #2529188: Provide better error handling for MySQL client and server utf8mb4 incompatibility, as well as chime in on the API changes
Nit: unused use statement
Comment #72
alexpottRemove the unused use.
Comment #73
catchNot 100% comfortable with Twig and #theme here compared to not having it, but also I don't think it's adding a dependency we don't already have.
Won't this test fail if the translation is changed on l.d.o?
Comment #74
alexpott@catch:
Tentatively setting back to rtbc.
Comment #75
catch#1 and #2 both make sense.
Committed/pushed to 8.0.x, thanks!