Thanks!
I think it'd be pretty safe for me to commit this directly (and I've already discussed it with @poker10) but NR for now.
@arjun@pennywisesolutions this is not the right place to be asking for help with that. Please have a look for existing issues, file a new Support Request, or ask for help in Drupal slack.
I replied here to make sure that your comment didn't confuse anyone into thinking there was an unaddressed vulnerability.
This issue is closed and doesn't need more comments.
For posterity...
It's worth filing a followup for D7 and D11 to update to 1.5.0
D7 has merged the changes: 📌 Sync D7's copy of Archive_Tar with new 1.5.0 release RTBC
Issue for D11: 📌 Archive_Tar release 1.5.0 Active
Oh, and draft CR looks good, thanks.
I'm not aware of anywhere else that core might be affected by this, no.
We have some quasi-unit tests (which couldn't be actual unit tests as they involve using e.g. the temp directory which means Drupal has to be somewhat bootstrapped IIRC) in SystemArchiverTest (added in
#3102159: Add tests for Archive_Tar →
) if we wanted to add anything specific around this change.
I don't think we specifically need to in this case as we're just mirroring the upstream change, and existing tests cover core's actual usage - I think that's e.g. \UpdateTestUploadCase::testUploadModule which I only found after I'd added the unit(ish) tests mentioned above.
Tweaked the default settings comments on commit, as discussed with @poker10 in chat.
Thanks everyone!
Looks good, thanks.
Does any of this still apply to D10/11?
I'm a bit surprised that there's apparently no existing test of Form API's maxlength but indeed.. I don't see one.
This looks like a good change / BC layer, thanks all!
I thought this one seemed familiar when I started reading :) I suppose both @poker10 and I have written parts of the patch (/MR) so perhaps neither of us should commit it.
However, I think this is okay to commit on the basis that the change is to more cleanly handle the situation where Drupal is going to fail to load a valid controller class anyway. We have tests (thank you!) to prove that the code does what we expect, and multiple reports that (earlier versions) of this change solved (/ mitigated) the originally reported problem.
Also, it's sufficiently long ago that I hardly remember writing the code so it's like reviewing someone else's patch anyway :)
Anyone with a site that hits this error still has some debugging to do to, but it's an improvement on hitting unhandled Fatal errors.
Great - agree it's good to prevent warnings when bots etc.. hit this path.
Thanks!
This is really useful functionality; I've used a script / drush command that does something very similar many times to debug cron problems.
I definitely think that it should not be enabled by default on existing sites though, in order to avoid ballooning log volume.
It looks to me like the latest MR disables the functionality by default in the system module:
/**
* Detailed cron logging disabled by default.
*/
define('DRUPAL_CRON_DETAILED_LOGGING', 0);
...but the comment in default.settings.php suggests that the default is the opposite way around:
/**
* Cron logging.
*
* By default drupal_cron_run() will log each execution of hook_cron() together
* with the execution time. Set this variable to FALSE in order to opt out of
* this behaviour.
*/
# $conf['cron_logging_enabled'] = TRUE;
This should be simple to fix if we're agreed that we just need to make the settings.php stanza match (i.e. default to off).
Thanks - it's a bit circuitous but I think better to avoid potentially(/theoretically) referencing non-existent elements.
Thanks!
This looks good.
I thought I'd spotted a tiny problem in the test because the docs for \DrupalTestCase::randomName() say:
/**
* Generates a random string containing letters and numbers.
*
* The string will always start with a letter. The letters may be upper or
* lower case.
If that was literally true, the test may not always be doing exactly what we want it to.
However, the first letter will actually always be lowercase:
$str = chr(mt_rand(97, 122));
...so the problem there is slightly misleading documentation, rather than a bug in the new test here.
We could fix the docs in a follow-up, but it's not a top priority.
Thanks!
Thank you everyone!
This looks pretty good but I'm not sure about the assertions that reference an element on the return value of variable_get() - e.g.:
$this->assertEqual($file->uri, variable_get('file_test_results', array())['download'][0][0]);
Perhaps it's safe to assume that the variable will always be set and that element will be present, but if we do something similar in isolation:
$ drush php
Psy Shell v0.9.12 (PHP 7.4.33 — cli) by Justin Hileman
>>> print_r(variable_get('file_test_results', array())['download'][0][0]);
PHP Notice: Undefined index: download in phar:///usr/local/bin/drush8/vendor/composer/..eval()'d code on line 1
It may make the code more long-winded but can we avoid this?
CVE-2021-32610 applies to "Archive_Tar before 1.4.14" (emphasis added).
There are no vulnerabilities in 1.4.14 that are fixed in 1.5.0
Drupal core was updated to address the CVE in https://www.drupal.org/sa-core-2021-004 →
FWIW I am now a co-maintainer of Archive_Tar, and was the reporter of that (upstream) CVE.
It's worth filing a followup for D7 and D11 to update to 1.5.0 but doing so would not be a security release.
A dev release is up.
I'm not positive that all of the logic is perfect (e.g. placing generated forum content into containers and selecting parent taxonomy terms) but it seems to do a passable job if you just want to throw together an example forum structure.
Patches welcome, but I don't really anticipate doing more work on this.
I removed some trailing whitespace from a couple of lines on commit; I checked I'd not broken the file with a linter.
Thank you!
Less code is more good :)
Thank you!
Thanks everyone!
I don't think we need to jump through hoops to backport the test to D7.
However, per #2927012-33: _drupal_log_error() returns a 0 exit code on errors → it deserves a Change Record as it may cause a change in the behaviour of tests.
benjifisher → credited mcdruid → .
I tweaked the module name in the update hook per #13.
I've also changed it so that standard_install() only switches on the opt-out flag for the specific test class that otherwise fails. This meant adding a new element to $GLOBALS['drupal_test_info'] which feels a bit icky, but I think it's a reasonable compromise.
This means we're getting all tests to pass, plus testing the opt-out itself (within the profile if not the update hook) but all the other tests that use the standard profile will get the module enabled by default. That's good because it means we're more likely to catch any regressions involving the new module.
With the module enabled by default in the standard profile, we could probably tweak some of the tests in announce_feed_test.test where it shouldn't be necessary any more to explicitly enable the new module, but I don't think we need to do that immediately (and it shouldn't do any harm if we never make that change).
Erm, incidentally, should announce_feed_test.test be just announce_feed.test? Again, not an urgent thing to address.
Tests pass with that hack.
Manually testing the opt-out flag in settings.php
without:
$ ddev drush -y si && ddev drush pml | grep -i announcement
...snip...
Installation complete. User name: the-head-honcho User password: ...snip...
Core Announcements (announcements_feed) Module Enabled 7.100-dev
with the opt-out:
$ ddev drush -y si && ddev drush pml | grep -i announcement
...snip...
Installation complete. User name: the-head-honcho User password: ...snip...
Core Announcements (announcements_feed) Module Not installed 7.100-dev
I've also manually tested system_update_7087() with and without the opt-out variable set.
So the only remaining question for me is whether we can think of a cleaner way to achieve something similar to the check for the drupal_test_info global in standard_install()?
We also need to write the CR and Release notes snippet.
Per @poker10 the problem is that ModuleUnitTest::testModuleList() compares the list of modules from .info files with what module_list() reports has actually been enabled.
With any luck, the commit I just pushed should make the test pass, as we set the opt-out flag when the standard profile is being installed inside a test.
As an added bonus, this also tests the opt-out itself (at least in the install profile).
I would like to find a cleaner way of doing this though; I tried adding a \ModuleUnitTest::setUp() in which to set the opt-out variable, but that doesn't work because the parent setUp() method wipes $conf clean before installing the test site.
We could mess about more with setting a global there, and checking for it during the install in \DrupalWebTestCase::setUp but I'm not sure that'd be any better than adding the check in standard_install()?
Thanks!
@Fabianx suggested that the opt-out should also work for a fresh install, which seems reasonable but means we'll need a slightly different approach.
I've removed the entry from standard.info and put the same code that checks the opt-out in the system module's update hook into standard.install.
For a quick manual test, here are the relevant watchdog messages after running a drush site-install:
38 05/Mar 08:25 actions notice Action 'Unpublish comment' added.
37 05/Mar 08:25 actions notice Action 'Publish comment' added.
36 05/Mar 08:25 system info standard module enabled.
35 05/Mar 08:25 system info standard module installed.
34 05/Mar 08:25 system info announcements_feed module enabled.
33 05/Mar 08:25 system info announcements_feed module installed.
32 05/Mar 08:25 system info toolbar module enabled.
31 05/Mar 08:25 system info toolbar module installed.
The module seems to be enabled just before the list of dependencies in the .info file, which should be fine in this case.
I've also done a manual test to confirm that adding this to settings.php
$conf['announcements_feed_enable_by_default_opt_out'] = TRUE;
...means that the module is not enabled during installation.
I think it's preferable to do this in the standard install profile so that we don't install the module as part of the minimal profile, as that doesn't seem like it'd be appropriate.
I think it makes sense to enable announcements_feed by default in the standard profile, but not minimal.
I've pushed a commit for that to the MR.
As this is done via an entry in the .info file of the profile, the opt-out variable won't do anything there... I think that's okay though. It's really for existing sites to opt-out of the module being enabled by the update hook.
@Fabianx approved this in a different channel. However I won't mark it as RTBC just yet...
We need to commit the actual module first, and will then perhaps need to start a new MR.
@poker10 has also pointed out that we need to consider whether this is enabled by default for a clean install. I think it probably should be.
@Fabianx has approved this (and the opt-out) variable in a different channel.
I think that means it's ready to commit.
I think maybe just keep it really simple.
I've pushed a very basic update hook with a comment that looks like this via drush:
$ drush updb
System 7087 Enable the Announcements Feed module - see Release Notes for opt-out details.
I'd suggest that we don't add the opt-out variable to default.settings.php as this is a one-off situation; once the update hook has been run, the variable won't do anything.
So if this looks like it'll do the job, we just need to write up the CR and snippet for the Release Notes.
Thought about adding a link to the (currently draft) CR to the hook_update_N() comment to clearly signpost instructions for the opt-out - e.g.:
/**
* Enable the Announcements Feed module by default - see https://www.drupal.org/node/3424912 to opt out.
*/
function system_update_7087() {
// Allow opt-out.
if (!variable_get('announcements_feed_enable_by_default_opt_out', FALSE)) {
module_enable(array('announcements_feed'), FALSE);
}
}
However this ends up being output like this by drush:
$ drush updb
System 7087 Enable the Announcements Feed module by default - see https:www.drupal.orgnode3424912 to opt out.
..so that doesn't quite work.
I didn't mean to set this back to NR; I've only made small tidy up changes since @poker10 RTBC'ed in #91.
Back to RTBC; I think this is ready for a final Framework Manager review.
haha; wondered why a couple of $variable names didn't show up in those last commit messages.. it was exactly the same as https://www.drupal.org/project/cvslog/issues/563110 → - apparently I need more coffee.
Just noticed that we're creating the $user twice in the newest test.. I probably copy-pasted that from one of the other tests.
Should be an easy thing to tidy up before commit.
@poker10 I've hopefully addressed your latest round of feedback.
I've just pushed a new template_preprocess_announcements_feed() function similar to the one in aggregator.
Also noticed that the template should have a hyphen in the name rather than an underscore.
Tests pass with this change locally.. we should add at least one test that checks the sanitisation actually works though.
I think there's one more item in the feedback to address; no idea why we've ended up with tabs in a file. I'll work out whether to fix it in vim or emacs :)
Good question on security / sanitisation. Of course we'd hope that we can trust content from this source, but we all know what assumptions lead to.
The aggregator module is hopefully a good reference; here's where it sanitises feed items:
https://git.drupalcode.org/project/drupal/blob/7.99/modules/aggregator/a...
It doesn't feel ideal to duplicate aggregator_filter_xss() but we can't rely on that module being enabled.
Do we need an allow-list like that for filtering, or would using defaults work?
I believe I've addressed all of the outstanding feedback in the MR, including my own :)
@mitthukumawat not sure if you're still working on this, but I have a little time today so am assigning it to myself to attend to the most recent feedback.
Thanks for the offer - I've added you as a maintainer.
This new feature makes me slightly nervous, but I can see why it might be a pragmatic solution in some cases.
I'm pleased to see that there's logging, a specific permission for it, and that it's not enabled by default.
It'd be great if you can do some housekeeping on the module, including getting it setup for properly for gitlab.
Thanks again.
Interesting suggestion, thanks.
I think this looks good.
I'd like to see a couple of tweaks:
'#description' => t('Example: %url or %path or in early testing js.cookie and D9 shim with %jscookie', array(
- Let's drop "early testing"; I'd be more comfortable saying "experimental"?
- Can we provide a link to explain more about the shim - perhaps to: https://www.drupal.org/node/3104677 →
- I am a bit nervous about this:
if (strpos( $custom_path, 'js-cookie')){
// if js.cookie is being used to replace jquery.cookie 1.4.1, then use this shim from a Drupal 9.5.10 environment
$libraries['cookie']['js']['misc/jquery.cookie.shim.js']['data'] = $path . '/replace/ui/external/jquery.cookie.shim.js';
}
(nit - there's an extra space before the variable with the if condition.)
It's probably okay to check for this string in the custom path, but that could easily go wrong e.g. if someone stores a local copy of the library and renames it without that exact pattern.
If we're going to rely on the presence of that string, let's be explicit about it (in the admin UI), or let's add a checkbox instead to indicate that the shim should be included... that might also give us some space in the admin UI to better explain what this is all about.
Finally, it'd be great to see some additional testing of this option being used.
Thanks for the patch/MR.
Can we add a test which demonstrates what problem we're fixing here please?
Thanks for the report.
It'd be good to have a patch/MR (with tests) to review.
Sorry for the long delay.
It'd be good to fix this; especially for jQuery Migrate which - as you mention - is likely to be used by a lot of the installs of this module.
Writing out to a file sounds like it'd make sense.. presumably that'd typically go into an aggregated bundle.
If the JS snippet is relatively static, could the module provide the file itself rather than generating an ephemeral one?
Are there examples of other modules addressing this same issue?
Are you still having this issue?
Did you get any further investigating it?
Are you able to provide steps to reproduce?
I'm struggling to think of anything unusual that this module does which would affect something like this.
Sorry for the very slow reply, but yes - please feel free to update tests if you can to show definitively whether a leading slash is required, or conversely does more harm than good.
Alternatively, we could treat this a documentation issue?
..and/or tweak the admin UI so that it adjusts the examples / does validation if it detects that Drupal's running in a subdirectory.
I'm open to suggestions about how to improve this for the subdirectory case, but we need to ensure nothing's broken for the (majority) case of Drupal not being in a subdirectory URL-wise.
If we're going to make this change (mostly to reduce noise from scanners like the one you mention), let's update all of the places where there's a similar URL - e.g.:
jquery_update.admin.inc:171: '!url' => 'http://github.com/jquery/jquery-migrate/#readme',
jquery_update.module:18: '@jquery' => 'http://jquery.com',
jquery_update.module:84: 'website' => 'http://docs.jquery.com/Plugins/bgiframe',
jquery_update.module:443: 'website' => 'http://plugins.jquery.com/migrate',
jquery_update.module:550: 'website' => 'http://jqueryui.com/demos/menu/',
jquery_update.module:561: 'website' => 'http://jqueryui.com/demos/spinner/',
jquery_update.module:572: 'website' => 'http://jqueryui.com/demos/tooltip/',
I'm guessing that part of the motivation for not specifying defaults in calls to variable_get() is to avoid repetition / duplication.
In D7 constants are quite often used for variable defaults, so that the actual default value is set in one place - for example:
modules/system/system.module:16:define('DRUPAL_CRON_DEFAULT_THRESHOLD', 10800);
modules/system/system.admin.inc:1645: '#default_value' => variable_get('cron_safe_threshold', DRUPAL_CRON_DEFAULT_THRESHOLD),
modules/system/system.module:3577: if (($threshold = variable_get('cron_safe_threshold', DRUPAL_CRON_DEFAULT_THRESHOLD)) > 0 && variable_get('install_task') == 'done') {
modules/update/update.module:17:define('UPDATE_DEFAULT_URL', 'https://updates.drupal.org/release-history');
modules/update/update.fetch.inc:335: return isset($project['info']['project status url']) ? $project['info']['project status url'] : variable_get('update_fetch_url', UPDATE_DEFAULT_URL);
Wow, thanks again for all the work that's gone into this.
I am pretty happy with how it's all looking, but there's one thing that's made me raise an eyebrow.
function announcements_feed_install() {
// Creating the default values.
variable_set('announcements_feed_max_age', 86400);
variable_set('announcements_feed_cron_interval', 21600);
variable_set('announcements_feed_limit', 10);
variable_set('announcements_feed_json_url', 'https://www.drupal.org/announcements.json');
variable_set('announcements_feed_link', 'https://www.drupal.org/about/announcements');
}
I'm not keen on the pattern of setting the defaults for variables in hook_install() and then not specifying a default in the calls to variable_get().
In D7, I'd expect to be able to delete a variable (e.g. drush vdel announcements_feed_cron_interval) to remove any customisations that have been made to it, and for that to have the effect of restoring the default.
In fact, that's a bad example because there is a default in hook_cron() for that particular variable, but there are other places where there's not, e.g.:
$announcements_feed_json_url = variable_get('announcements_feed_json_url');
$response = drupal_http_request($announcements_feed_json_url);
$build = array_merge($build, array(
'#theme' => 'announcements_feed',
'#count' => count($announcements),
'#feed_link' => variable_get('announcements_feed_link'),
));
There are examples in D7 core of calls to variable_get() that don't specify a default, but I'm not sure I can see a good reason for using that approach here?
The D7 maintainer team have discussed this a bit.
Another semi-relevant example that came up is the case of JetBrains enabling their AI assistant by default, and some users being quite vocally aggravated about not being able to cleanly disable / remove the feature. There are threads about this on reddit / hacker news.
@fabianx and I both like the suggestion of enabling the module by default, but having an opt-out variable documented in the release notes.
We can canvass a few other opinions, but I think that's pretty close to a consensus on the approach.
Interesting.. thanks for filing this.
The idea would be that jQuery Update now supports bringing in any version of jQuery (and associated libraries) via custom paths.
However, looks like there are quite a lot of changes in this major version, including lots of deprecated APIs being removed.
So seems pretty likely that some BC/shim code would be necessary.
Sadly there's no automated JS testing in D7 so it'd be a manual effort to figure out what would break.
I personally am not going to be able to devote much/any time to this, but patches welcome.
The beta release notes linked to in the IS mention that:
The jQuery Migrate plugin will also be ready to assist.
So if we're lucky there won't be anything specific to do for D7 - sites would just need to pull in an appropriate release of jQuery Migrate, which jQuery Update can do - but we'll have to see how that turns out.
There is of course the option of just sticking with jQuery 3 on D7 sites.
Drupal maintainers have been discussing what the jQuery 4 release will mean for jQuery 3 EOL with the jQuery maintainers for some time in:
https://github.com/jquery/jquery/discussions/5001
...and it sounds like jQuery 3 is likely be supported beyond the EOL of D7.
Erm, oops - an interdiff is not a patch; apparently my muscle memory forgot.
Everything else looks okay though.
@TR are you happy with this patch?
Reverting just the base64-related change from #35, along a tiny tweak to comment wording.
Test-only is unchanged.
Updated IS to show manual testing in the 5 targeted browsers is done (between #50/#53).
Yeah a follow-up would be a good idea if we're going to add the hook.
Test only patch, and a little cleanup.
Oops - there's a bit of copypasta and other mess left over in the patch; I'll tidy it up if we decide to proceed in this direction.
Depending on how we proceed, we might want a new followup issue.
Therefore I've not created an MR, but here's a quick patch with one suggestion for how we could handle this issue.
Rather than making a(nother) potentially impactful change within D7's core filtering, we could add a drupal_alter() which would allow contrib/custom modules to make changes required for e.g. better unicode support.
This patch includes a test based on the examples provided in the previous few comments.
benjifisher → credited mcdruid → .
I think my motivation for adding the comment was to avoid anyone (including myself later on) thinking that we needed to add more code / dependencies to the test in order to provide more complete user objects.. per the vintage comment in the API docs which confirms that drupal_anonymous_user() doesn't return a full user object, or invoke hooks etc.. etc..
https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/drup...
I'd be happy with a tweak to the wording, or for the comment to be removed on commit if we think it's not worth keeping.
The parent has been committed to D10/11 - I think we're okay to commit this to D7 now, but I don't want to commit my own patch/MR.
One tweak I might suggest is to use the word "minimal" instead of "lightweight":
// Lightweight user objects are sufficient.
$user = drupal_anonymous_user();
We don't really need to use objects at all as the properties are passed as individual params, but I think I'm happy with the rest of the test.
@poker10 are you happy to commit this (perhaps changing the comment as above)?
Thanks; thought I'd tried that but I must have been doing something wrong..
Unpublishing this public issue for now as requested by @lleber because it may need to be handled in the private security tracker.
Added a draft CR based on the one @poker10 made for the parent issue:
The test-only job failed as expected:
https://git.drupalcode.org/project/drupal/-/jobs/506876
Reset password 284 passes, 1 fail, 0 exceptions, and 88 debug messages
Test run duration: 22 sec
Detailed test results
---------------------
---- UserPasswordResetTestCase ----
Status Group Filename Line Function
--------------------------------------------------------------------------------
Fail Other user.test 960 UserPasswordResetTestCase->testUniq
No user_pass_rehash() hash collision for different users with no stored
password.
Same as the parent issue, this will need a CR - any "in flight" reset links will become invalid when the code changes, but they are short-lived by design.
Anything else?
Added a similar test to the one we're working on in the parent issue.
It could perhaps be a unit test, but not sure how reliable that would be as user_pass_rehash() calls drupal_get_hash_salt() which may use the $databases global etc..
Ah, interesting thanks.
The test only job seems to have passed, but also shows the expected failure:
https://git.drupalcode.org/project/drupal/-/jobs/506479
Testing Drupal\Tests\user\Kernel\UserPassRehashTest
F 1 / 1 (100%)
Time: 00:00.568, Memory: 4.00 MB
There was 1 failure:
1) Drupal\Tests\user\Kernel\UserPassRehashTest::testUniqueHashNoPasswordValue
Failed asserting that 'VxdDGWYOnSlfWVeVDwheEkF38Avnca_EsLcz3fdfMm4' is not equal to 'VxdDGWYOnSlfWVeVDwheEkF38Avnca_EsLcz3fdfMm4'.
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/builds/project/drupal/core/modules/user/tests/src/Kernel/UserPassRehashTest.php:50
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 1, Assertions: 6, Failures: 1.
Do we need anything else in the MR?
I'd think this will need a CR; apart from anything else any reset URLs that have been sent out but not yet used will become invalid (although they only last for 24hrs by default IIRC).
Right, good plan.
Added a basic KernelTest that illustrates a possible hash collision when there are empty passwords in the db.
I've manually verified that the test fails without the change to user_pass_hash().
Sorry, I've not kept up with how we're doing (the equivalent of) test-only patches these days, but I'll try to find out in the next couple of days.
More tests are always good, but I'm not sure what additional tests we'd add here.
There are already tests which cover functionality where user_pass_rehash() is used to construct URLs - e.g.:
- \Drupal\Tests\user\Functional\UserPasswordResetTest
- \Drupal\Tests\user\Functional\UserCancelTest
The idea of the improvement being suggested is to reduce the chances of a successful brute force attack, and that's not necessarily an easy thing to test.
Open to suggestions of what testing we could add, but I think we may have to be satisfied that existing tests prove this change is not introducing a regression.