.htaccess rules broken since yarn.lock got added

Thanks for spotting this and adding the test. I agree that the regex is malformed and the $ and | have accidentally been added the wrong way round.

I think we need a few more tweaks to be consistent with the ^(\.(?!well-known).*|Entries.*|Repository|Root|Tag|Template|composer\.(json|lock)|web\.config)$

In fact I think really we should be putting both of these in that part of the rule for consistency... so it should be:
^(\.(?!well-known).*|Entries.*|Repository|Root|Tag|Template|composer\.(json|lock)|web\.config|yarn\.lock|package\.json)$

There are two more things that would be fixed by this... the dot would be escaped and therefore packageAjson would not be matched. But also the path would have to be exactly yarn.lock or package.json.

Here's the fix as per #14

I thought it might be easier to review the total change to the file from before #3308369: Block access to yarn.lock and package.json, so this is what I did:

1. Committed the approaches from #11 and #15 to local branches from 10.1.x named "11" and 15".

2. Compared changes in the relevant files in those branches to 2233484^, which is the last commit before #3308369: Block access to yarn.lock and package.json, i.e.:

   git diff --color-words="."  2233484^ -- core/assets/scaffold/files/htaccess 
   git diff --color-words="."  2233484^ -- .htaccess
   

   .

Here's images of that to compare the two versions in color more easily:

#11

Scaffold file

.htaccess

#15

Scaffold file

.htaccess

Oh and since I can, here's the "colorized interdiff" for #15.

I consider this RTBC. If someone else can review and +1, I am also willing to commit this once the tests finish (despite the code freeze) to fix a bad regression for 10.0.0.

#15 looks good to me. RTBC+1.

Did a manual local test using Apache for testing htaccess.

The following are tested:

D10.0.x and D9.5.x

9.5.0-rc2 and 10.0.0-rc3

Before the patch:

both core/package.json and core/yarn.lock files are accessible with 200.

After the patch (via curl https://www.drupal.org/files/issues/2022-12-14/3327115-15.patch | git apply):

both core/package.json and core/yarn.lock files are 403 Forbidden

Thanks @pandaski for the manual test!

The test failure in https://www.drupal.org/pift-ci-job/2545386 is a known random that happens about once a week -- in fact it also just today happened in HEAD in https://www.drupal.org/pift-ci-job/2544788. So that is not blocking.

Committed to 10.1.x, and cherry-picked to 10.0.x and 9.5.x. Thanks!

I don't think this hotfix needs to be in the release notes; we can just use the release note @longwave added to the original issue.

@Eric_A I may also forget about it 50% of the time. Including when committing this issue. However, looking at its current code, I think it's OK:

          <match url="\.(engine|inc|install|module|profile|po|sh|.*sql|theme|tw\
ig|tpl(\.php)?|xtmpl|yml|svn-base)$|^(code-style\.pl|Entries.*|Repository|Root|\
Tag|Template|all-wcprops|entries|format|composer\.(json|lock)|\.htaccess|yarn.l\
ock|package.json)$" />

If there's anything not-right about it, let's open a followup? Thanks!

FWIW, I did check web.config before writing #19, and thought it was okay since it already included yarn.lock and package.json. However, I did miss that the dots weren't escaped there. I don't believe that's a security problem: it just means that web.config is denying access to file names (e.g., yarn-lock) that it doesn't have to deny access to. But a follow-up to make that consistent with what's in .htaccess is worth doing.

This should be send to nginx to update their page about drupal:https://www.nginx.com/resources/wiki/start/topics/recipes/drupal/ (which is by the way an outdated doc).

Here's the fix for nginx:

   # Protect files and directories from prying eyes.
    location ~* \.(engine|inc|install|make|module|profile|po|sh|.*sql|theme|twig|tpl(\.php)?|xtmpl|yml)(~|\.sw[op]|\.bak|\.orig|\.save)?$|composer\.(json|lock)|web\.config|yarn\.lock|package\.json$|^(\.(?!well-known).*|Entries.*|Repository|Root|Tag|Template)$|^#.*#$|\.php(~|\.sw[op]|\.bak|\.orig|\.save)$ {
        deny all;
        return 404;
    }

If I understand it correctly apache match only the last part of the url/filepath (most right directory or filename) but nginx match the whole path.

So in order to deny all composer.json, composer.lock, web.config, yarn.lock or package.json, I move this part to specific part of the regex:

-    location ~* \.(engine|inc|install|make|module|profile|po|sh|.*sql|theme|twig|tpl(\.php)?|xtmpl|yml)(~|\.sw[op]|\.bak|\.orig|\.save)?$|^(\.(?!well-known).*|Entries.*|Repository|Root|Tag|Template|composer\.(json|lock)|web\.config)$|^#.*#$|\.php(~|\.sw[op]|\.bak|\.orig|\.save)$ {
+    location ~* \.(engine|inc|install|make|module|profile|po|sh|.*sql|theme|twig|tpl(\.php)?|xtmpl|yml)(~|\.sw[op]|\.bak|\.orig|\.save)?$|composer\.(json|lock)|web\.config|yarn\.lock|package\.json$|^(\.(?!well-known).*|Entries.*|Repository|Root|Tag|Template)$|^#.*#$|\.php(~|\.sw[op]|\.bak|\.orig|\.save)$ {

Could someone test and validate this modification?

As I don't understand what Entries.*|Repository|Root|Tag|Template is referring too, it would be nice to document this part.