We have also run into this. I think that the honeypot code needs to be more configurable. There should either be config to disable it, or config to control it's value separately from the default IP banning.
Closing, ... I ran into the same issue. We've decided to not pursue SRI for external JS provided by reliable 3rd parties such as yourself. While there is a security concern, it's impossible for us to keep up with 3rd party libraries which might change. This really is the responsibility of those 3rd party libraries, such as crazyegg, to provide SRI's.
I took a different approach than the existing work, and wrote a drush command that encrypts existing data. See ✨ Add drush command to encrypt existing data Needs work
douggreen → created an issue.
To answer my own question from above, see 📌 Create a Bulk Encryption/Decryption Service Class to facilitate encryption of pre-existing data Needs review and 📌 Create a UI for managing encryption of pre-existing data and changing encryption profiles. Active
The patch solves the error. I'm wondering if there's another solution to encrypt existing content that wasn't previously encrypted.
I've seen this warning in my logs but I'm having a hard time finding a test case, and I haven't seen it recently. I think that this patch is just hiding an underlying problem, which is that either that #max_delta is wrong, or that the element that #max_delta points to wasn't added. Can anyone reproduce this still? Is it possible that this was a problem in core that has since been fixed?
Patch attached
douggreen → created an issue.
LGTM
I still get a PHP warning in AttributeArray (line 79) because the original input array is statically cached in ViewExecutable::getExposedInput() on line 726, which happens before the data is validated. We need to validate the data before it is saved.
Since it's not version controlled, we can't rely on the SRI, and should remove it. The file has changed at least once since we implemented this.
I rebase and force pushed the branch. It needs review by someone who is familiar with what this does.
douggreen → made their first commit to this issue’s fork.
Do this do anything that ✨ Add option to show only start or end date in the DateTime Range custom formatter Needs review doesn't do? If it does, I think it needs to be re-worked now that the other issue has committed.
The above commit should fix this.
douggreen → created an issue.
douggreen → created an issue.
$ curl https://cdn.localizejs.com/localize.js | openssl dgst -sha384 -binary | openssl base64
7w/vCB9txvKaPgvRpSBQWnsULnvvH1VyQJXvjZujqTrsAvQI6B0H42gKqQUCG3y6
douggreen → created an issue.
I like the idea of removing the config option extlink_target_append_new_window_label which defines the label text and is new to this patch, always using the recommended text, and linking to the external resource (from above). This isn't removing an option in production. This is removing an option that someone in this thread thought helpful, and replacing it with a standardized (maybe industry standard) a11y text.
The current patch checks for "new window" in the existing label (not "opens in a new window") so I think that the existing patch is already backwards compatible with production systems.
If we agree, then what's left is to remove the config option extlink_target_append_new_window_label and make sure the tests work. If I understand correctly, we're pretty much at the place that you started with this ticket, it just took me a while to get there (sorry). What do you think?
This appears to have merged, so marking as fixed.
This was made an option because of comments starting in #11 above without any discussion until #51. Also, #51 suggested linking to WCAG somewhere, but that was never done. And IMO that is a better thing to have done than making the accessible feature optional. What should we link to?
While I dislike removing options that other people think are helpful, I agree with @jenlampton → that the new option extlink_target_append_new_window should not be an option but just always be true.
I'm a little less clear on whether the other new option extlink_target_append_new_window_label should be configurable or some set text. The fact that extlink_target_label is configurable makes me think that extlink_target_append_new_window_label should also be an option.
Also a little unclear to me is if the new JS I wrote should even exist. If both labels are configurable, and if the defaults are as we currently have them, this JS will never be used. And since the site builder has full control over this, is this new JS a waste of time to run in the browser and maintain in code?
There is some overlap (and conflicts) with ✨ A11y: add screen reader text for external links Needs review so I copied this commit from here to there. This can still be merged, if it goes first. Or if that MR goes first, this can be closed.
There is some overlap (and conflicts) with 🐛 Aria-label for external links span throws errors on Accessibility Arc Toolkit Needs work so I copied that commit from https://git.drupalcode.org/project/extlink/-/merge_requests/18 to https://git.drupalcode.org/project/extlink/-/merge_requests/16 and resolved the conflicts.
I created MR https://git.drupalcode.org/project/extlink/-/merge_requests/18 so I'm removing the patch attachments from this, to avoid confusion.
douggreen → made their first commit to this issue’s fork.
I created a MR, so I'm hiding all of the patches attached to this ticket, to avoid confusion.
The attached patch :
- makes the "(new window)" label configurable
- compares the title and the "(new window)" label in a case-insensitive manner without the parenthesis. See Drupal.extlink.compareLabels().
- combines the two titles in a more readable manner, see Drupal.extlink.combineLabels().
It does not update the tests.
douggreen → created an issue.
In response to #3 - You could keep logBlockedSubmission in the native condition, that's a design choice. I like using my own hooks as it shows other developers how to use them.
In response to #4 - we aren't altering anything, I think invokeAll is the correct method. And yes, we should inject the module_handler.
Committed, thanks!
douggreen → created an issue.
douggreen → created an issue.
I've seen a similar error in my logs, but I'm on Drupal 10.2.x (not 7.x as this issue is against), so this is not the right fix. And I don't think the problem is passing an empty path to the delete, because my log shows the 1.xml in the path not being deleted.
This is easy to test. Create a URL that sends the lat and not lng or vice-versa. I normally agree that we shouldn't hide PHP warnings, except when they can be triggered by a user sending bad data. It's a simple fix, and I disagree that this is how it is intended to work. We should fail, but not fail with PHP warnings.
merged, thanks!
Thanks, I've updated the project page and marked the project as Unsupported.
sure, thanks for the help!
Odd, I don't see the "create Merge Request" button. I created a branch and pushed to it but the "create merge request" button is missing and the link that git gives me gives a 404.
https://git.drupalcode.org/issue/drupal-3412420/-/compare/11.x...3412420...
MR created.
In reply to the comment by @smustgrave that this was possibly done on purpose because of a layout builder scenario, that isn't mentioned at all in the original ticket. And this very problem here was mentioned there.
This didn't fix create access. See 🐛 BlockContentAccessControlHandler requires access block library permission for create Needs review
Patch attached
douggreen → created an issue.
Patch attached
douggreen → created an issue.
Thanks! Committed :)
Patch attached
douggreen → created an issue.
I rebased and force pushed.
douggreen → created an issue.
douggreen → made their first commit to this issue’s fork.
We also are seeing this, a total of 8 sites, that we upgraded over a period of 2 weeks (didn't want to upgrade all at once).
On 10/11 we upgraded the first 5 sites
On 10/18 we upgrade the remaining 3 sites
On 10/31 I recorded 1867 errors in the prior week
On 11/7 I recorded 845 for the prior week
Has anyone considered using fallback defaults? For example, if 'theme' is not set as a query argument, use the default theme (taking into account if it's an admin path). If 'delta' is not set, use 0. etc...
Is this different than 🐛 Global password is requested multiple times for every protected page Needs work ?
Is this different than ✨ Access to multiple pages with a one password Needs review ?
This must have been a D9 problem, because now that we're on D10, this patch has broken things. I'm closing this issue.
Can you cut a new release so that we can stop using the dev branch please :)
lol, no worries, I made a similar mistake earlier today with another module! It happens.
I'm attaching a new patch that reverts the previous commit and still makes my change from above.
You committed something different than my patch, now we have duplicate dependencies in the definition.
I'm closing this issue, as it was more of a support request that has been answered.
I fixed the typo in the table name, thanks!
Committed, thanks!
Uploading the same patch, but without the do-not-test name, since this is now a 7.x ticket.
Updated patch applies cleanly to 7.98
Attached is an equivalent fix for D7.
I do wonder if the preExecute() should be done on the original query before clone is called, so that the hook_query_tag_alter()'s get called once instead of twice, when the same query is used for both a count and an execute... But the D9 version does it here in the counting, so I've kept it the same here.
See attached
douggreen → created an issue.
yes we are using this in production (for several years now)
Backport for Drupal 7
Oops, that last comment was meant to update the issue summary.
Problem/Motivation
When multiple files in a directory use the protected pages password, the user must re-enter the password for every page they want to access. It would be a better user experience if they only had to enter the password once.
Proposed resolution
Add a new option "validate per section" which would remember if the user has entered a valid password already and not require it for all pages in the section.
User interface changes
A new admin option is added. See https://www.drupal.org/files/issues/2023-04-19/validate_per_section_chec... →
Updated patch is relative to the module and not docroot, so that it applies cleanly.
Stripped ^M off of patch so that it applies.
I didn't use all these changes, but I did use some of these fixes, so credit was given, thanks!
Duplicate of 📌 Adjust core_version_requirement in info.yml Fixed
committed, thanks
Committed, thanks
Oops, I missed a jQuery.once(), see attached.
Oops, this fixes a syntax error.
You can ignore my comment about the private function. I was confused. I thought from the description that someone wanted to use this function with other modules, and make it more flexible at the same time. If the purpose of this change is to just make _allowed_formats_field_types more flexible with configuration, then this is great!
I've been running this on a production site since making the patch. A link to IpUtils makes sense. Can you add that? And for tests, I'd prefer that someone else writes them ... if you require them and don't want to write them yourself, I'll ask someone else on my team to do it, but I'd also prefer that you do that too ... since this patch does what I need.