file_download returns 404 instead of 403 [#1221214]

As learned during debugging #1179426: File module security fixes from SA-CORE-2011-001 not yet applied to Drupal 8 file_download will return a 404 if no modules respond with headers. That's incorrect -- we know the file exists as the function checks for it already, we need to return 403 instead of a 404. Patch needs doxygen changes, possibly tests -- although writing tests are borderline trivial after the other issue went in because we just need to flip the final assertResponse to 403.

Comment	File	Size	Author
#5	1221214-5.patch	1.87 KB	chx
#5
	file_download_drupal_access_denied.patch	325 bytes	chx

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

chx CreditAttribution: chx commented 17 July 2011 at 10:28

Version:	7.x-dev	» 8.x-dev
Status:	Active	» Needs review

Comment #2

aaron CreditAttribution: aaron commented 22 July 2011 at 19:49

file_download_drupal_access_denied.patch queued for re-testing.

Comment #3

aaron CreditAttribution: aaron commented 22 July 2011 at 20:09

Status:	Needs review	» Reviewed & tested by the community
Issue tags:		+Needs tests, +Needs documentation

works as advertised.

Comment #4

aaron CreditAttribution: aaron commented 22 July 2011 at 20:13

chx, what test needs changing? i only see the following, which looks correct to me:

    // Deny access to all downloads via a -1 header.
    file_test_set_return('download', -1);
    $this->drupalHead($url);
    $this->assertResponse(403, t('Correctly denied access to a file when file_test sets the header to -1.'));

    // Try non-existent file.
    $url = file_create_url('private://' . $this->randomName());
    $this->drupalHead($url);
    $this->assertResponse(404, t('Correctly returned 404 response for a non-existent file.'));

Comment #5

chx CreditAttribution: chx commented 22 July 2011 at 21:13

Issue tags:

-Needs tests, -Needs documentation

File	Size
1221214-5.patch	1.87 KB

Comment #6

chx CreditAttribution: chx commented 22 July 2011 at 21:13

Status:

Reviewed & tested by the community

» Needs review

Comment #7

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 22 July 2011 at 21:23

I looked at the surrounding code and this patch really does make a lot of sense, and is self-documented.
The whole file API is a bit out of my league, though, so leaving for another review before RTBC.