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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mike503’s picture

Around 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.

mike503’s picture

So, 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)

mike503’s picture

By commenting out line 318 in modules/image/image.module

-  if ($file->status) {          
       return file_file_download($uri, 'image');
-  }

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...

mike503’s picture

Title: Image styles not created on-demand after upload with private files » Image styles are not created for private images by default
Priority: Major » Critical
mike503’s picture

Note, this has been reproduced by someone on IRC as well who was running HEAD.

effulgentsia’s picture

Title: Image styles are not created for private images by default » Private image newly uploaded to an image field fails to display a thumbnail preview until the node is saved
Version: 7.0-beta1 » 7.x-dev
Component: image system » image.module
Priority: Critical » Normal

This 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.

mike503’s picture

Priority: Normal » Major

How 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.

droplet’s picture

agree 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.

mike503’s picture

I 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.

mike503’s picture

The 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 :)

eojthebrave’s picture

Status: Active » Needs review
FileSize
472 bytes

So 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.

lotyrin’s picture

Status: Needs review » Reviewed & tested by the community

This solves the issue.

effulgentsia’s picture

Component: image.module » file system
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs security review

It does, but I'm not sure it's safe. Would be great to get a security evaluation of this.

lotyrin’s picture

Issue tags: +Needs tests

I was realizing it could also probably use a test.

adrinux’s picture

sub.

Pancho’s picture

Confirm patch #11 fixes the issue.

sun’s picture

Version: 7.x-dev » 8.x-dev
Priority: Major » Normal
Issue tags: +Needs backport to D7

Normal, since there's a simple workaround: Use the public filesystem.

Kars-T’s picture

Confirm 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.

webflo’s picture

The patch in #11 bypass file access for all orphan files. Maybe this is not a good idea.

eule’s picture

is this related to http://drupal.org/node/1228890 ?

AdamGerthel’s picture

@ #20 probably not unless it uses private file system. It's likely related to read/write permissions on the file system.

jantimon’s picture

@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.

Status: Needs review » Needs work
Issue tags: -Needs tests, -Needs security review, -Needs backport to D7

The last submitted patch, file-remove_ref_check_for_temp_files-943924-22.patch, failed testing.

jantimon’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +Needs security review, +Needs backport to D7
jantimon’s picture

eojthebrave’s picture

Status: Needs review » Needs work

Oh. 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" ...

lotyrin’s picture

My 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.

lotyrin’s picture

Also, do we really need to explicitly allow user 1? If we do, why? And why wouldn't we allow other admin users?

lotyrin’s picture

Title: Private image newly uploaded to an image field fails to display a thumbnail preview until the node is saved » Allow preview of private image on node add form
Status: Needs work » Needs review
FileSize
1.39 KB

Patch with my suggested changes

lotyrin’s picture

Further improvement to the comment.

Kars-T’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

chx’s picture

That'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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

This 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!

Status: Fixed » Closed (fixed)
Issue tags: -Needs tests, -Needs security review, -Needs backport to D7

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