Problem/Motivation

We've introduced .htaccess files to prevent access to images and content. We only need to introduce one file and we could test using the testing profile for speedier tests.

Proposed resolution

Do it

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
5.76 KB

here's a patch. We also get additional protection since we won't serve core/profiles/demo_umami/modules/demo_umami_content/default_content/articles.csv either. Plus the test will be way way quicker. A single test and using the testing profile since install Umami is not required for testing this.

Eli-T’s picture

Status: Needs review » Reviewed & tested by the community

Have applied patch in #2

Patch reviewed - looks good.

Tested umami still installs and the recipes and articles are still created.

Have tested the following paths successfully return a 503 response:

/core/profiles/demo_umami/modules/demo_umami_content/default_content/images/chocolate-brownie-umami.jpg
/core/profiles/demo_umami/modules/demo_umami_content/default_content/articles.csv
/core/profiles/demo_umami/modules/demo_umami_content/default_content/articlesNONSESNE.csv
/core/profiles/demo_umami/modules/demo_umami_content/default_content/recipes.csv
/core/profiles/demo_umami/modules/demo_umami_content/default_content/article_body/lets-hear-it-for-carrots.html

Therefore putting this to RTBC.

Gábor Hojtsy’s picture

  • Gábor Hojtsy committed 1539316 on 8.6.x
    Issue #2942769 by alexpott, Eli-T: Consolidate umami .htaccess files and...

  • Gábor Hojtsy committed 779f756 on 8.5.x
    Issue #2942769 by alexpott, Eli-T: Consolidate umami .htaccess files and...
Gábor Hojtsy’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Makes total sense, thanks! Committed to 8.6 and backported to 8.5.

Anonymous’s picture

  • alexpott committed 554e251 on 8.6.x
    Revert "Issue #2942769 by alexpott, Eli-T: Consolidate umami .htaccess...

  • alexpott committed 79b2e3b on 8.5.x
    Revert "Issue #2942769 by alexpott, Eli-T: Consolidate umami .htaccess...
alexpott’s picture

Status: Fixed » Needs work

@vaplas thanks for reporting - Ho hum that's weird how we got a pass. But this is a module discovery issue.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.1 KB
5.87 KB

Here's the fix. Also going to create an issue about the 8.6.x module listing behaviour. Being able to discover modules from not installed profiles is a serious bug.

Gábor Hojtsy’s picture

Duh this is the nth time the new module discovery is biting us. (At least we are working out the bugs now I guess).

alexpott’s picture

At least we are working out the bugs now I guess

Yep totally!

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

The problem with fail on 8.5.x is resolved. @alexpott opened #2945306: The new ModuleList in 8.6.x can discover modules in tests the previous code could not. to address module discovery problem. So, back to RTBC.

Gábor Hojtsy’s picture

Version: 8.5.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
$ git apply --index 2942769-12.patch
error: patch failed: core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php:163
error: core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php: patch does not apply

Apparently some stuff I committed in the meantime, sorry :/

smaz’s picture

Status: Needs work » Needs review
FileSize
5.45 KB

Interdiff was empty, patch is the same, just fixes:

Hunk #1 succeeded at 163 with fuzz 1.

smaz’s picture

Status: Needs review » Needs work

Hmm, my patch seems to miss moving /core/profiles/demo_umami/modules/demo_umami_content/default_content/article_body/.htaccess.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.01 KB
5.25 KB

Here's another approach. What we really want to ensure is that these files are not accessible regardless of the whether umami is installed on not. So let's just hardcode the path.

Interdiff back to 12.

The last submitted patch, 17: 2942769-17.patch, failed testing. View results

Anonymous’s picture

Another approach looks nice, but there are a couple of proposals:

  1. +++ b/core/profiles/demo_umami/modules/demo_umami_content/tests/src/Functional/DefaultContentFilesAccessTest.php
    @@ -0,0 +1,36 @@
    +   * Note the demo_umami profile is not used because we want to ensure that if
    +   * you install another profile these files are not available.
    

    Move to assert

  2. +++ b/core/profiles/demo_umami/modules/demo_umami_content/tests/src/Functional/DefaultContentFilesAccessTest.php
    @@ -0,0 +1,36 @@
    +    // Hard code the path sonce the demo_umami profile is not installed.
    

    sonse/sinse

  3. +++ b/core/profiles/demo_umami/modules/demo_umami_content/tests/src/Functional/DefaultContentFilesAccessTest.php
    @@ -0,0 +1,36 @@
    +    $default_content_path = 'core/profiles/demo_umami/modules/demo_umami_content/default_content/';
    ...
    +      $default_content_path . 'images/chocolate-brownie-umami.jpg',
    

    merge in one variable

  4. +++ b/core/profiles/demo_umami/modules/demo_umami_content/tests/src/Functional/DefaultContentFilesAccessTest.php
    @@ -0,0 +1,36 @@
    +      $this->assertFileExists(str_replace('/', DIRECTORY_SEPARATOR, $this->root . '/' . $file));
    

    Do not convert to OS-independent path. In Windows this assert will work too. A problem only arises when a straight string comparison of two paths as is. Example.

Done.

alexpott’s picture

Thanks @vaplas that looks really nice! +1

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

Looks super solid, great work @vaplas!

  • Gábor Hojtsy committed 07e76fd on 8.6.x
    Issue #2942769 by alexpott, vaplas, smaz, Gábor Hojtsy, borisson_, Eli-T...

  • Gábor Hojtsy committed d6674ad on 8.5.x
    Issue #2942769 by alexpott, vaplas, smaz, Gábor Hojtsy, borisson_, Eli-T...
Gábor Hojtsy’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed
+++ b/core/profiles/demo_umami/modules/demo_umami_content/tests/src/Functional/DefaultContentFilesAccessTest.php
@@ -0,0 +1,38 @@
+    // The demo_umami profile should not used because we want to ensure that if
+    // you install another profile these files are not available.

"should not *be* used" (be missing AFAIS). Fixed that on commit.

Thanks all!

neclimdul’s picture

FileSize
1008 bytes

commit on fix added whitespace to the end of the line and broke phpcs testing ;)

I hate the one character followup issue but I'll create it if you want. Here's the patch if you just want to fix it.

alexpott’s picture

Committed and pushed 097d71c2fd to 8.6.x and 9a4e8a9a98 to 8.5.x. Thanks!

Fixed the CS issue.

  • alexpott committed 097d71c on 8.6.x
    Issue #2942769 by neclimdul: Consolidate umami .htaccess files and...

  • alexpott committed 9a4e8a9 on 8.5.x
    Issue #2942769 by neclimdul: Consolidate umami .htaccess files and...
Gábor Hojtsy’s picture

Duh I have no idea how did that happen. The committer script did not let me commit due to the CS fail so I fixed the CS fail locally and then the committer scripts let me commit (without a CS fail). Oh well.

alexpott’s picture

@Gábor Hojtsy that happens because you didn't git add your updates. I need to work on https://github.com/alexpott/d8githooks/pull/1

Status: Fixed » Closed (fixed)

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