- @avpaderno opened merge request.
- 🇩🇪Germany diqidoq Berlin | Hamburg | New York | London | Paris
Usually NR is rather used to review patches, but it also creates attention. The situation needs some more reviews and opinions like explained in #65. Repeating posted patches triggered by Needs Work just flood the issue but do not bring us closer to a final route to go. Usually this state would require Postponed (maintainer needs more info) ("PMNMI") but since I am not the maintainer of the image framework I would leave it up to the maintainers to choose this Status. There is sadly no Status in between these both.
Further Descriptions of the Priority → and Status → values can be found in the Drupal project issues → documentation.
- 🇮🇹Italy apaderno Brescia, 🇮🇹
Issues to fix PHP_CodeSniffer errors/warnings needs to show the arguments passed to
phpcs
and the report produced using those arguments. - Issue created by @Sunil Jolapara
The button should be disabled when making a post if any required field in the form is empty.
- 🇺🇸United States smustgrave
Fixes should be in MRs
Leaving issue summary tag as the User interface changes section is still empty.
- 🇮🇳India DishaKatariya
Hi, I have verified this issue, and it looks fine to me.
Testing Steps
1. Go to your Drupal site.
2. Go to any page with an off canvas dialog with a submit button. ( For example in the module Layout Builder.)
3. Observe the size of the buttonTesting Results
Patch applied cleanly and Olivero: submit button isn't looking wide in the off canvas dialog boxAttaching before and after screenshot.
Hence it can be move to RTBC
RTBC++Thanks!
- 🇺🇸United States AaronBauman Philadelphia
Sounds like a duplicate of 🐛 Exception: Warning: Undefined array key "preprocess functions" Drupal\Core\Theme\Registry->mergePreprocessFunctions() Needs work
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
- 🇺🇸United States cilefen
Put the steps to reproduce in the issue summary. The current merge request fails a DateTime test.
- 🇮🇳India arunkumark Coimbatore
I created an MR with the #6 patch. I ran the PHPUnit test locally. It is working fine. I have attached a screenshot for your
@smustgrave hope issue summary is fine
- @arunkumark opened merge request.
- First commit to issue fork.
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇭🇺Hungary balintk
- 📌 Improve UX of adding new sections Needs review landed 🚢
- 🐛 No upward auto-scroll when components are dragged Needs review landed 🚢
- 🇳🇿New Zealand quietone New Zealand
The links in the patch are part of deprecation testing and they do not need to be a valid URL.
This was fixed in a later issue 📌 Fix links that are 404 Fixed which was part of a meta, 🐛 [META] Many documentation / handbook URLs redirect to D7 content Needs work
- 🇳🇿New Zealand quietone New Zealand
Is this Support request supposed to be a Bug report? I ask because there is a patch here. Is this still relevant?
If this is still relevant, it needs an Issue Summary update and complete steps to reproduce the issue → (starting from "Install Drupal core").
Since we need more information to move forward with this issue, I am setting the status to Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
- @kmani opened merge request.
- 🇮🇳India kmani
Step 1: create a datatime field in any content type
step 2: Render the datatime field in twig file.
step 3: Check the time tag<time datetime="2024-09-05T04:09:23Z">05/09/2024 12:09</time>
Both datatime and displayed time is different
Solution: In DateTimeFormatterBase.php field formatter, we need update the below code to fix this issue.
Current one in Core:
protected function buildDateWithIsoAttribute(DrupalDateTime $date) { // Create the ISO date in Universal Time. $iso_date = $date->format("Y-m-d\TH:i:s") . 'Z'; $this->setTimeZone($date); $build = [ '#theme' => 'time', '#text' => $this->formatDate($date), '#attributes' => [ 'datetime' => $iso_date, ], '#cache' => [ 'contexts' => [ 'timezone', ], ], ]; return $build; }
Solution: We need to move above $this->setTimeZone($date); to iso_date field conversion.
protected function buildDateWithIsoAttribute(DrupalDateTime $date) { // Create the ISO date in Universal Time. $this->setTimeZone($date); $iso_date = $date->format("Y-m-d\TH:i:s") . 'Z'; $build = [ '#theme' => 'time', '#text' => $this->formatDate($date), '#attributes' => [ 'datetime' => $iso_date, ], '#cache' => [ 'contexts' => [ 'timezone', ], ], ]; return $build; }
- 🇵🇭Philippines a_ramos
I'm setting the status of this issue to "Needs review" as the patch is updated.
- 🇫🇮Finland lauriii Finland
Added 🐛 Keyboard commands to zoom in and out should only impact the preview canvas Active to the list.
- 🇬🇧United Kingdom catch
Did some installer testing. While
InstallerKernel::installationAttempted()
is theoretically correct, it doesn't actually work for errors generated by settings.phpThis is because unlike regular bootstrap,
install_begin_request() requires settings.php before InstallerKernel::bootEnvironment() is called - so any errors from settings.php itself use the default error handler.
Two options:
1. Refactor the installer to call InstallerKernel::bootEnvironment() prior to including settings.php, this might require initializing the kernel twice - both before and after, because we use the contents of settings.php to determine what $environment should be.
2. Add a custom error handler or tweak prior to settings.php being included, or set error display to FALSE altogether until it is.
The above is exactly the reason that issues like this should be handled in the public queue - potentially significant refactors of installer handling should not be carried out in secret by a small group of people especially to fix such a marginal issue.
I'm shocked, that this issue exists for 10 years. I discovered it recently while forking the contact module → and didn't find the time until now to report it as a security issue with a proper proof of concept. I guess, I can mark this as done now.
Sorry for the rant. To add something productive: In case this issue persists for some more years, I wrote the module disable_libraries → , which can be used to globally disable the
core/drupal.form
library.- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Oops … 🙈
I got this wrong, because I got #3469609 wrong — see #3469609-18: Prepare for multiple component types: ComponentTreeStructure should contain Component config entity IDs, not SDC IDs → .
Fixed now.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
🐛 Adding a component with slots does not show the slots in the layers view Closed: duplicate was marked as a duplicate of 🐛 Adding a component with slots does not register the slots as children Fixed by Jesse.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
✨ Allow deleting component instances by pressing "Delete" or "Backspace" Needs review landed 🚢
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Another issue that is closely related to this: 🐛 Redux support for ImageWidget: `[image] String value found, but an object is required` Needs work .
I think the solution that 📌 Media Library integration (includes introducing a new main content renderer/`_wrapper_format`) Needs work introduced with
data-media-file
is not scalable/sustainable. But I do think there also is an opportunity to solve this generically.See https://git.drupalcode.org/issue/experience_builder-3463842/-/tree/34638... for an outline (explained in 3 commits) of what I have in mind 🤓 (Although that surely won't work for real-time updates, only for AJAX updates, which is the case for complex widgets like image, media library, etc.)
- 🇧🇾Belarus chegor
Can be related to theme that used on exact site.
F.e. see: https://www.drupal.org/project/bootstrap/issues/3460442 🐛 Cannot run update.php Active - 🇺🇸United States smustgrave
Was previously tagged for tests which are still needed
Issue summary is also incomplete.
- Issue created by @Sunil Jolapara
- 🇮🇳India Sahana _N
Hi,
I am to reproduce the issue in 11.x I created an MR please review it.
Here, the “Back to content edit” URL redirects to the default language edit URL. I updated that
I attached the screenshot for reference.
I am happy to accept more suggestions for improvement.
Thank you!!
- @sahana-_n opened merge request.
- 🇫🇮Finland lauriii Finland
Adding 📌 Remove the default value for the XB field and move components to a separate module Active to the list.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
@lauriii was being trolled by Google Chrome — he literally couldn't type anything anymore 🙃 So, making this change on his behalf 😄
- 🇫🇮Finland lauriii Finland
Descoping 📌 Create Cypress E2E test to ensure component variants work Active because we have manually validated that variants work. This means that user story 6. is 🟢.
- 🇫🇮Finland lauriii Finland
Adding 🐛 Adding a component with slots does not show the slots in the layers view Closed: duplicate to the list.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
FYI, per 🐛 [PP-1] Can't toggle boolean prop back to true after changing to false Postponed , @kristen pol created these related issues, which I suspect ought to be child issues of this one:
- 🇫🇮Finland lauriii Finland
Adding 🐛 Redux support for ImageWidget: `[image] String value found, but an object is required` Needs work to the list as the new #1 blocker.
- 🇫🇮Finland lauriii Finland
Created an issue to polish the page hierarchy: 📌 Adjust page hierarchy styles to avoid elements moving Active .
- Issue created by @kmani
- 🇦🇺Australia kim.pepper 🏄♂️🇦🇺Sydney, Australia
Thanks for reporting this issue. We rely on issue reports like this one to resolve bugs and improve Drupal core.
As part of the Bug Smash Initiative, we are triaging issues that are marked "Postponed (maintainer needs more info)". This issue was marked "Postponed (maintainer needs more info)" more than 1 year ago.
Since we need more information to move forward with this issue, I am closing it.
Please feel free to reopen with more information.
- 🇭🇺Hungary balintk
📌 Component props form: map textarea, bool, and select elements to React components Fixed (previously titled make form elements match design) semi-landed (🛥️ ← smaller), 📌 Component props form: style plain text, link, textarea, bool, and select elements Active is taking its place.
I know a reroll isn't all that's needed, and that's why I left the status Needs Work.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
✨ Implement the concept of sections within the client Fixed landed 🚢
- 🇺🇸United States sea2709 Texas
@jessebaker: Thanks for your feedback :-) Will improve the UX based on your inputs!
I like how Gutenberg editor in Wordpress implements this feature https://wordpress.org/gutenberg/ and learning its implementation, still not figured out yet, but will dig into it more.
- 🇺🇸United States smustgrave
Previously tagged for tests and issue summary both appear still needed.
- 🇮🇳India samit.310@gmail.com
Hi, I have created the MR, I also added an extra condition in
viewsFormValidate
that fixed following error(Also added the screenshot).TypeError: array_filter(): Argument #1 ($array) must be of type array, null given in array_filter() (line 185 of /var/www/html/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php).
Test case is still pending, Post observing
core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaLibraryTest.php
it seems not compatible with Drupal 11 as it still usingcreate blog content
permission.One related issue is https://www.drupal.org/project/drupal/issues/3351597 🐛 [random test failure] Drupal\Tests\ckeditor5\FunctionalJavascript\MediaLibraryTest Active
Thanks
Samit K. - 🇮🇳India samit.310@gmail.com
samit.310@gmail.com → made their first commit to this issue’s fork.
- 🇩🇪Germany diqidoq Berlin | Hamburg | New York | London | Paris
Some points made before are still not addressed in the latest patches (like tests) so a re-roll only won't make it. Also we maybe need thoughts of project maintainers if this issue is outdated or still required, since it had its most momentum in the Drupal 8 migration live cycle to prevent wasted efforts and resource of contributors.
Additionally: #57 and below ✨ Call to a member function transformDimensions() on null in template_preprocess Needs work are these tags added in cooperation with the project maintainers or Drupal events and are these still required? Since there was no comment added to the tags they fade a little bit into curiosity.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
📌 Create a component for testing form backend + frontend integration Active landed weeks ago 👍
@bnjmnm, FYI: this has been added to 🌱 Milestone 0.1.0: Experience Builder Demo Active not to solve all the things, but to address at minimum ✨ Allow components to use textarea in favor of input Needs review — and perhaps also
type: boolean
(the latter is AFAICT a blocker in 📌 Component props form: map textarea, bool, and select elements to React components Fixed being reported by Utkarsh).(i.e. make at least those 2 field widgets work, just like 🐛 Prop select lists don't affect the component Fixed made
<select>
work.)It makes sense to me to prioritize this issue now, so that @bnjmnm can convert it to a meta issue where he outlines his plan for scaling this to all Drupal core field widgets.
- 🇬🇧United Kingdom jessebaker
@sea2709 the work you have done in https://git.drupalcode.org/project/experience_builder/-/merge_requests/241 seems to be a good approach to me. As you mentioned it covers more use cases (like loading in the component's CSS and JS) than what we have now.
However I think more work is required before it can be merged. These issues do seem quite challenging to solve, but ultimately are required for a good UX.
1) On hover there is a flash/flicker where the tooltip container (the div with the dropshadow) shows but the iFrame has not yet loaded. Can this be improved to look smoother.
2) The scaling seems to be wrong. Ideally a small button would display full size but a large Hero component would be scaled to a maximum width to fit inside the tooltip
3) the height of the tooltip is fixed and a lot taller than it needs to be.
(see above screenshot) - 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
📌 Media Library integration (includes introducing a new main content renderer/`_wrapper_format`) Needs work landed 🚢
Replacing #36's 📌 [PP-1] Media library should use styles from Claro Postponed with 📌 Media Library dialog styling Active .
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
📌 Improve the page hierarchy display Active landed 🚢
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
- @quietone opened merge request.
- 🇫🇮Finland lauriii Finland
Adding 📌 [PP-1] Media library should use styles from Claro Postponed to the list.
- 🇺🇸United States smustgrave
Still needs an issue summary update or risk closing out.
- 🇳🇿New Zealand quietone New Zealand
Tested on Drupal 11.x, umami install. I was able to reproduce the problem.
- 🇺🇸United States smustgrave
So I'm kinda not seeing the idea of adding the area? Could custom themes not do that themselves?
Anyone able to argue for this in 2.0.x ?
- 🇺🇸United States smustgrave
Since there has been no follow up. Going to close out. If still a bug please reopen
- achap 🇦🇺
The latest commit of https://git.drupalcode.org/issue/inline_entity_form-2822764/-/tree/2822764-support-adding-new-entities-when-translating applies cleanly to 3.0.0-rc20 as a patch. I made a small change to remove
@label
from the translatable markup that was never actually getting replaced.The failing
TranslationTest
needs to be fixed and it seems to me that this patch has unintentionally changed the behavior even when allow asymmetric translations hasn't been enabled as the test fails on line 108 which is well before we enable it.// Both inline nodes should now be in English. $first_inline_node = $this->drupalGetNodeByTitle('Kann ein Känguru höher als ein Haus springen?'); $second_inline_node = $this->drupalGetNodeByTitle('Can a kangaroo jump higher than a house?'); $this->assertSame('en', $first_inline_node->get('langcode')->value, 'The first inline entity has the correct langcode.'); $this->assertEquals('en', $second_inline_node->get('langcode')->value, 'The second inline entity has the correct langcode.');
- 🇺🇸United States smustgrave
Since there hasn’t been a follow up going to close out but if still valid later please reopen.
- First commit to issue fork.
- 🇺🇸United States cmlara
the installer prints errors on purpose, so... If we want to revisit that, it needs to be its own issue again, since that will need usability input and a lot of additional testing.
It is essentially the same root fault. I would warn that while the CVE might have only said "authorize.php" the "atttack" could could easily have used install.php instead. We should expect (just by the fact we have discussed it) install.php to be included as an amendment to the existing CVE or a NEW CVE in the future.
Making it easy for the users is not always the right decision. "I intended that" does not give a pass in an audit, being able to describe the 'reasonable security controls' that are in place to reduce the risk does. An anonymous user able to pull up with no restriction is 'no reasonable security controls present', while requiring a login and validating access could be 'reasonable security controls implemented'.
InstallerKernel::installationAttempted()
based on the docblock description and our limited operational capabilities at this point sounds like it could be a 'reasonable security control' though I have not done an in-depth audit to confirm. - 🇬🇧United Kingdom catch
@poker10 in Drupal 7, update.php defined
MAINTENANCE_MODE
and was a separate file. update.php in Drupal 10 is a route and doesn't defineMAINTENANCE_MODE
at all.In Drupal 11.x the only two places that define
MAINTENANCE_MODE
are install.php and authorize.php unless my grepping is failing.So limiting this to 'install' should be absolutely fine. As above we should be getting rid of
MAINTENANCE_MODE
altogether anyway, but one step at a time.@cmlara, the installer prints errors on purpose, so that people who try to install Drupal via the UI and hit environment or settings.php misconfiguration errors see an actual error messages instead of a WSOD or something else cryptic - that is the reason for the original code here. If we don't print errors in that situation, their only recourse is server logs, but there are various situations where the person installing may not have access to those (or know where they are even if they technically do).
Having said that, I think we can add a check for
InstallerKernel::installationAttempted()
which detects whether someone is trying to install or not. Pushing a commit for that. - 🇸🇰Slovakia poker10
This "override" code was added in #742246: Handle uncaught exceptions → in 2010, so no, this is not new and it is a part of Drupal for 14 years. The same code is also in D7 - with the difference, that there is explicitely check for updating, so this probably indicates we should be careful what the change in MR could affect:
https://git.drupalcode.org/project/drupal/-/blob/7.x/includes/errors.inc...
$updating = (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'update');
Drupal 8+ code was changed here to override also when installing: #2176105: Installer catches exceptions and manually re-prints them; does not use error/exception handler →
+ if (defined('MAINTENANCE_MODE')) { + return TRUE; + } - $updating = (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'update');
- 🇺🇸United States cmlara
@catch What about the install.php also leaking? Do we have any protection on a default install to prevent prying eyes from reaching through this vector?
I've added a list of versions I've tested against in the D8+ series. No surprise, our entire D8+ line appears to be subject to this.
- 🇬🇧United Kingdom catch
Put an MR up that adds an additional check for
MAINTENANCE_MODE === 'install'
in the error handler. Pipeline is green which means there's zero test coverage for error handling andMAINTENANCE_MODE === 'update'
but also means this isn't breaking anything else. - @catch opened merge request.
- 🇬🇧United Kingdom catch
Let's try just not automatically showing errors when MAINTENANCE_MODE === 'update' and explicitly check for 'INSTALL'.
- 🇺🇸United States greggles Denver, Colorado, USA
Thanks for that advice, catch. I do think pursuing the short-term path could be good as it may take some time and be difficult to resolve the blocker of composer operations with roave/security-advisories.
- 🇬🇧United Kingdom catch
authorize.php should be completely deprecated for removal when ✨ [PP-1] Add Alpha level Experimental Automatic Updates module Needs review is done.
In the meantime I think we should do a minimal fix to not show errors when MAINTENANCE_MODE = update is set, or to prevent setting it altogether in authorize.php (as long as that doesn't prevent it from working at all).
- 🇺🇸United States smustgrave
Did not review
Issue was tagged for issue summary update which appears to still be needed.
- 🇳🇱Netherlands Lendude Amsterdam
There was a fatal error when submitting an empty value. Fixed the error and added some test assertions to check this scenario.
- First commit to issue fork.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
We need a change record. There are contrib modules that will need to be updated due to removing this feature. See http://codcontrib.hank.vps-private.net/search?text=data-user-info-from-b... for examples.
Also I think we should consider adding a deprecation to template_preprocess_form() if the array key exists...
$form['#attributes']['data-user-info-from-browser']
. That way contrib and custom code can find out about this and update their code. - 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
🐛 Declare explicit runtime dependency on `justinrainbow/json-schema` RTBC was missing.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
The thing is, and I believe Berdir hinted at such a thing earlier in the comments, all systems need to know about your custom value you are trying to pass to the cache context.
With the access policy processor it's fine because the processor itself receives an account and we can use the account switcher there if someone uses a user-based cache context. It also passes the account object around so the individual access policies don't have to rely on the current user service (but could, if they wanted to). But we don't have this global representation of a value like "current user" for everything and neither do we have switchers for all the things.
Another thing you would end up with is that pages would become completely unchacheable or bogged down if we start using cache contexts that do not rely on a global context value. Keep in mind that cache contexts need to be fast. If we start bubbling up a bunch of cache contexts that need to load a few entities to get their return value, we'd be getting really slow pages.
Having said that, I am in favor of trying to rethink the cache contexts we have to allow for better reusability. Currently, what we're seeing in this issue is that we have a system which is given an account and then caches by a cache context which does not know what that account is and assumes the current user. The APP can work around this because it controls the caching, but the AccessResult does not so it can't.
So what we would need here is a calculated cache context called user.permissions:%uid, which would default to current_user if nothing was given and the permission hash of the account if provided. Then the cache context could have an optimization where it compares the passed in account ID to the current user and runs with the latter if the IDs match.
The issue is that if you have user.permissions and user.permissions:%uid on the same page, they will get folded into user.permissions and that crucial piece of information (the UID) gets lost.
Which brings us to the next possible solution: current_user.permissions and user.permissions, but that would require all of core and contrib to rename their user.foo cache contexts to current_user.foo. And you'd still risk folding issues as I'm not sure CacheContextsManager::optimizeTokens() currently knows how to deal with
user:13,user.permissions:14
oruser:13,user.permissions:13
. At a glance it seems it would not work.So hopefully it is starting to become clear what a pain in the ass it can be to tell a cache context which specific value to use for all ancestors of a given cache context.
Which brings us back to reusability (again): If we could somehow say: "I need to know the cacheability of a given context, provided I swap out its global value for something else.", we'd be in better shape. For user.foo we can using the account switcher, sure, but we need a universal way for all cache contexts like route, url, etc...
If we allowed cache contexts to declare what they depend on, we could write a service which allows you to call said cache context's methods while in a mocked environment so to speak (different route, user, url, ...), without actually changing the environment like the workaround in APP.
Solve that, and you solve this whole issue and more. But keep in mind the most preferred option is the one that doesn't require all cache contexts to be rewritten, even though I see no other way around it.
- 🇧🇪Belgium klaasvw
Setting the z-index too high will make the balloon overlap any dialog that is opened from the balloon (e.g. when using the linkit_media_library module).
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
🐛 `enum` data shapes: error when choosing "- None -" in `` Needs work is necessary too, to avoid the
- None -
option appearing for thetype: string, enum: […]
prop shapes. - 🇫🇮Finland lauriii Finland
Adding 🌱 [META] Redux sync on ALL prop types, not just ones with a single [value] property Active because based on #3471171-12: Allow components to use textarea in favor of input → , it's needed for the textareas to work.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
#30: I think you meant to remove it? 😅
✨ Allow components to use textarea in favor of input Needs review is also in, removing.
- 🇮🇳India Prashant.c Dharamshala
@jan kellermann
Not sure about the files you mentioned in #80 🐛 Do not prepopulate the user register form with the email address and username of the last person who registered using the same web browser Needs work because the other methods from the library could have been used on these forms.
- 🇺🇸United States sea2709 Texas
https://git.drupalcode.org/issue/experience_builder-3469856/-/pipelines/...
I just updated the Cypress E2E scripts for previewing components, it's working now, yeah!!! It took a while since I can't set up Cypress on my local!
I see the job UI eslint fails, I think it failed after I merged branch 0.x into my branch, so maybe there is some in progress UI eslint work on 0.x branch. - 🇩🇪Germany jan kellermann
@nod_:
- Maybe the `core/drupal.form` is not neccessary in modules/comment/comment.libraries.yml also?
- In modules/user/src/RegisterForm.php on line #36 `drupal.form` is also added. - 🇩🇪Germany jan kellermann
Thank you prashant c., the MR works for me (tested with PHP8.3 and D11-dev). Did not change state because of open comments.
- 🇸🇰Slovakia poker10
Adding a related issue which propose to remove
MAINTENANCE_MODE == 'update'
(which is used in authorize.php as well). - 🇺🇸United States cmlara
That research can (and should) be done by anyone. There's no special access that makes it easier or better to be done by a member of the Security Team.
I was expecting the DST to have easy access to this as part of a Root Cause Analysis which should have been completed prior to the issue being cleared for public disclosure which would have been in private tracker.
To answer the question I asked: Yes this flaw can be confirmed as leaking database details when connection errors occur. I would assume any other 'early bootstrap' issues could leak as well occur too.
Invalid database:
PDOException: SQLSTATE[HY000] [1044] Access denied for user 'db'@'%' to database 'db2' in Drupal\Component\DependencyInjection\PhpArrayContainer->createService() (line 77 of /var/www/html/web/core/lib/Drupal/Component/DependencyInjection/PhpArrayContainer.php).
Invalid username or password:
Drupal\Core\Database\DatabaseAccessDeniedException: SQLSTATE[HY000] [1045] Access denied for user 'db'@'172.16.5.68' (using password: YES)
Re titling issue. This needs an IS update, though care should be taken as this is the linked document for a published CVE.
- 🇺🇸United States greggles Denver, Colorado, USA
> A report by the DST of what could leak would help us better title this issue.
That research can (and should) be done by anyone. There's no special access that makes it easier or better to be done by a member of the Security Team.
- 🇮🇳India Prashant.c Dharamshala
PR is failing it is showing the test
core/modules/block/tests/src/Functional/BlockCacheTest.php
failing but locally when I executed all the tests passed. Not sure what is causing these test to fail in the pipelines. - 🇮🇳India Prashant.c Dharamshala
Tested the MR with i think originally the changes from taken from patch submitted in #55 🐛 Do not prepopulate the user register form with the email address and username of the last person who registered using the same web browser Needs work .
It is fixing the issue of this autocomplete and the username and email address are no longer auto-filled during user registration and browser no longer has values stored in localstorage for
Drupal.visitor.mail
andDrupal.visitor.name
.Steps to reproduce
- Login as admin and go to /admin/config/people/accounts
- Select "Visitors" in "Who can register accounts?" and save
- Now visit the site as anonymous user and register as a user from "/user/register"
- Logout with the currently logged in user and try to register another user, you will see that "Email address" and "Username" fields are autofilled with the values you filled to register your previous user
- Also in the browser's local storage there are 2 properties
Drupal.visitor.mail
andDrupal.visitor.name
where these values are also stored.
- 🇮🇳India Prashant.c Dharamshala
Rebase the branch
2409107-11x
with latest code from11.x
. Need to see if any tests are failing. - 🇮🇳India Prashant.c Dharamshala
prashant.c → made their first commit to this issue’s fork.
- First commit to issue fork.
- 🇭🇺Hungary mxr576 Hungary
Alternative approach
How about introducing "self-contained" cache contexts? Because our actual problem is depending on a global context (current user, current route, etc.).
I built this years ago in a module that may becomes public soon (no strict commitment).
/** * Defines a cache context "per acting user's node grants" caching. * * Cache context ID: 'node_grants_by_acting_user:||%uid' (to vary by all * operations' grants). * Calculated cache context ID: 'node_grants_by_acting_user:%operation||%uid', * e.g., 'user.node_grants:view||%uid' (to vary by the view operation's grants). * * The built-in NodeAccessGrantsCacheContext always calculates grants for the * current user that might not be the same as the acting user. This cache * context SHOULD BE only used in that case otherwise it is redundant. */
The only drawback of this approach that I see is implementation details (e.g., route id) and internal ids (e.g., UID) get exposed by cache context ids, but we could also find a solution for those.
- 🇭🇺Hungary mxr576 Hungary
// Sadly, there is currently no way to reuse the cache context logic outside // of the caching layer. If we every get a system that allows us to process // cache contexts with a provided environmental value (such as the current // user), then we should update the logic below to use that instead.
Before we would go for a "Wild hunt" with defining an almighty API that would allow to fold ANY cache context with any contextual value, do we really need that to solve this issue? Would not we only need a generic, or worst case a copy-pasted solution from
\Drupal\Core\Session\AccessPolicyProcessor::processAccessPolicies()
, for translatinguser.*
cache contexts t actual cacheability data? (I am asking this to balance between increasing scope vs. delivering solution.) - 🇭🇺Hungary mxr576 Hungary
2024 update and bump :)
So Re: #56: Both VariationCache and Flexible Permissions is in Drupa core \o/ (The latter is accalled Access Policy API)
If I am not mistaken the "cache context fold" solution atm locked inside
\Drupal\Core\Session\AccessPolicyProcessor::processAccessPolicies()
but that can be easily abstracted if necessary for solving this task. (Which #61 refers to)🐛 VariationCache needs to be more defensive about cache context manipulation to avoid broken redirects Active may also come handy when we start refactoring code for solving this one.
- 🇩🇪Germany jan kellermann
Added new tag GDPR because storing cookies or data in localstorage without consent can violate data protection law, in Germany for example the TTDSG.
I support the proposal to remove it completely from the core.
- 🇳🇿New Zealand quietone New Zealand
I tested the commands on the 110.0 release page and found that that all work to update a site. The most challenging was using a site pinned at 10.0.1. For that I had to follow the link to the doc page to unpin first but after that was done the update proceeded without error.
Therefor, closing this now. Re-open if I missed anything.
- 🇺🇸United States sea2709 Texas
@Wim Leers: When you have a chance, can you take a look at this MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/241 ?
This is how it works on my local https://drive.google.com/file/d/1hr2WXAg8V-xX2oC91i7oDHO78dYZHUO0/view?u...This MR is using iframe and attach a src attribute to iframe for component preview. At first, I was about to use srcdoc for the iframe by building a preview HTML for each component, but then I saw this issue https://www.drupal.org/project/experience_builder/issues/3471070 🐛 [Performance regression] Loading the components takes >5s Active , I changed the implementation to build a component preview route.
I noticed that the MR's pipeline https://git.drupalcode.org/issue/experience_builder-3469856/-/pipelines/... is failed at the job cypress E2E, I searched through source code test cases we implemented, and found test cases that are checking CSS classes for ShadowDOM, since I'm no longer using ShadowDOM then the test cases fail. I'm not familiar with doing Cypress E2E test cases, but can give it a try if you think using iframe is a good approach.Thanks :-)
- @samit-khulve opened merge request.
- 🇮🇳India samit.310@gmail.com
samit.310@gmail.com → made their first commit to this issue’s fork.
- @sea2709 opened merge request.
- 🇺🇸United States cmlara
It’s displaying the install path when there is a coding error causing an exception in settings.php.
DST: What other error types does this expose that should otherwise be hidden? Connection errors with hostnames or usernames (and in the case of really bad code passwords)? I'm thinking about Database or Queue modules off hand though I'm sure there are others here.
While the file path is a great example my first though based on the code linked in #6 is the issue description might currently be under-scoped. A report by the DST of what could leak would help us better title this issue.
- 🇺🇸United States cilefen
Could the issue summary be cleaned up not to say “traversal” because this isn’t a traversal? Since it’s a critical now it should be accurate.
It’s displaying the install path when there is a coding error causing an exception in settings.php.
- 🇳🇿New Zealand quietone New Zealand
Restoring template to assist with tracking the work here.
- 🇺🇸United States cmlara
While the overall impact (as noted in the CVE) is LOW In accordance with the Issue Priority Field documentation I'm the upgrading priority to Critical as the issue meets the condition "Expose security vulnerabilities" as documented by the issuance of a CVE by an approved CVE Numbering Agency. We addtioanly have risk the CVE could be reviewed and publish releases added to the CVE.
The priority can reduced if the CVE is retracted.
Opened 💬 Publication of CVE-2024-45440 by MITRE Active for the directly related discussions regarding the publication of the CVE by MITRE and its impact on the D.O. ecosystem.
- 🇺🇸United States greggles Denver, Colorado, USA
Let's please keep the comments here focused on solving this problem. Commenting about other topics distracts and makes it harder to solve the problem.
Thanks, @poker10 for your analysis so far.
I read the analysis in #6 and it seems that MAINTENANCE_MODE is being "overloaded" and used to indicate 2 purposes. Unless there is a specific reason why errors really should be shown when a site is in MAINTENANCE_MODE, it seems ideal to remove this behavior OR create 2 variables, one for MAINTENANCE_MODE and another for something like MAINTENANCE_MODE_SHOW_ERRORS to have each variable behave as intended and avoid using one variable for 2 purposes.
- 🇺🇸United States cmlara
If you are concerned about specific policy, feel free to open an issue to discuss it. Thanks!
Others and myself have.
In some cases I would raise a concern on an issue or in SLACK (where we’re discussing a problem and a possible solution fixes said problem as well as additional problems at the same time) and been shut down so aggressively by DST members that my concern was an actual dedicated policy change issue would likely have been filed as a D.O. Code of Conduct violation.
If you, in your role as a security team member, are saying the team would like dedicated issues for these I can attempt to review my records and open outstanding concerns.
or other exceptions here
I’m not sure we have an official name for this in the industry however we recognize this as CNA’s that refuse to publish “valid” security concerns.
This is why I mention the CNA-LR portion of the program in previous posts and in direct questions to the DST. The CNA-LR is one method vulnerability reporters have to bypass a CNA that fails to publish a CVE for a “valid” threat. Either MITRE failed to defer this to the registered CNA, or they published this as a CNA of Last Resort(other CNA refused to publish or no other CNA held scope, both could apply based on how this CVE was written to target the development branch which the DST CNA explicitly lists as outside its scope).
Subnote: The CNA program has recently changed to v4 rules, I’m more familiar with v3 rules, and some of these processes may have changed in v4.
The DST has refused to adopt new standards in the past due to “labor” concerns, and yet advocates against requests to reduce their labor by giving maintainers more autonomy similar to GitHub.
- 🇩🇪Germany rkoller Nürnberg, Germany
I've taken a lookt at the aural interface in voiceover on macos sonoma (see voiceover.mp4), a few observations:
- i've just entered a single character to get more results across different groups (tested on a test instance without any content), but somehow after entering the single character "n" the announcement providing the number of results and the instructions how to navigate the list right after is being cut off somehow.
- The group titles like
content - article
,contact form
,file
and so on are unavailable in the aural interface. - The meta data of the username as well as the date and time an entity was created is unavailable in the aural interface
- The file name and the name of media item are identical but the name is only announced the first time either way (going downwards same as going upwards), the second time remains unannounced.
and unrelated to screenreaders, woult it make sense to leave a pointer either on
admin/config/content/link-suggesters
or on the link suggestor edit page either in a help text or a description pointing out that for activating the link suggestor the "Entity Links" checkbox has to be ticked on a text format? At the moment that fact is implied. - 🇩🇪Germany C-Logemann Frankfurt/M, Germany
I created additional routes for /user/{uuid} and /user/{uuid}/edit in the u3id module and published already:
https://git.drupalcode.org/project/u3id1. This is working fine.
2. The pathprocessing from actual patch makes it impossible for such clean route solution. So I strongly recommend to not commit for core. I currently even not recommend to not move to contrib or with add a warning for risks of breaking other code.There are lot's of concerns against a pure uuid strategy as I read here: [1726734]
For getting forward I suggest to focus on a a minimal improvement:
1. First of all we should provide an entity_uuid ParamConverter. For u3id I copied of the core jsonapi: 🐛 Use UUID as entity ID Needs review
2. Following the initial feature request we can just provide a possibility to access entities based on additional uuid route configurations.
3. Per default we can just do redirections and introduce settings.php options to change behavior to an entity view.Everything else should be postponed for core until there is a larger support for more uuid in core routes. I'm currently fine with my contrib project u3id and already solved the problem of rendering the serial uid in the canonical html link with replacing the entity view controller and use now the uuid path.