Last modified: 2014-05-08 10:42:13 UTC
Filing as a security bug to be paranoid, but I don't really think its actually that much of a security issue. Worst case is someone could get a version of a deleted file, if they planned ahead before it was deleted. Example: https://upload.wikimedia.org/wikipedia/test/archive/b/b3/20130709191600!Bawolff-test-del.jpg returns the file, despite https://test.wikipedia.org/w/index.php?title=File:Bawolff-test-del.jpg It appears that we send HTCP purges for old version thumbnails, but not the actual file asset itself. Steps to reproduce: *Upload some file *Overwrite it with something *In the file description page, click on the old version of the image, thus loading the full resolution version of the old file (and getting it in varnish cache) *Delete the file *The old full resolution link still works as its still in varnish cache. ---- I'm working on a patch for this, and I'll post it to the bug when done.
Hi Brian, I'm assuming you're making progress on this, but let me know if you need help. Thanks!
bawolff: How are things going? Any news, or do you need help from somebody (who)?
Bryan is going to try having a go at it, since this is relatively self contained.
Reproduced on test2 with: * upload https://test2.wikipedia.org/wiki/File:BDavis-Our-Beach.jpeg * upload replacement with image flipped upside down * Open first version: https://upload.wikimedia.org/wikipedia/test2/archive/f/f0/20130814212216%21BDavis-Our-Beach.jpeg * Delete first version * Hard reload https://upload.wikimedia.org/wikipedia/test2/archive/f/f0/20130814212216%21BDavis-Our-Beach.jpeg Response Headers HTTP/1.1 304 Not Modified Server: nginx/1.1.19 Date: Wed, 14 Aug 2013 21:37:56 GMT Content-Type: image/jpeg Connection: keep-alive X-Object-Meta-Sha1base36: q6tjiuuvytupp30yb76eav509pl464c Last-Modified: Wed, 14 Aug 2013 21:22:16 GMT Etag: f046823df66ca83b706e67c31a6d2420 X-Timestamp: 1376515336.85862 x-varnish: 176829175 176335682, 3224550997 Via: 1.1 varnish, 1.1 varnish Accept-Ranges: bytes Age: 242 X-Cache: cp1061 hit (5), cp1062 frontend miss (0) access-control-allow-origin: *
Created attachment 13103 [details] Call SquidUpdate::purge() when deleteing I think this patch will send the appropriate squid/varnish purge messages. I tested it locally by running a sniffer to watch for the HTCP packets being sent. You can get the sniffer I used from https://github.com/bd808/htcpsnoop. I have not tested with an actual squid/varnish cache layer.
I almost posted a review to gerrit and then I remembered this was marked as "security".
Aaron, can you look at the patch too, since this is part of filerepo?
I've tested and read over this patch. It looks good to me. Some notes: *body of the foreach loop should be indented *There's still a code path with moving files where the old name of the file isn't purged, but that could be a separate issue, since we're not trying to hide the old filename, and long term we want the old filename to give an http redirect anyhow, so really this code path probably doesn't matter. *The revision delete code path (RevDel_FileList::doPostCommitUpdates) still has this issue. This is a much more important code path then the move file (This includes things like oversight of files), but still could be considered a mildly separate bug. Nonetheless, I would recommend fixing this code path at the same time.
> *body of the foreach loop should be indented Easy fix. I think my current listchars setting in vim is making it hard for me to see some of these indent issues. I'll work on that. > *There's still a code path with moving files where the old name of the file > isn't purged, but that could be a separate issue, since we're not trying to > hide the old filename, and long term we want the old filename to give an http > redirect anyhow, so really this code path probably doesn't matter. I think that the call to purgeEverything() in move() _should_ purge the squid. File::purgeEverything() calls LocalFile::purgeCache() which in turn calls SquidUpdate::purge(array($this->getURL())). > *The revision delete code path (RevDel_FileList::doPostCommitUpdates) still > has this issue. This is a much more important code path then the move file > (This includes things like oversight of files), but still could be considered > a mildly separate bug. Nonetheless, I would recommend fixing this code path > at the same time. I'm looking at this now. Unfortunately the level of abstraction here and my very limited understanding of the codebase may make this take longer than it could/should. The original patch took me 2 days to create. :)
Created attachment 13129 [details] Call SquidUpdate::purge() when deleteing Updated patch that fixes indent issue and adds a call to SquidUpdate::purge() from RevDel_FileList::doPostCommitUpdates().
(In reply to comment #10) > Created attachment 13129 [details] > Call SquidUpdate::purge() when deleteing > > Updated patch that fixes indent issue and adds a call to SquidUpdate::purge() > from RevDel_FileList::doPostCommitUpdates(). This looks good to me
Change 79842 had a related patch set uploaded by BryanDavis: Purge upstream caches when deleting file assets. https://gerrit.wikimedia.org/r/79842
Change 79842 merged by jenkins-bot: Purge upstream caches when deleting file assets. https://gerrit.wikimedia.org/r/79842
Change 79843 had a related patch set uploaded by BryanDavis: Purge upstream caches when deleting file assets. https://gerrit.wikimedia.org/r/79843
Change 79844 had a related patch set uploaded by BryanDavis: Purge upstream caches when deleting file assets. https://gerrit.wikimedia.org/r/79844
The patch for master has been merged, so marking fix. The patches for stable still need to be merged.
Change 79915 had a related patch set uploaded by BryanDavis: Release note update for bug 51064. https://gerrit.wikimedia.org/r/79915
Change 79914 had a related patch set uploaded by BryanDavis: Release note update for bug 51064. https://gerrit.wikimedia.org/r/79914
Change 79918 had a related patch set uploaded by BryanDavis: Release note update for bug 51064. https://gerrit.wikimedia.org/r/79918
Change 79914 merged by jenkins-bot: Release note update for bug 51064. https://gerrit.wikimedia.org/r/79914
Chris: did you want me to merge the backports of Bryan's patch?
Yes please!
Change 79844 merged by jenkins-bot: Purge upstream caches when deleting file assets. https://gerrit.wikimedia.org/r/79844
Change 79915 merged by jenkins-bot: Release note update for bug 51064. https://gerrit.wikimedia.org/r/79915
Change 79843 merged by Brian Wolff: Purge upstream caches when deleting file assets. https://gerrit.wikimedia.org/r/79843
Change 79918 merged by jenkins-bot: Release note update for bug 51064. https://gerrit.wikimedia.org/r/79918
Change 80515 had a related patch set uploaded by Brian Wolff: Backport purge upstream caches when deleting file assets. https://gerrit.wikimedia.org/r/80515
Change 80515 merged by jenkins-bot: Backport purge upstream caches when deleting file assets. https://gerrit.wikimedia.org/r/80515
