The code added in https://www.drupal.org/SA-CORE-2017-003 required writing to the session every time an anonymous user uploads a private file, and also reading from that session to check if the current anonymous user is the same one who uploaded it and should have access to it.
For simplicity in a security release, this was done using a hardcoded string and a direct write to the session. But it would be useful to have more of API for this. There were two possible suggestions related to that:
- @larowlan suggested replacing the string with a class constant.
- I suggested creating an actual API to grant/check anonymous access to the file, and putting all the session code internal to that API. I suggested this partially because we do know of at least a couple contrib modules that need to deal with this functionality also (see the https://www.drupal.org/project/drupal/releases/7.56 release notes).
The options aren't mutually exclusive (it is possible to do both).
Comment | File | Size | Author |
---|---|---|---|
#18 | 2888592-nr-bot.txt | 144 bytes | needs-review-queue-bot |
#13 | 2888592-13.patch | 2.65 KB | ranjith_kumar_k_u |
#2 | 2888592-2.patch | 2.6 KB | David_Rothstein |
Comments
Comment #2
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI was not a huge fan of the first option since I thought the benefits were minor and it had a little too much in common with ideas like #260226: Replace variable string literals with defined constants which were rejected in the past.
That said, it was easy to write, and I wrote code for it while working on the security issue until we decided to pull that out and just go with the minimal fix in the security release that didn't add anything to the API.
So, here's the code I wrote as a patch.
Comment #4
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedWe might consider backporting this to Drupal 7, especially if we go with option 2 in the issue summary.
Comment #5
larowlanHappy either way, a lightweight 'AnonymousFileAccessCheck' service could internalize this detail.
People could then swap out a session implementation for something else.
Comment #13
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedRe-rolled for 9.2
Comment #18
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.