Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
orcomposer.lock
directly from the web - malicious visitors could use the information in the files to start looking for vulnerabilities
Beta phase evaluation
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:
- Disallowing search engines from indexing
composer.json
andcomposer.lock
by adding them torobots.txt
- Blocking access to
composer.json
andcomposer.lock
using.htaccess
andweb.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
Decide on the path moving forwards.
We decided to update.htaccess
andweb.config
Write a patch for D8.Update patch to add tests.- Review and RTBC.
- Commit to D8, set Status to Patch (to be ported), change Version to
7.x-dev
. - Post patch to D7 (see #10).
Note that @mparker17 could not find any automated tests in D7 that checked.htaccess
rules. - Review and RTBC.
- Commit to D7.
- Decide if it's worth porting to D6?
User interface changes
None.
API changes
None
Comment | File | Size | Author |
---|---|---|---|
#28 | disallow_composer_json-2392153-28.patch | 1.52 KB | hussainweb |
#22 | disallow_composer_json-2392153-22.patch | 2.51 KB | chris.smith |
Comments
Comment #1
hussainwebComment #2
hussainwebImplementing patch as per option 1. This should be a quickfix and I don't see any disruption.
Comment #3
dawehnerI 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.
Comment #4
hussainwebSounds 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.
Comment #5
tstoecklerYup, this makes the files unaccessbile via the web on my install. Having the additional layer in robots.txt makes sense IMO -> RTBC
Comment #6
alexpottAdding to both .htaccess and robots.txt is overkill. Plus we should add to web.config.
Comment #7
hussainwebAgreed. 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:
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.
Comment #8
Mile23Comment #9
saltednutUpdated 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.
Comment #10
mparker17Backporting 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).Comment #11
mparker17I can't think of a good reason why someone would want to access
composer.json
orcomposer.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
andweb.config
.Looking at
robots.txt
, I do not see any lines which correspond with blocking rules in.htaccess
orweb.config
. Based on this precedent, I do not think that it's necessary to disallow search engine access tocomposer.(json|lock)
inrobots.txt
as well.Comment #12
mparker17Missed the change to
web.config
in #10.Comment #13
alexpottWe can improve Drupal\system\Tests\System\HtaccessTest to test this.
Comment #14
mparker17Added
composer.json
andcomposer.lock
to the list of protected files to test in\Drupal\system\Tests\System\HtaccessTest::testFileAccess()
.Comment #16
mparker17This should work.
Comment #17
mparker17Updated issue summary to keep it up-to-date with the latest decisions in the comment section.
Comment #18
mparker17Whoops, used
code
tags instead ofdel
tags on Remaining Tasks that were Done.Comment #19
cilefen CreditAttribution: cilefen commented+1 for this change
Comment #20
meba CreditAttribution: meba commentedI 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.
Comment #21
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedpatch doesn't apply
Comment #22
chris.smith CreditAttribution: chris.smith at Portage CyberTech commentedRerolled 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.
Comment #23
naveenvalechapatch rerolled in #22
Comment #24
dawehnerWe have test coverage, so this looks perfectly for me.
Comment #25
alexpottCommitted 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.
Comment #28
hussainwebMaking the changes. I can't find an existing test for this. Is there one in Drupal 7?
Comment #29
naveenvalechaIs there one in Drupal 7?
We don't have test coverage, this looks good to be in.
Comment #30
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedYeah, I don't think we need automated tests for something like this.
Committed to 7.x - thanks!
Comment #32
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis 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)