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.

CommentFileSizeAuthor
#5 1221214-5.patch1.87 KBchx
file_download_drupal_access_denied.patch325 byteschx

Comments

chx’s picture

chx commented
Version: 7.x-dev » 8.x-dev
Status: Active » Needs review
aaron’s picture

aaron commented

file_download_drupal_access_denied.patch queued for re-testing.

aaron’s picture

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

works as advertised.

aaron’s picture

aaron commented

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.'));
chx’s picture

chx commented
Issue tags: -Needs tests, -Needs documentation
StatusFileSize
new 1221214-5.patch1.87 KB
chx’s picture

chx commented
Status: Reviewed & tested by the community » Needs review
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

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.

aaron’s picture

aaron commented
Status: Needs review » Reviewed & tested by the community

great! thanks, chx

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs issue summary update

Tagging issues not yet using summary template.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 7.x and 8.x. Thanks chx.

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