πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
Account created on 9 March 2008, about 16 years ago
  • Senior Principal, Security Operations at AcquiaΒ 
#

Merge Requests

Recent comments

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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!

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Less code is more good :)

Thank you!

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Thanks everyone!

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

mcdruid β†’ created an issue.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

mcdruid β†’ created an issue.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

added 7.100

πŸ‡¬πŸ‡§United Kingdom 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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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()?

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Thanks!

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

@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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

@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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

@Fabianx has approved this (and the opt-out) variable in a different channel.

I think that means it's ready to commit.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

mcdruid β†’ created an issue.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

@poker10 I've hopefully addressed your latest round of feedback.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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?

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

I believe I've addressed all of the outstanding feedback in the MR, including my own :)

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

@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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Thanks for the patch/MR.

Can we add a test which demonstrates what problem we're fixing here please?

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Thanks for the report.

It'd be good to have a patch/MR (with tests) to review.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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?

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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/',
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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);
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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?

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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?

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Reverting just the base64-related change from #35, along a tiny tweak to comment wording.

Test-only is unchanged.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Updated IS to show manual testing in the 5 targeted browsers is done (between #50/#53).

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Yeah a follow-up would be a good idea if we're going to add the hook.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Test only patch, and a little cleanup.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
πŸ‡¬πŸ‡§United Kingdom 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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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)?

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Thanks; thought I'd tried that but I must have been doing something wrong..

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Unpublishing this public issue for now as requested by @lleber because it may need to be handled in the private security tracker.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Added a draft CR based on the one @poker10 made for the parent issue:

https://www.drupal.org/node/3409960 β†’

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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?

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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..

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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).

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

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.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

mcdruid β†’ created an issue.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Adding credit from the private DST discussion - which is where the idea for this MR came from.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

This issue was discussed by the Drupal Security Team.

It was agreed that it can be handled in public as a security hardening; we can use this issue.

The problem is that there is significantly less entropy in the inputs to user_pass_rehash() if the user has an empty password field in the database.

Users may have empty passwords if - for example - a site uses a Single Sign On integration to offload authentication.

Improvements could be made to the implementation to reduce the likelihood that an attacker could brute force e.g. a password reset URL if the user has no password stored in Drupal.

It may also be advisable for sites / projects that do not use Drupal's passwords for authentication to avoid leaving the password field empty in the database, but that can be looked at in a (public) follow-up. It wouldn't help much to set the password field to the same value for all users.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Adding some more tests.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Changed my mind about the default for padding; I think it should be enabled.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί
πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

mcdruid β†’ created an issue.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Looks like this can be "Closed (won't fix)" as there are no plans for further D9 releases (and it's not obvious what we'd do to fix it without changing minimum PHP requirements or adding ugly workarounds).

Another reason not to stay on D9.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Keeping the threshold as it is.

There's a hook_update_N() in the patch above if we do decide to change the default later.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

This patch produces the following list of examples:

        (
            [qwerty] => 10584572
            [12345] => 2591854
            [monkey] => 1422866
            [admin] => 276638
            [cheese] => 270587
            [changeme] => 147401
            [dictionary] => 13630
            [!@#$%^&] => 1430
            [correcthorsebatterystaple] => 232
            [drupal] => 129
            [joomla12345] => 40
            [phprules] => 30
            [1wordpress] => 6
            [ohnoes!!] => 1
        )

...and increases the threshold to 250.

Actually, thinking about it... maybe we don't want to change the threshold and 10 is okay.

Let's keep the new examples though.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

mcdruid β†’ created an issue.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Tweaked the wording of messages a little depending on whether this new option is set.

Thanks for the patch; definitely an improvement!

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Here's a couple of basic tests.

These reveal the fact that with the "check_failed_login" option enabled, the module may emit messages saying:

Your current password appears in ...

...when it wasn't really the current, stored password that was checked.

I don't think this is an especially big deal, but we could tweak that wording.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Sounds good, thanks!

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Thanks - adding the test made this easy to commit.

Still needs the CR.

Is this a problem in D10?

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Thanks; great that we added a test.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Thanks everyone!

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Thank you everybody that contributed to this.

Very hard to get credit perfect on an issue that's almost old enough to drive a car (in some places) with well over 100 comments; apologies if I've made mistakes.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Having another go at committing #20 again (without reversing it this time).

With any luck that should address some of the issues people have experienced in D7 (and seems fairly low risk).

Per #174 let's please not re-open this issue (it was me that reopened it last); if anything else needs to be looked at, let's do so in a follow-up that's specific to the relevant Drupal version.

Not much chance I've got credit perfectly correct in this issue; apologies if I missed anyone.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Excellent, thanks for adding the test.

This reminded me that I wanted to have a look at a weird upload error that D7 emits when the browser sends 0 bytes because e.g. something like AppArmor blocked it from reading the file locally - see: https://www.mcdruid.co.uk/article/stopping-apparmor-blocking-firefox-rea...

I don't think I got round to doing that (yet) though; it's probably more of an edge-case than this one, but relatively similar.

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Less code is more good :)

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

This looks like a good backport, although the implementation's quite different, for the reasons that have been well explained.

Thanks!

πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

Thanks!

Production build https://api.contrib.social 0.62.1