Create a CCK image field
- change "Upload destination" to "Private files"
Go post a piece of content to this new content type
When an image is selected and "Upload" is clicked, the image comes back broken
- The original file is in the right path on the filesystem
- The function image_style_create_derivative() in includes/image.inc is not called on the first request
- After publishing the node that the image is in, it -still- isn't generated
- Navigate directly to the thumbnail'ed image - it finally hits the image_style_create_derivative() function and generates the file properly
Not sure why the original request (which it looks like -is- issued in Firebug) didn't generate the file though. It took this follow-up request to generate it.
Comments
Comment #1
mike503 CreditAttribution: mike503 commentedAround line 698 in modules/image/image.module - I can watchdog that - the code doesnt get executed when the image is being previewed for the first time. But when I load the image directly, or go back in later to edit the node, it executes that for the "just-in-time" image generation that image_style_create_derivative() provides.
Comment #2
mike503 CreditAttribution: mike503 commentedSo, after debugging this more it could be a chicken and egg problem:
For an image to be able to show up in the preview from an imagecache style approach, it has to be generated on the filesystem. However, it hasn't saved the full-sized image yet to the filesystem or in the database, so an attempt to generate a thumb off the temp file can't happen [yet] - that's why it only works once the file is properly uploaded.
So somehow, there needs to be a way that [private] temp files can still be uploaded and leverage this, but not leave behind the cruft from a temp file (I assume the public files option has the same idea, but I don't know enough about the process right now. I'm still manually going around trying to determine where things get triggered)
Comment #3
mike503 CreditAttribution: mike503 commentedBy commenting out line 318 in modules/image/image.module
I am able to properly show the file properly the first time around. It generates the file on the filesystem properly.
However, it does not delete that imagecache style file when it is removed.
I am not sure of the full ramifications of this though. First off, the cached file needs to be garbage collected on delete, also, I'm not sure if this is properly checking any user privileges.
Someone with full visibility on how file/image handling and private files work might know this a bit better...
Comment #4
mike503 CreditAttribution: mike503 commentedComment #5
mike503 CreditAttribution: mike503 commentedNote, this has been reproduced by someone on IRC as well who was running HEAD.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedThis appears fixed to me in latest dev, except for the much more limited bug reflected in the new issue title. Basically, the preview you normally get in the image field widget itself is broken until the node is saved. Unlike the original issue description, once the node is saved, all is fine, so no longer "critical". Please correct me if I'm wrong about this.
In fact, it's possible this remaining issue is a "by design", since maybe until the node is saved, we have no way of checking if you have access to its file.
Comment #7
mike503 CreditAttribution: mike503 commentedHow is this "by design"? You just inform your users "due to a limitation in the platform, the normal image preview you see will be a broken image until you actually save the article"
Why not just disable image preview then for private images? It's utterly pointless at that point.
I am upgrading this again to a "MAJOR" because if it is definitely a major bug, one reason I am having to go back to D6 is because of this.
Comment #8
droplet CreditAttribution: droplet commentedagree with @mike503.
but we can't disable image preview. it can save into tmp dir or any private first. when node saved, it can move into right dir.
Comment #9
mike503 CreditAttribution: mike503 commentedI don't want to disable image preview but that's just one of a few options available :(
The temp directory thing was something I was trying to figure out by changing my settings to public files and then forcing things into private file locations after, but the logic is all messed up and I am not that deep of a Drupal hacker. Especially with the file path changes to schemes in d7 it added to my confusion a lot more.
Comment #10
mike503 CreditAttribution: mike503 commentedThe issue seems to stem from this:
when an image is uploaded, it wants to show the thumb instantly. For user experience. Understandable.
However, when it is uploaded as a "private" type, it has no access to that yet.
Someone with better understanding of how to fix this chicken and egg problem could probably fix this with a handful of lines of code. I'd be willing to sponsor this, or if you're in Portland, buy you enough beer until you can't drink anymore.
Not sure if it makes sense to have all image uploads go to public:// first and then get moved to private:// on node save?
Please contact me if interested. mike503@gmail.com - say "I want to fix your Drupal bug" or something :)
Comment #11
eojthebraveSo here's what I think is going on here, and a really simple fix.
When you first attach an image using private:// storage to a node it attempts to display a preview of the image and fails to do so. The code responsible for displaying the image runs through the full gamut of file reference and access check functions inside of file_file_download() it checks to see if any modules are declaring references for the file and if none claim to do so it stops processing so that file_file_download does not output headers for files controlled by other modules. Since temporary files don't have any references declared yet (these are set by file_usage_add() when the node is actually saved) the function just returns before file_file_download() is able to set the appropriate headers necessary for delivering the preview image.
Attached patch fixes the issue for me.
Comment #12
lotyrin CreditAttribution: lotyrin commentedThis solves the issue.
Comment #13
effulgentsia CreditAttribution: effulgentsia commentedIt does, but I'm not sure it's safe. Would be great to get a security evaluation of this.
Comment #14
lotyrin CreditAttribution: lotyrin commentedI was realizing it could also probably use a test.
Comment #15
adrinux CreditAttribution: adrinux commentedsub.
Comment #16
PanchoConfirm patch #11 fixes the issue.
Comment #17
sunNormal, since there's a simple workaround: Use the public filesystem.
Comment #18
Kars-T CreditAttribution: Kars-T commentedConfirm patch #11 fixes the issue.
And I don't think that "Use the public filesystem" is a workaround as we loose any PHP access control.
Comment #19
webflo CreditAttribution: webflo commentedThe patch in #11 bypass file access for all orphan files. Maybe this is not a good idea.
Comment #20
eule CreditAttribution: eule commentedis this related to http://drupal.org/node/1228890 ?
Comment #21
AdamGerthel CreditAttribution: AdamGerthel commented@ #20 probably not unless it uses private file system. It's likely related to read/write permissions on the file system.
Comment #22
jantimon CreditAttribution: jantimon commented@18 Absolutely agreed.
See the reworked patch witch should fix the issue in a more secure way:
Temporary private files may be accessed only by their owners.
Comment #24
jantimon CreditAttribution: jantimon commented#11: file-remove_ref_check_for_temp_files-943924-11-eojthebrave.patch queued for re-testing.
Comment #25
jantimon CreditAttribution: jantimon commentedComment #26
eojthebraveOh. Nice. I like this solution. The comments could use a little work though. Can we make it more clear in the example that this is for a node that has a file attached to it but the node hasn't been saved yet.
Also "exception to this rule are" should be "exception to this rule is" ...
Comment #27
lotyrin CreditAttribution: lotyrin commentedMy two cents:
"Stop processing if there are no references to avoid returning headers for files controlled by other modules. The exception is a temporary file where the host entity has not yet been saved (for example, an image displayed on a node add form), in which case, we allow download by the file's owner and user 1."
saying "host entity has not yet been saved" then clarifying to "node hasn't been saved" isn't enough of an example to really be worth providing in my opinion, as we're just providing an example of an entity, not an example of the situation.
Also tweaked the backward structure of the first sentence.
I'm no English professor though; it could probably use some more eyes.
Comment #28
lotyrin CreditAttribution: lotyrin commentedAlso, do we really need to explicitly allow user 1? If we do, why? And why wouldn't we allow other admin users?
Comment #29
lotyrin CreditAttribution: lotyrin commentedPatch with my suggested changes
Comment #30
lotyrin CreditAttribution: lotyrin commentedFurther improvement to the comment.
Comment #31
Kars-T CreditAttribution: Kars-T commentedI did check the patch with D7.8 and xdebug. As written in the comment of this patch it will add a check to accept files with currently no references that have a permanent status and that belong to the current user. Don't know what more to write if I can do anything more please ask. I think this is a rather important bug to fix.
Comment #32
chx CreditAttribution: chx commentedThat's interesting. The issue has needs tests tag and yet it's RTBC. But, I am completely unsure how on earth could we test this so I am leaving at RTBC.
Comment #33
webchickThis seems like it could use look-over by the security team, but since chx signed off I assume this is ok.
Committed and pushed to 8.x and 7.x, plus a minor spacing adjustment. Thanks!