Problem/Motivation

As found in this post, composer.json files are indexed by search engines. This is not (necessarily) a security problem, but:

  • it seems like a good idea to disallow users from finding a website with such search queries
  • we can't think of a good reason why someone would want to access composer.json or composer.lock directly from the web
  • malicious visitors could use the information in the files to start looking for vulnerabilities

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Issue priority Not critical because it does not open a security problem, but it seems like a good idea.
Prioritized changes The main goal of this issue is security.
Disruption Not disruptive.

Proposed resolution

Initially, we proposed either:

  1. Disallowing search engines from indexing composer.json and composer.lock by adding them to robots.txt
  2. Blocking access to composer.json and composer.lock using .htaccess and web.config

Looking at robots.txt, we did not see any lines which corresponded with blocking rules in .htaccess or web.config. Based on this precedent, we decided to only update .htaccess and web.config.

Remaining tasks

  1. Decide on the path moving forwards.
    We decided to update .htaccess and web.config
  2. Write a patch for D8.
  3. Update patch to add tests.
  4. Review and RTBC.
  5. Commit to D8, set Status to Patch (to be ported), change Version to 7.x-dev.
  6. Post patch to D7 (see #10).
    Note that @mparker17 could not find any automated tests in D7 that checked .htaccess rules.
  7. Review and RTBC.
  8. Commit to D7.
  9. Decide if it's worth porting to D6?

User interface changes

None.

API changes

None

CommentFileSizeAuthor
#28 disallow_composer_json-2392153-28.patch1.52 KBhussainweb
#22 disallow_composer_json-2392153-22.patch2.51 KBchris.smith
#16 disallow_composer_json-2392153-16.patch3.2 KBmparker17
#16 interdiff.txt970 bytesmparker17
#14 interdiff.txt573 bytesmparker17
#14 disallow_composer_json-2392153-14.patch2.52 KBmparker17
#12 disallow_composer_json-d7-2392153-12-do-not-test.patch1.81 KBmparker17
#10 disallow_composer_json-d7-2392153-10-do-not-test.patch965 bytesmparker17
#7 disallow_composer_json-2392153-7.patch1.52 KBhussainweb
#4 disallow_composer_json-2392153-4.patch1.01 KBhussainweb
#2 disallow_composer_json-2392153-2.patch307 byteshussainweb
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented
Issue summary: View changes
hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented
Status: Active » Needs review
Issue tags: +Quickfix
FileSize
disallow_composer_json-2392153-2.patch307 bytes

Implementing patch as per option 1. This should be a quickfix and I don't see any disruption.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

I don't really see a reason why we should not even include it in our .htaccess, as this adds much more security, than the robots.txt here does.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented
FileSize
disallow_composer_json-2392153-4.patch1.01 KB

Sounds like a plan. I got confused initially as I thought I remembered an issue where we removed composer.json from htaccess. I just recollected it was actually info files.

I didn't put an interdiff as it is a really small change. The robots.txt is same as previous patch. The only new thing is change in .htaccess.

tstoeckler’s picture

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

Yup, this makes the files unaccessbile via the web on my install. Having the additional layer in robots.txt makes sense IMO -> RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work

Adding to both .htaccess and robots.txt is overkill. Plus we should add to web.config.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented
Status: Needs work » Needs review
FileSize
disallow_composer_json-2392153-7.patch1.52 KB

Agreed. If we are doing this for .htaccess, changing robots.txt is probably an overkill. However, should we consider servers where .htaccess is disabled? What about nginx? If yes, then robots.txt would be a backup to stop search engines from indexing it.

Anyway, I am attaching a patch with only .htaccess and web.config changed. And I changed the regex in .htaccess slightly compared to #4. Interdiff won't really help here as it is all on one line. The change is that instead of starting a different pattern, I am putting it with this pattern as follows:

^(\..*|Entries.*|Repository|Root|Tag|Template|composer\.(json|lock))$

If we decide to go with plain robots.txt approach (as the concern is really just search engines), the patch in #2 should be good. Otherwise, the patch in this comment blocks it from both .htaccess and web.config.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented
Parent issue: » #2002304: [META] Improve Drupal's use of Composer
saltednut’s picture

saltednut
she/her
Primary language English
Location Chicago
CreditAttribution: saltednut commented
Title: Disallow composer.json and composer.lock via robots.txt » Disallow composer.json and composer.lock from being indexed

Updated title to reflect #7 - it looks like the question remains: can we solve this via .htaccess and web.config? Yes, for the most part. At the same time, it seems thorough and simple enough to additionally disallow indexing via robots.txt. As a backup for non-htaccess situations, its not overkill so much as just being thorough. I believe the thing to do is combine #7 and #2 but I won't move this to Needs Work until we have that consensus.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-4
CreditAttribution: mparker17 as a volunteer commented
Issue tags: +Needs backport to D7
FileSize
disallow_composer_json-d7-2392153-10-do-not-test.patch965 bytes

Backporting this to Drupal 7 would be ideal, as I'd like to be able to use different versions of certain tools (like Drush) on different projects.

Attaching a D7 patch; but marking as "do-not-test" because it won't apply to the 8.0.x branch (testbot runs tests based on the Version in the issue metadata).

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-4
CreditAttribution: mparker17 as a volunteer commented

I can't think of a good reason why someone would want to access composer.json or composer.lock directly from the web: if anything, malicious visitors could use the information in the files to start looking for vulnerabilities. I vote we block it with .htaccess and web.config.

Looking at robots.txt, I do not see any lines which correspond with blocking rules in .htaccess or web.config. Based on this precedent, I do not think that it's necessary to disallow search engine access to composer.(json|lock) in robots.txt as well.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-4
CreditAttribution: mparker17 as a volunteer commented
FileSize
disallow_composer_json-d7-2392153-12-do-not-test.patch1.81 KB

Missed the change to web.config in #10.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs review » Needs work
Issue tags: +Needs tests

We can improve Drupal\system\Tests\System\HtaccessTest to test this.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-4
CreditAttribution: mparker17 as a volunteer commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
disallow_composer_json-2392153-14.patch2.52 KB
interdiff.txt573 bytes

Added composer.json and composer.lock to the list of protected files to test in \Drupal\system\Tests\System\HtaccessTest::testFileAccess().

Status: Needs review » Needs work

The last submitted patch, 14: disallow_composer_json-2392153-14.patch, failed testing.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-4
CreditAttribution: mparker17 as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff.txt970 bytes
disallow_composer_json-2392153-16.patch3.2 KB

This should work.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-4
CreditAttribution: mparker17 as a volunteer commented
Issue summary: View changes

Updated issue summary to keep it up-to-date with the latest decisions in the comment section.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-4
CreditAttribution: mparker17 as a volunteer commented
Issue summary: View changes

Whoops, used code tags instead of del tags on Remaining Tasks that were Done.

cilefen’s picture

cilefen CreditAttribution: cilefen commented

+1 for this change

meba’s picture

meba CreditAttribution: meba commented
Priority: Normal » Major
Issue tags: +Needs security review

I actually think this is Information Disclosure. According to #1975220: Allow a Composer user to manage Drupal, modules, and PHP dependencies with a custom root composer.json you should be able to modify this file therefore could potentially include raw data or links to custom repositories. I'll bump this to major and tag with needs security review.

Feel free to move down if you disagree.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

patch doesn't apply

chris.smith’s picture

chris.smith CreditAttribution: chris.smith at Portage CyberTech commented
Status: Needs work » Needs review
FileSize
disallow_composer_json-2392153-22.patch2.51 KB

Rerolled and submitted for testing, based on the assumption that we want users to receive a 403 when attempting to access composer.json and composer.lock.

    // Ensure composer.json and composer.lock cannot be accessed.
    $file_paths["$path/composer.json"] = 403;
    $file_paths["$path/composer.lock"] = 403;
naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer and at Acquia for Acquia commented
Issue tags: -Needs reroll

patch rerolled in #22

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Reviewed & tested by the community

We have test coverage, so this looks perfectly for me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: -Needs backport to D7, -Needs security review +8.0.2 release notes

Committed 8a0a5c1 and pushed to 8.0.x and 8.1.x. Thanks!

I think we should mention this change in the 8.0.2 release notes as it is a change to the .htaccess file.

  • alexpott committed 8a0a5c1 on 8.0.x 
    Issue #2392153 by mparker17, hussainweb, chris.smith, alexpott, dawehner...

  • alexpott committed 580b4cf on 
    Issue #2392153 by mparker17, hussainweb, chris.smith, alexpott, dawehner...
hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
Status: Patch (to be ported) » Needs review
FileSize
disallow_composer_json-2392153-28.patch1.52 KB

Making the changes. I can't find an existing test for this. Is there one in Drupal 7?

naveenvalecha’s picture

naveenvalecha
He/Him
Primary language Hindi
CreditAttribution: naveenvalecha as a volunteer and at Acquia for Acquia commented
Status: Needs review » Reviewed & tested by the community

Is there one in Drupal 7?
We don't have test coverage, this looks good to be in.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +7.42 release notes

Yeah, I don't think we need automated tests for something like this.

Committed to 7.x - thanks!

  • David_Rothstein committed aa755ed on 7.x 
    Issue #2392153 by mparker17, hussainweb, chris.smith, alexpott, dawehner...
David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Issue tags: -8.0.2 release notes +8.0.3 release notes

I think we should mention this change in the 8.0.2 release notes as it is a change to the .htaccess file.

This was tagged for the 8.0.2 release notes, but it looks like it wasn't in 8.0.2 at all - rather it's scheduled for 8.0.3. So I'm changing the tag accordingly.

I would suggest in general that the Drupal 8 release notes start including something like the standard line used in the Drupal 7 notes ("No changes have been made to the [list of files] in this release, so upgrading custom versions of those files is not necessary") and then of course highlight cases like this issue (where a change was made) after that.

Here are some recent examples for reference:
https://www.drupal.org/drupal-7.40-release-notes (which had some changes)
https://www.drupal.org/drupal-7.41-release-notes (which did not)

Status: Fixed » Closed (fixed)

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