Secure SCSS file extension in .htaccess

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Adding the patch against Drupal 10.1.x-dev. No interdiff since the previous patch does not apply.

Sass has two syntaxes! The SCSS syntax (.scss) is used most commonly. It's a superset of CSS, which means all valid CSS is also valid SCSS. The indented syntax (.sass) is more unusual: it uses indentation rather than curly braces to nest statements, and newlines instead of semicolons to separate them. All our examples are available in both syntaxes.

See https://sass-lang.com/documentation/syntax

.sass is a valid sass file extension. Meanwhile, .less is a similar one (e.g. Less is used in Bootstrap 3)

So I would suggest securing .sass and .less files as well.

Meanwhile, HtaccessTest, core/assets/scaffold/files/web.config, web.config, should be updated

A CR is required I think.

The last submitted patch, 23: 3190045-23.patch, failed testing. View results →

Add 3 files: access_test.scss, access_test.sass and access_test.less for testing.

Should pcss.css files be added to that list?

Right

.pcss.css file exists in Drupal see https://api.drupal.org/api/drupal/core%21themes%21olivero%21css%21base%2...
.pcss is in use, see #3151341: Change the PostCSS file extension to pcss →

Tested this on ddev, Drupal 10.1 with an apache-fpm server.
Visited https://drupal-10-x.ddev.site:8443/core/themes/claro/css/components/acti... and got an access denied.

Reviewed Patch on #34, access has been successfully invoked, can be merged now.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Update core/scripts/css/postcss-build.js to ignore core/modules/system/tests/fixtures/HtaccessTest/access_test.pcss.css

Seems to have a CI failure.

Added a dummy comment to the .pcss.css file and generated its associated .css file for testing.

Made a wrong file extension

FYI, change made to core/scripts/dev/commit-code-check.sh in change[#3343495] failed #34

+++ b/core/assets/scaffold/files/web.config
--- /dev/null
+++ b/core/modules/system/tests/fixtures/HtaccessTest/access_test.css

+++ b/core/modules/system/tests/fixtures/HtaccessTest/access_test.css
@@ -0,0 +1,6 @@
+/*
+ * DO NOT EDIT THIS FILE.
+ * See the following change record for more information,
+ * https://www.drupal.org/node/3084859
+ * @preserve
+ *//* foo */

--- /dev/null
+++ b/core/modules/system/tests/fixtures/HtaccessTest/access_test.pcss.css

+++ b/core/modules/system/tests/fixtures/HtaccessTest/access_test.pcss.css
@@ -0,0 +1 @@
+/* foo */

Changes since #34

Looks good now!

Reviewed the Patch on #42, works for me, so issue noticed.thanks

The last submitted patch, 42: 3190045-41.patch, failed testing. View results →

Random failure.

The last submitted patch, 42: 3190045-41.patch, failed testing. View results →

The failure occurred in the testContextualLinks function, where there was an assertion failure due to a NULL value being evaluated as empty.

Random failure

The last submitted patch, 42: 3190045-41.patch, failed testing. View results →

Given the previous comments and the issue reported, I think it is again random failure.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

We are working on this issue at DrupalCon Portland 2024

fengtan → changed the visibility of the branch 11.x to hidden.

Rebased and tests are green.

Rebase seems good.

This seems like a great security improvement, but given the changes to the .htaccess, and well-tested, but to be on the safe side I'd like a framework manager signoff on the change. Will ask about.

....What about web.config?

Looks good to me

larowlan → closed merge request !8006

larowlan → reopened merge request !8006

I'm not sure if the /* foo */ added in #41 was intentional, but it certainly should at least say something other than foo. I'm going to try removing it and running the pipeline. Leaving RTBC since this is a tiny thing.

OK, so it can't be empty:
[{"type":"issue","check_name":"no-empty-source","description":"Unexpected empty source (no-empty-source)","content":{"body":"Error found in no-empty-source. See https://stylelint.io/user-guide/rules/no-empty-source for more details."},"categories":["Style"],"location":{"path":"modules/system/tests/fixtures/HtaccessTest/access_test.pcss.css","lines":{"begin":1,"end":1},"positions":{"begin":{"line":1,"column":1},"end":{"line":1,"column":2}}},"severity":"major","fingerprint":"035978200c47e4bd55c604f40144ce3d"}]

Going to make it say a meaningful thing instead.

Pushed up a revert of my bad commit and a change to the uninformative/debug-code-looking "foo" comment. Leaving RTBC since this is such a small change.

Saving creditable contributions.

Pipelines passed with that fix.

New changes look fine to me

Committed to 11.x only as a change to site-owner-managed files.

Tagging for the release notes. I edited the change record to copy the release note, because "As title" is not actually good change record content. :D

Thanks everyone!

Also added the snippet to the drafts of the alpha and stable release notes for 11.3.0.

xjm → closed merge request !8006

I'm late to the party here but I'm wondering if this will impact source maps for local development if there are some configurations of postcss/sass etc that rely on browsers fetching the original source? Looks like that may be the sourcesContent option in postcss that defaults to true, so would only be affected if this had been set to false. Sass might have a similar option - https://sass-lang.com/documentation/cli/ruby-sass/#sourcemap

If we can verify that this is indeed an issue then this might warrant a CR.

Excellent catch, @mstrelan. Reopening for review/testing. If appropriate, we should add a bit to the existing CR and update the release notes to link it. (They didn't right now because it was 1:1.)

@mstrelan had another good thought in Slack which was:

Also if we're blocking these for security reasons, but then recommended they make sure the source is embedded in the map, then we should also recommend that the map file is not deployed to production. Maybe we should be blocking the map file too ...

So we could have a followup to add (I think) .css.map to the extension list as well?

xjm → credited nod_ → .

OK, I made a mistake here in that I tagged this for framework manager review because I was thinking about serving assets and the implications thereof, when in retrospect the actual correct choice would have been to tag it for frontend framework manager review because this is about access (or not) to frontend/em> assets, and (as per #79) affects the frontend developer's experience in a non-trivial way.

@nod_ also made the case that the security justification for this is very weak compared to PHP or JS sources. (For what it's worth, I mostly shrugged at the issue summary point about inline comments; I just considered it more of a "less is more" situation in terms of security hardening.) But if the security benefit is at best a marginal edgecase, while also making frontend development and debugging significantly harder, that's not a good tradeoff. It also doesn't look like anyone else is doing this.

@nod_ suggested that actually the correct course of here is to revert and wontfix. Since this is primarily a frontend framework issue, we'll do that. (The issue will remain credited, since wontfixed issues also grant credit now.)

Marking wontfix as per #82, unpublishing the CR, and removing from the release notes. Thanks all!