The files in core/misc/ui/themes/base/images/ all have file permissions of 755. They should, of course, be changed to 644. Patch to follow.

There was overlap with #1948148: Update file permissions

Follow-up:
#1891270: [discussion] How can we prevent "invisible" regressions?

CommentFileSizeAuthor
#8 drupal-file-perms-all-1841896-8.patch637 bytesTravisCarden
#8 drupal-file-perms-no-scripts-1841896-8.patch338 bytesTravisCarden
#1 drupal-ui-images-perms-1841896-1.patch1.86 KBTravisCarden
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
Assigned: TravisCarden » Unassigned
Status: Active » Needs review
FileSize
drupal-ui-images-perms-1841896-1.patch1.86 KB

I wonder: should there be an automated test for file permissions?

swentel’s picture

swentel CreditAttribution: swentel commented
Status: Needs review » Reviewed & tested by the community

Good to go

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

A follow-up to discuss how we could try and prevent these types of "invisible" regressions in the future sounds great.

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented

Thanks, @webchick. I created #1891270: [discussion] How can we prevent "invisible" regressions? for the follow-up.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Title: Fix file permissions in core/misc/ui/themes/base/images/ » Fix file permissions in core/misc/ui/themes/base/images/ (rollback)
Status: Fixed » Reviewed & tested by the community

I think this should be rolled back - see previous discussion of these files in #1830330: Make default.settings.php not executable.

If you think they shouldn't be executable, talk to the people at https://github.com/jquery/jquery-ui/tree/master/themes/base/images and convince them there :) But for whatever reason, the files are executable upstream, and for a minor thing like this we shouldn't make changes to an external library. (Especially because, if we only fix it in Drupal, the "invisible regression" will almost certainly come back the next time we update the version of jQuery UI in core.)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Title: Fix file permissions in core/misc/ui/themes/base/images/ (rollback) » Fix file permissions in core/misc/ui/themes/base/images/
Status: Reviewed & tested by the community » Postponed

Oops. Fair enough. Good catch!

Reverted 6baf46137.

Marking... postponed? I guess? On the upstream issue.

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented

Thanks, @David_Rothstein. Here's an upstream pull request: https://github.com/jquery/jquery-ui/pull/887.

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
Title: Fix file permissions in core/misc/ui/themes/base/images/ » Fix various file permissions
FileSize
drupal-file-perms-no-scripts-1841896-8.patch338 bytes
drupal-file-perms-all-1841896-8.patch637 bytes

We've got the same issue on some files from Symfony. Here's an upstream pull request for them: https://github.com/symfony/symfony/pull/6933.

In the meantime, we do have a few of our own files with the executable bit on:

  • core/modules/locale/locale.fetch.inc
  • core/modules/views/tests/views_test_config/test_views/views.view.test_dropbutton.yml
  • core/scripts/drupal.sh
  • core/scripts/password-hash.sh
  • core/scripts/run-tests.sh

I figured the shell scripts were supposed to be executable (?), but not all of them in core/scripts are, so I wasn't sure. Are they not supposed to be since they're meant to be invoked by the PHP interpreter as opposed to a shell? For expedience's sake, here are patches both ways.

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
Status: Postponed » Needs review

Oops.

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented

Update: The upstream issues in jQuery UI and Symfony have both been fixed.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

I have reviewed and applied drupal-file-perms-no-scripts-1841896-8.patch, it's working fine for me.
I skipped drupal-file-perms-all-1841896-8.patch, as I assume the shell scripts are executable by intention.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Reviewed & tested by the community

Yup, the second patch in #8 is RTBC.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

#8: drupal-file-perms-no-scripts-1841896-8.patch queued for re-testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Quick fix
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Ressin fressin! Thanks.

Committed and pushed to 8.x.

YesCT’s picture

YesCT CreditAttribution: YesCT commented

... overlap with #1948148: Update file permissions (adding to issue summary)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

added overlap/duplicate and followup discussion issue