USA
Account created on 25 June 2007, over 17 years ago
#

Merge Requests

More

Recent comments

🇺🇸United States mradcliffe USA

Not sure why I hadn't closed this yet.

🇺🇸United States mradcliffe USA

This comes from auto_entitylabel_entity_presave(), which checks if the label is equal to the placeholder value, but if the label is NULL, then it won't do anything.

I think the previous behavior could be maintained if the check included NULL and the placeholder value.

I don't think I was correct about this, but there are tests now.

🇺🇸United States mradcliffe USA

Was also running into a similar issue as @michèle in #106 🐛 Set Label runs two times on node creation Needs work as title was calculated based on a saved value (via serial module) so "Before save" is not an option. I had a hook_node_insert implementation that checked isNew(), which no longer passed because of this issue. Luckily I no longer needed to do that extra check.

🇺🇸United States mradcliffe USA

I have custom code that creates a node and prior to the change I was able to not set a title value using the create method on the entity storage handler. It looks like if I provide the value of "%AutoEntityLabel%", then programmatically created node is successful without the integrity constraint violation exception.

This comes from auto_entitylabel_entity_presave(), which checks if the label is equal to the placeholder value, but if the label is NULL, then it won't do anything.

I think the previous behavior could be maintained if the check included NULL and the placeholder value.

🇺🇸United States mradcliffe USA

Looks good locally and with test run.

Quick merge.

🇺🇸United States mradcliffe USA

Updated from Annotation to Attribute.

🇺🇸United States mradcliffe USA

This should now fail because of the PHP warning.

🇺🇸United States mradcliffe USA

I'm satisfied with my testing locally.

Merged.

🇺🇸United States mradcliffe USA

I read CWE-863 and CAPEC-87 and those seem like the closest for the Information Disclosure vulnerability in SA-CONTRIB-2024-034 (freelinking row 42).

I looked through some of the children of the parent CWE-285 and CAPEC-115, but did not see any children that were more specific for Information Disclosure.

🇺🇸United States mradcliffe USA

Should be good. Need to do some manual testing and let this gel for a bit.

🇺🇸United States mradcliffe USA

Also the test run from 3 months ago passes on PHPUnit 9:

PHPUnit 9.6.20 by Sebastian Bergmann and contributors.
..................................                                34 / 34 (100%)
Time: 00:02.545, Memory: 6.00 MB
🇺🇸United States mradcliffe USA

But this will not be (backwards)compatible with PHPunit 9 I presume?

It should be easy to figure out by adding test previous major into gitlab CI yaml. I needed to add that anyway to most of my modules because of the flip to testing Drupal 11 as the current major.

🇺🇸United States mradcliffe USA

Revoked Drupal 10 compatibility from 2.0.x branch because it is not possible to be compatible with Drupal 10 and Drupal 11 due to Symfony incompatibilities.

🇺🇸United States mradcliffe USA

It is not possible to make this module compatible with both Drupal 10 and Drupal 11 thanks to Symfony version incompatibilities.

So I'm going to revoke 10 compatibility from 2.0.x.

🇺🇸United States mradcliffe USA

Added some better alternative text to the typed data diagram.

🇺🇸United States mradcliffe USA

og is only a dev dependency so setting it to ^1.x-dev for now.

🇺🇸United States mradcliffe USA

I don't think this is RTBC, but I wanted to show red-green test runs.

🇺🇸United States mradcliffe USA

I wasn't sure if fixing phpcs wold be in scope, but it's great to see fully green. Thank you, @liam morland.

🇺🇸United States mradcliffe USA

Merging for both 2.0.x and 1.0.x.

🇺🇸United States mradcliffe USA

mradcliffe changed the visibility of the branch 3455705-refactor-tests-for to active.

🇺🇸United States mradcliffe USA

mradcliffe changed the visibility of the branch 3455705-refactor-tests-for to hidden.

🇺🇸United States mradcliffe USA

Unpostponing, tests are running on D11 by default now on this branch.

🇺🇸United States mradcliffe USA

Updates from Annotation to Attribute.

🇺🇸United States mradcliffe USA

That should do it. Tests pass for Drupal 10 (previous major) and won't pass for Drupal 11 until module is compatible with Drupal 11.

🇺🇸United States mradcliffe USA

I added a test as well, but this will probably fail due to 📌 Automated Drupal 11 compatibility fixes for nodeinfo Needs review .

🇺🇸United States mradcliffe USA

I cleaned up the issue summary a little bit and added the branches to the issue summary.

🇺🇸United States mradcliffe USA

10.3.x, 10.4.x and 10.5.x composer.lock files match previous release of composer so I cherry-picked the 10.3.x commit into branches for 10.4.x and 10.5.x.

🇺🇸United States mradcliffe USA

I'm going to re-open this as Normal since there is a workaround, but we should still update the dependency in core.

I'm removing the performance and security tags since that is no longer relevant. And maybe it's probably a task now.

🇺🇸United States mradcliffe USA

I ran the pipeline again on the merge request, and it passed.

🇺🇸United States mradcliffe USA

Updating twig/twig caused some Functional failures so setting this to Needs work.

🇺🇸United States mradcliffe USA

twig/twig:3.14.2 and twig/twig:3.11.3 are released now by Fabien.

This needs to be in 11.x-dev since it needs to go into 11.0.x, 11.1.x, 10.3.x and 10.4.x.

🇺🇸United States mradcliffe USA

Adding some tags, updated issue summary with sections from template.

I don't feel like only checking \is_object($v) would be acceptable upstream since the point of the method ensureToStringAllowed() method checks arrays as well, but as soon as we recurse via arrays too, we run into the recursion limit.

A potential Drupal-only solution would be to create a custom DrupalSandboxExtension that copies the code in SandboxExtension, and maybe checks $policy::$allowed_methods somehow to ensure that nobody has overridden core's TwigSecurityPolicy to remove __toString as an allowed method. If it is still allowed, then we can bypass the call to that method entirely and return $obj, and if somehow someone is doing something funky in custom land, then it would try its best with the original code.

We cannot extend SandboxExtension because it is final so this would require maintaining the code separately.

And then maybe we can work with twig/twig:4.x to allow for a way to override ensureToStringAllowed on the policy level rather on the extension level so we can get rid of it.

🇺🇸United States mradcliffe USA

I think I spoke too soon about it being infinite in our case, but it does hit the recursion limit. Sorry.

Checking the is_object($v) helps to reduce that.

🇺🇸United States mradcliffe USA

I don't know if this should be addressed upstream or not, @gillesbailleux. Reporting an issue upstream might help to get someone more knowledgeable about the right approach though.

🇺🇸United States mradcliffe USA

Following Issue Priority > Critical > Bug , I think this qualifies as a Critical bug report as the site becomes unusable when applying the release for content editors. Either render system or theme system.

🇺🇸United States mradcliffe USA

# 3.14.1 (2024-11-06)

* [BC BREAK] Fix a security issue in the sandbox mode allowing an attacker to call attributes on Array-like objects
They are now checked via the property policy
* Fix a security issue in the sandbox mode allowing an attacker to be able to call `toString()`
under some circumstances on an object even if the `__toString()` method is not allowed by the security policy

We may be doing something that's triggering this based on the BC BREAK note here.

🇺🇸United States mradcliffe USA

I also ran into this updating my local ddev development environment from 10.3 to 11.0, but prior to updating to 11.0 I did run composer update -W per upgrade instructions.

I added the empty stracktrace from #8 that I also got into the issue summary.

This seems like a bug report.

🇺🇸United States mradcliffe USA

Confusingly, this can also happen if you are writing a new kernel test even without any request object being constructed when making a mistake with dependencies or services.

🇺🇸United States mradcliffe USA

Made a little more progress. Done for now. Maybe some changes with membership since I did the PR originally?

🇺🇸United States mradcliffe USA

This looks good to me.

Potentially setting core_version_requirement to ^10.3 || ^11 might help for the next stable release as well in case anyone one who hasn't gotten to 10.3 tries to update and runs tests. That could also be done in the other issue as well.

🇺🇸United States mradcliffe USA

Thank you for going through this effort, but no further development is happening on this module due to the processing.js library being deprecated. The module must be completely written for a different library before any further development occurs.

It would probably be better to look for an existing issue within a contributed module to work on first.

🇺🇸United States mradcliffe USA

I updated IEF to 3.0@rc so the test on next major runs, but there is a failure in D11 that's not present in D10. I'm not sure if it's related to IEF or not.

🇺🇸United States mradcliffe USA

Seems good to me. Merged.

🇺🇸United States mradcliffe USA

Could a site builder end up creating a situation where they set the sub title to use h2 and then the hierarchy no longer matches between teaser and page views?

- Page
- h1: Node Title via page-title
- h2: Sub title
- Teaser
- h1: Page title
- h2: Node title
- h2: Sub title

🇺🇸United States mradcliffe USA

I added some items to the work needed based on Slack.

🇺🇸United States mradcliffe USA

Sorry for the very late response.

I looked at the site_alert module and I think the only thing that may be causing the double render is that the module has a route it loads via JavaScript, /ajax/site_alert.

If the agreement is configured with Visibility to "Show on every page except the listed pages", then adding that path to the Visibility Pages configuration will prevent the Agreement from rendering when that path is requested.

🇺🇸United States mradcliffe USA

We can start by creating an issue fork and merge request for 11.x, and the issue is in Needs work based on comments above. Once in Needs review, it might help to add this to the Needs Review Queue Initiative or discuss in an appropriate Slack channel.

I removed the beta table adjusting the issue summary to compensate because that was something we did specifically for Drupal 8 and no longer applies.

🇺🇸United States mradcliffe USA

Removing Needs issue summary update tag and setting status to RTBC because I think recent comments were addressed.

🇺🇸United States mradcliffe USA

Applied #13 patch and commit now that I should be unblocked on another issue. Leaving active because Drupal 11 compatibility depends on module dependencies that are not yet ready.

🇺🇸United States mradcliffe USA

Yes, it's firefox ESR. If it's related to that, those will probably go away when I'm able to get on the next ESR release within the next several days.

🇺🇸United States mradcliffe USA

Updating component to Trial Experience.

🇺🇸United States mradcliffe USA

Probably should have fixed this a long time ago.

🇺🇸United States mradcliffe USA

Added a merge request. See what fails and address those items.

🇺🇸United States mradcliffe USA

I removed the UX review task in the issue summary and added a note about it in the User interface changes section.

I added the Needs issue summary update tag for further updates to the User interface changes section to update the screenshot.

🇺🇸United States mradcliffe USA

One of the most helpful things that @volkswagenchick created as part of the First-time Contributor Workshop slide deck were hidden slides at the top with instructions on how to modify the deck based time and audience as well as reminders about how to copy and share the slides with accessibility in mind.

These would be helpful as well for a shared deck.

🇺🇸United States mradcliffe USA

Added mention for issue credit for PR review and work on GitHub for users mpp, semiaddict, voleger.

🇺🇸United States mradcliffe USA

Okay, all migrated over. Probably several code quality issues to fix and tests to fix so bumping to Needs work.

🇺🇸United States mradcliffe USA

I forgot to double-check user names.

🇺🇸United States mradcliffe USA

Alison is providing feedback at GovCon 2024.

🇺🇸United States mradcliffe USA

Oh, you're trying to download the module from the fork? This is a composer issue rather than a git issue. The branch and issue fork exist in git.

But it would be easier to test the issue fork in DrupalPod there is a browser extension for DrupalPod that makes this seamless on an issue page.

When I'm working locally with my own modules, I have the module checked out directly and can add git remotes and checkout branches from issue forks.

🇺🇸United States mradcliffe USA

I was able to clone the fork and branch successfully on a new DrupalPod instance.

gitpod /workspace/drupalpod/repos/theme_switcher (3434980-gitlabci) $ git status
On branch 3434980-gitlabci
Your branch is up to date with 'theme_switcher-3434980/3434980-gitlabci'.

nothing to commit, working tree clean
gitpod /workspace/drupalpod/repos/theme_switcher (3434980-gitlabci) $ git remote -vvv
origin  https://git.drupalcode.org/project/theme_switcher.git (fetch)
origin  https://git.drupalcode.org/project/theme_switcher.git (push)
theme_switcher-3434980  https://git.drupalcode.org/issue/theme_switcher-3434980.git (fetch)
theme_switcher-3434980  https://git.drupalcode.org/issue/theme_switcher-3434980.git (push)

I'll merge origin/2.x and push to the fork. Hopefully that helps.

🇺🇸United States mradcliffe USA

Re-postponing on gitlab ci.

🇺🇸United States mradcliffe USA

Drupal 11 is released. Closing as fixed.

🇺🇸United States mradcliffe USA

mradcliffe made their first commit to this issue’s fork.

🇺🇸United States mradcliffe USA

Thank you for the patch. I reviewed it, and applied the .module fixes.

🇺🇸United States mradcliffe USA

I created an issue fork branch with an initial commit, but I think it is Needs work because Lucas, Elli and Brian had similar questions about participating without being physically at events so I did not want to remove their names yet.

🇺🇸United States mradcliffe USA

I also was not able to reproduce. This looks to have been resolved with 📌 Making a theme compatible with core's theme generator is too difficult Needs work in Drupal 10.3.x and higher.

More info on the change record: Starterkit themes now use starterkit.yml file . The src/StarterKit.php file is ignored.

Production build 0.71.5 2024