Problem/Motivation

The core security update added a download access permission check, we are seeing cases in our behat tests where that can break file uploads, combined with validation errors.

Steps to reproduce

Proposed resolution

Add the same always-allow-download-of-public-files access check that core has. This might break some tests but it probably makes sense as it is technically true.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#18 file-entity-public-download-3171480-18-interdiff.txt620 bytesberdir
#18 file-entity-public-download-3171480-18.patch4.72 KBberdir
#15 file-entity-public-download-3171480-15-interdiff.txt2.11 KBberdir
#15 file-entity-public-download-3171480-15.patch4.35 KBberdir
#13 file-entity-public-download-3171480-11-interdiff.txt2.34 KBberdir
#13 file-entity-public-download-3171480-11.patch3.1 KBberdir
#11 file-entity-public-download-3171480-11.patch780 bytesberdir
#2 file-entity-public-download-3171480-2.patch780 bytesberdir

Comments

Berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new file-entity-public-download-3171480-2.patch780 bytes

This fixes it for us.

Status: Needs review » Needs work

The last submitted patch, 2: file-entity-public-download-3171480-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Status: Needs work » Reviewed & tested by the community

makes sense,
as for the test failures, HEAD currently has 4 fails on D9.1 30 pass, 4 fail

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Reviewed & tested by the community » Needs work

Thanks, but those fails are definitely related, they are not on HEAD. 9.1 might have broken some other tests.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

One problem is that we use download to control access to the download operation, which isn't exactly the same, but I guess we have to drop that separation and update the tests.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Issue tags: +SA-CORE-2020-009
joseph.olstad’s picture

joseph.olstad
Primary language French
commented

the SA-CORE-2020-009 seems to affect media in D7, with solution to upgrade from jQuery 1.4/1.5 to 1.6 or higher.

would otherwise be a strange co-incidence.

maybe file_entity involved also (medua D7 requires file_entity), I haven't had a chance to closely look at it yet.
#3171590: Mediabrowser breaks after updating to Drupal 7.73

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: File upload broken im some cases for anonymous users after SA-CORE-2020-009 » File upload broken im some cases for anonymous users after SA-CORE-2020-011
Issue tags: -SA-CORE-2020-009

Sorry, I got the wrong SA here, this isn't about the ajax thing but file access and is therefore not related to the D7 issues.

solideogloria’s picture

solideogloria commented
Title: File upload broken im some cases for anonymous users after SA-CORE-2020-011 » File upload broken in some cases for anonymous users after SA-CORE-2020-011
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new file-entity-public-download-3171480-11.patch780 bytes

Turns out we have failing tests because of this, so I don't need to add new test coverage. Fixed the comment, tests should be green again now.

Status: Needs review » Needs work

The last submitted patch, 11: file-entity-public-download-3171480-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new file-entity-public-download-3171480-11.patch3.1 KB
new file-entity-public-download-3171480-11-interdiff.txt2.34 KB

Adjusting tests to the fact that you can now always download public files.

Status: Needs review » Needs work

The last submitted patch, 13: file-entity-public-download-3171480-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new file-entity-public-download-3171480-15.patch4.35 KB
new file-entity-public-download-3171480-15-interdiff.txt2.11 KB

Missed one failing test, a bit a messy situtation, but I don't think we have a choice.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Would be great to keep test for 403 for private files additionally check that public files are always downloadable

Status: Needs review » Needs work

The last submitted patch, 15: file-entity-public-download-3171480-15.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new file-entity-public-download-3171480-18.patch4.72 KB
new file-entity-public-download-3171480-18-interdiff.txt620 bytes

Private files have their own tests, but I did manage to convert that last failing test to a private file and verified that still works.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Status: Needs review » Reviewed & tested by the community

Looks to me like this issue is resolved.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Reviewed & tested by the community » Fixed

Committed.

  • Berdir committed 537daa7 on 8.x-2.x 
    Issue #3171480 by Berdir: File upload broken in some cases for anonymous...

Status: Fixed » Closed (fixed)

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