The solution to enabling the color picker to work when the file download method is private is to put the colors in the configuration directory instead of the files directory. Having all of a site's settings files in one directory makes more sense, too.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Darren Oh’s picture

Status: Active » Needs review
FileSize
1.72 KB

Patch attached.

Darren Oh’s picture

Title: Don't disable color picker when download method is private » Don't disable color picker for private downloads
cog.rusty’s picture

Title: Don't disable color picker for private downloads » Don't disable color picker when download method is private

Just for not repeating things which have already been said, a link to the old discussion:
http://drupal.org/node/92059

+1 for the idea of an always public place for some Drupal generated files, and the conf directory seems the right place for this.

It is a limited implementation of Steven's case (1) in http://drupal.org/node/92059#comment-151727 Not yet a "hybrid download method" but it would serve well.

Perhaps it can take some generalization. Giving to an optional module like 'color' a whole subdirectory of its own directly under 'sites' seems... asymmetric. What about 'drupal_files/color'?

Darren Oh’s picture

It doesn't get its own directory under sites. It gets a directory under the configuration directory for a particular site. For example:

sites/
default/
colors/
generated color files
settings.php
example.com/
colors/
generated color files
settings.php

I would avoid putting configuration files in a files directory from now on. The case for having both public and private file downloads doesn't need to include configuration files.

cog.rusty’s picture

I agree that this kind of files can be completely separated from the user files.
That was a typo. I meant the same thing. You have:

sites/example.com/modules
sites/example.com/themes
sites/example.com/color

Not to disparage the color module, but it doesn't deserve that place. It also sets a bad example for other modules' developers, who may feel like adding a few directories there. On the other hand,

sites/example.com/drupal_files

can see eye to eye with 'modules' and 'themes' and can contain 'color'.

Darren Oh’s picture

FileSize
1.76 KB

drupal_files is an awkward name to me. If we decide that to always use a files directory in the site's configuration directory, instead of the directory returned by file_directory_path(), I think the problem would be solved. Even if it is used as the user files directory and downloads are set to private, files in that directory are actually public.

drumm’s picture

Status: Needs review » Closed (works as designed)

I do not think this is a good change for the stable 5.x branch. There is quite a bit of infrastructure around helping users get the permissions right on the usual file upload directory, such as the line in admin/logs/status. None of this, except for a bit around the installer, exists for the sites/... directory.

Additionally, we really do not want to have files being written to the sites/... directories outside of the installer and should keep the filesystem permissions read-only. Any potential writing to settings.php or encouraging surrounding files to have less-secure filesystem permissions would be a security problem.

Darren Oh’s picture

Title: Don't disable color picker when download method is private » Color picker promotes poor security
Category: task » bug
Status: Closed (works as designed) » Active

The color module requiring people to make their files directory public is also a security problem.

hass’s picture

It looks like we get private and public files handling in D7... so maybe won't fix!?

Gábor Hojtsy’s picture

Only won't fix once we really have public/private files at the same time committed to core :)

Pancho’s picture

Title: Color picker promotes poor security » color.module promotes poor security
Version: 5.x-dev » 6.x-dev

This is still active and it is a security problem. Even that late in the release cycle, we should try to get this fixed.

Drumm's argument in #7 doesn't seem to apply anymore, as we already moved the files folder to sites/default/files.
Adding another folder sites/default/system-files for system generated files (at the moment: color.module) is no additional security risk. We should do that.

Pancho’s picture

Title: color.module promotes poor security » Core features promotes poor security
Component: color.module » base system

This is not only about color.module, but also CSS and JS aggregation. We should put these files as well into sites/default/system-files.

Gábor Hojtsy’s picture

Version: 6.x-dev » 7.x-dev

Introducing a new folder for system generated files is unfortunately out of question in Drupal 6, since it is quite late.

Pancho’s picture

Version: 7.x-dev » 6.x-dev

Okay. I also found in a related issue a striking argument against private download method for all these files (color.module, aggregated css and js):

Steven pointed out that

using the color picker with private files is not practical because private files require a full bootstrap to serve the file. This would be for the CSS file, for the images (more than a dozen afaik), etc. And it would not just be the first load, but also all subsequent pages too, because the browser needs to check if its copy of the image/css is still recent. So, it would essentially kill your site, or at least make it so slow as to be unusable.

Too bad, otherwise it could be solved by a temporary switch to public download method.

Still, this is a major security, functionality and usability bug, that I was tempted to mark critical (Only left it at normal because of my record of false critical issues lately... :). This is no new functionality and we don't necessarily need to introduce new strings, so I don't really see the point in postponing this to D7.

hass’s picture

Version: 6.x-dev » 7.x-dev

We hopefully get private and public download in D7... in 2007 we have had a Google Summer of Code student working on it...

gpk’s picture

@Pancho: Don't forget there is a "fudged" work-around for simultaneous private and public downloads that works in D5 and should in D6 - namely, with private downloads enabled, keep the files folder within the web root and use URLs such as /files/... (or /sites/example.com/files/...) for public download and /system/files for private. Obviously this has severe limitations (not least that system generated URLs are nearly always private and also that if you know how then you can get any private file using "public" http access) but for some sites it is adequte.

Then to use color.module you can temporarily enable public downloads, do your color.module stuff, switch back to private, and the updated color scheme still works :-) since it is one of the rare instances when the generated URLs are always "public"; of course you can't change the color scheme unless you go back to private. See also http://drupal.org/handbook/modules/color#comment-657434.

BTW I think drumm's comments at #7 still largely apply since it is only sites/example.com/files that is left writable by the server. There was enough hassle making just this one folder work nicely/be writable etc. during the installation process. See http://drupal.org/node/194369.

zeno of elea’s picture

I was redirected here by the bottom of "lets not reopen this old issue"

http://drupal.org/node/92059

It's not an old issue. I got drupal 6, unzipped and uploaded to my webserver. Immediately I have a problem with the files directory as it's owner is listed as apache, I change the owner to my web user and it works. Then if I try and change the color of garland my theme disappears.

?!?!?

cog.rusty’s picture

The expected behavior of the color module (or any other php script which writes files) is to try to create its subdirectories and files as the web server user, usually 'apache' or 'nobody'.

To succeed, your files directory must be:
- either owned by user apache and having the normal 755 or 775 permissions
- or owned by your own user account and having 777 permissions (Watch out for this!)
Some, but not most, server configurations are more permissive.

The same must be true about your temp directory.

Additionally, php must not be configured in "safe mode", or else the operation may fail anyway (depending on other details of the configuration).

Is this still an issue?

birdmanx35’s picture

This is still an issue- it's still referenced in core :P

birdmanx35’s picture

Status: Active » Needs review
catch’s picture

Status: Needs review » Needs work

No longer applies.

catch’s picture

Title: Core features promotes poor security » color.module and private file downloads
arhak’s picture

subscribing

arhak’s picture

What about JavaScript translation? They seems to break when download method is set to private. The content is actually translated and the .js file is saved, BUT in a private folder.

I think is crucial to have a public directory under installation to handle several cases generally related to css/js and nothing to do with the concept of a private download method.

I wasn't aware of this discussion until today (also it's titled color.module which is the less important issue because it's a matter of fanciness), so I did a proof of concept for js aggregation and js translation.
I apologizes for the decisions taken weren't the best thought, but it was just a matter of testing. So, I provided a working patch for Drupal 6.3 at http://drupal.org/node/296831#comment-969381 which fixes the following three issues:
1- css/js aggregation
2- color.module
3- js translations

Note: it's not posted as an issue because it's not intended to be adopted neither proposed as a patch, instead it's for testing it in a D6.3 installation and measure how huge would be introducing similar considerations into core.

BTW: the three solved issues can be tested separately or together, to get the whole picture start reading from http://drupal.org/node/296831#comment-969355

bdragon’s picture

Subscribing, this affects gmap as well.

arhak’s picture

Title: color.module and private file downloads » misc folder required when using private file downloads

seems that this issue is going nowhere
patching color.module won't solve many other related issues
is just one big issue: private download doesn't have nothing to do with some public css/js/images from other modules like core's js/css aggregation, js translation, color.module, and now gmap comes up too.
So, thinking about solving the problem from it's root?
or will the issue queues will full of similar "private download method" issues?

PS: Although think this should be ported to D6, currently I'm working with my own patch to core (which at the time was for D6.3)

captaingeek’s picture

issue expirenced here clean install of drupal 6.1 on iis7. attempting to change the garland theme color breaks theme until reverting back to the default color brings back the default theme but still can't change colors.

I get the following when attempting to change colors.

The configuration options have been saved.
The directory sites/default/files/color/garland-0d57831b has been created.
The selected file could not be copied, because no file by that name exists. Please check that you supplied the correct filename.

status page
Drupal 6.10
Access to update.php Protected
Configuration file Protected
Cron maintenance tasks Last run 23 min 42 sec ago
You can run cron manually.
Database updates Up to date
Drupal core update status No update data available
No information is available about potential new releases for currently installed modules and themes. To check for updates, you may need to run cron or you can check manually. Please note that checking for available updates can take a long time, so please be patient.
File system Writable (public download method)
GD library bundled (2.0.34 compatible)
MySQL database 5.1.31
PHP 5.2.8
PHP memory limit 256M
PHP register globals Disabled
Unicode library PHP Mbstring Extension
Update notifications Enabled
Web server Microsoft-IIS/7.0

zeropaper’s picture

I completely agree with #24 and deadly would like to see such feature being backported (if implemented in 7.x) within 6.x.
By the way, I guess having a public (for files who does not need authentication) and a private folder could spare some server resources.

arhak’s picture

another issue raised due to private download method #423652: Private file system can't see crop_display.jpg image.

arhak’s picture

contributed alternatives:
private_upload
private_download

arhak’s picture

path commented at drupal.org/node/296831
this version was against Drupal 6.12, but I think it's the same for 6.13

read comment #24 of this thread for description

arhak’s picture

@#31 missing files in the patch, now fixed

remember that this patch requires the creation of misc/dynamic directory

if the site was already running with other languages set,
javascript translations might be pointing the wrong location
therefore, a cleanup might be required:
- SQL UPDATE languages SET `javascript` = '';
- PHP locale_inc_callback('_locale_rebuild_js');

talino’s picture

Pardon the newbiness: I need to apply this patch because my JS translation file's link is revealing my host's full path to the private files folder in the HTML source. However, I've never applied a patch before. I tried this in my (local) root install dir:

patch -p0 < 2009.08.25-public_misc02_0.patch

But I got this in return:

(Stripping trailing CRs from patch.)
can't find file to patch at input line 4
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/includes/common.inc b/includes/common.inc
|--- a/includes/common.inc
|+++ b/includes/common.inc
--------------------------

Do I need to modify the 'a' and 'b' to point to something? And if so, what?

Thanks a lot.

arhak’s picture

@#33 sorry, I made the patch with GIT (actually with Mercurial), I'll be back to you as soon as I can

talino’s picture

Thanks for replying quickly. I think I managed. After going through the .patch file and looking for stuff to be changed, I copied all the needed files into a folder called 'a' in which I've created the Drupal structure, like this:

/includes/common.inc
/includes/file.inc
/includes/locale.inc
/modules/system/system.admin.inc
/modules/locale/locale.module
/modules/color/color.module

Then I copied the modified and .orig files created by the patch back into the Drupal folders. Created the /misc/dynamic folder and enabled write permissions.

Not the easiest way to do it, I suppose, but it worked.

Semi-worked, really, because I got rid of the JS translation problem which was my major concern (as in "the full unix path to my private folder displayed in the HTML source"). CSS & JS aggregation doesn't work for some reason (radios are enabled, though, in the admin interface) but I can live without it.

All the issues I've read so far concerning Drupal core pale in comparison to this one. A core module that doesn't handle Private Downloads and shows a full unix path in source? Hmm...

Thanks for your help.

Damien Tournoud’s picture

Does that still apply to D7 now that the stream wrappers are in? If not, please requalify as D6 only.

arhak’s picture

Version: 7.x-dev » 6.x-dev

@Damien Tournoud: getting confused, this issue has been changes fron D6 to D7 forward and back, already don't know whether would be right to change it again neither opening another issue, since they got marked as duplicated
according to #13 (which I agree) this patch won't be committed to D6 HEAD, since it introduces new features
nevertheless, I think there is another issue for public/private download support on D7 branch

arhak’s picture

@talino#35 I'm sorry to hear that you got so much trouble

here are the facts:
- I don't have CVS, I use Mercurial
- right now I'm working on Windows (my Linux PC will be unavailable for a while)
- the patch I provided was original made for Drupal 6.3, I recently got back to it on demand and it needs to be reviewed again, I'm attempted a quick remake for Drupal 6.13, but obviously it will need some work

Now I will attach the corrected patch
nevertheless it was made with diff (on Windows) which means:
1- the patch will get done without headaches
2- some warnings will show up but won't affect the patch process (e.g: (Stripping trailing CRs from patch.), succeeded at 1312 with fuzz 1. and (offset -1 lines).), which actually means it is not a 100% correct patch, but patch command is wise enough to handle it

Looking forward:
If you give me feedback I'll fix every feature the original patch was addressing
are you interested on it?

arhak’s picture

@talino#35 I'm sorry to hear that you got so much trouble

here are the facts:
- I don't have CVS, I use Mercurial
- right now I'm working on Windows (my Linux PC will be unavailable for a while)
- the patch I provided was original made for Drupal 6.3, I recently got back to it on demand and it needs to be reviewed again, I'm attempted a quick remake for Drupal 6.13, but obviously it will need some work

Now I will attach the corrected patch
nevertheless it was made with diff (on Windows) which means:
1- the patch will get done without headaches
2- some warnings will show up but won't affect the patch process (e.g: (Stripping trailing CRs from patch.), succeeded at 1312 with fuzz 1. and (offset -1 lines).), which actually means it is not a 100% correct patch, but patch command is wise enough to handle it

Looking forward:
If you give me feedback I'll fix every feature the original patch was addressing
are you interested on it?

talino’s picture

Thanks for helping out, arhak. I will be hopefully testing this tomorrow morning, Paris (France) time, and will add a comment as soon as I have anything useful to say.

arhak’s picture

I just test it and also reviewed the code
everything looks fine

it works for me
we should troubleshoot your case

are the folders misc/dynamic/css and misc/dynamic/js created once you enable css/js aggregation, maybe it wasn't able to write into misc/dynamic to create those folders

arhak’s picture

BTW patch #32 works fine using patch -p1 < 2009.08.25-public_misc02_0.patch

note that the -p1 discards the first element in paths, so you don't have to trick anything

PS: I recommend using patch #32
there is nothing wrong with patch #38 (it is just the same) but was intended to fix Windows encoding and use UTF-8 instead, which leads to wrong source tracking on Windows making the whole files appear as changed (#38 was also fixed to work with -p0 option instead of required -p1 for #32)

always remember to manually create misc/dynamic folder AND ensure that directory is writable, since it will need to create subdirectories css and js for css/js aggregation

arhak’s picture

for those looking for workarounds for private download support:

according to issue 146611#comment-1182361 at #146611: Allow JS/CSS aggregation in private mode D7 is expected to ship with public/private by file functionality, so creating a new folder for D6 was denied

nevertheless, making a public folder available to support features unavailable for private download method is a very affordable cost

so I'll be maintaining this as a path for D6 over this thread

arhak’s picture

Title: private download method and dynamically generated CSS and JS » misc folder required when using private file downloads
Version: 7.x-dev » 6.x-dev
Status: Active » Needs work
arhak’s picture

Assigned: Unassigned » arhak
Category: bug » feature
Status: Needs work » Needs review

just updating the issue state

talino’s picture

I'm happy to say that patch #38 worked fine when run from the Drupal root using:

patch -p0 < 2009.09.07-public_misc03_0.patch 

JS Translation link is gone, CSS & JS aggregation are enabled *and* functional. CSS & JS folders created in misc/dynamic.

Well done arhak! And thanks a lot for your work.

P.S. Will comment in this thread if there are any problems but everything seems OK on my end.

arhak’s picture

what did you mean with "JS Translation link is gone"?

arhak’s picture

Status: Needs review » Reviewed & tested by the community

@#47 now I recalled and read above, it was the reason for #33 ..because my JS translation file's link is revealing my host's full path to the private files folder in the HTML source.

so I already test it too

hass’s picture

Status: Reviewed & tested by the community » Needs review

Never make your own patches RTBC, please.

arhak’s picture

@#49 sorry about that, but I have seen the issues marked RTBC by authors once it is confirmed by them self and another user, and talino confirmed it at #46
also I have this patch running since Drupal 6.3 without any problem
it seems to me pretty much tested

also:
- last contribution to this thread (previous mine) was in October 7, 2007
- last interest before talino at #33 was in May 4, 2009 - 05:23
- I don't thing any body will come up with testing this issue besides the ones sticking with

talino’s picture

Sorry about #46 being unclear. "JS Translation link is gone" should've been "Wrong JS Translation link is gone", of course.

hass’s picture

A "Feature" cannot get into D6... so this may never go in - until it's a bug. New features only go into head and are backported afterwards. In past it was a bug, I don't know why you have changed it. I think you need to find a trustworthy core reviewer. :-)

arhak’s picture

@#52 please, it is a long story, read the whole thread
it is a bug which won't be fixed on D6, it is a new feature on D7 and that's why it won't be backported

#11 January 15, 2008 the issue targets D6 branch
#13, #14, #15 switch between D6<->D7 back and forward, but ends in D7
#26 August 18, 2008 I got involved, generalized the issue and pointed it as a "going nowhere" issue and left a link to the tweaking patch for D 6.3
#28 May 4, 2009 last activity before I started providing the path on this thread
#31 August 26, 2009 I attached the (previously referenced) alternative patch
#36 asked to qualify the issue as D6 only, due to existing D7 issues
#37 I start getting lost with what would be the right thing to do, and from this point I decided to take care of this patch providing proper update to it, it won't be ever committed to D6 head, which is not a reason for stopping providing it

please, let this issue be, the same as #7881 has lived for a long while being a new feature hacked into core

for actual D7 issues and other duplicates you may follow link at #43 & #44

hass’s picture

Sounds like we need to give this case a WONT FIX status. Feal free to change it yourself. We do not need stale cases in the queue that never get fixed.

Damien Tournoud’s picture

Version: 6.x-dev » 7.x-dev
Assigned: arhak » Unassigned
Category: feature » bug

I asked in #36 to clarify if this issue still applies to D7, given the recent changes to the file handling. Bumping to D7 until we have a confirmation that this is not an issue anymore.

Damien Tournoud’s picture

Title: misc folder required when using private file downloads » Private file downloads and dynamically generated CSS and JS

Clarifying the title.

arhak’s picture

Title: Private file downloads and dynamically generated CSS and JS » Private download method and dynamically generated CSS/JS and JS translations
Version: 7.x-dev » 6.x-dev
Status: Needs review » Closed (won't fix)

#55 no, please read #43 & #44
if you mark this as D7 it would become a duplicate

for those interested on a D 6.13 patch go to #39

arhak’s picture

Title: Private download method and dynamically generated CSS/JS and JS translations » private download method and dynamically generated CSS and JS

just making the title contains "private download method" string

Damien Tournoud’s picture

Title: private download method and dynamically generated CSS and JS » Private download method and dynamically generated CSS/JS and JS translations
Version: 6.x-dev » 7.x-dev
Status: Closed (won't fix) » Active

Please answer my question: given that we have now a stream-wrapper based file handling in D7, are the color module, JS translations, etc. all work flawlessly regardless the "Default download method"?

arhak’s picture

Title: Private download method and dynamically generated CSS/JS and JS translations » private download method and dynamically generated CSS and JS

@#59 I really don't know, I haven't test it, I'm exporadically testing D7 tarball, I'm not following D7 development nearest than that

D5: #298180: Allow CSS Aggregation to Work with Private File Mode
D7: #298190: Allow CSS Aggregation to Work with Private File Mode
both of them marked as duplicate pointing to #146611: Allow JS/CSS aggregation in private mode
which in turns point to #166759: Public/Private File Handling
and from there to #517814: File API Stream Wrapper Conversion

arhak’s picture

Title: misc folder required when using private file downloads » private download method and dynamically generated CSS and JS
Version: 6.x-dev » 7.x-dev
Status: Needs work » Active

Status: Active » Needs review

Re-test of 2009.09.07-public_misc03.patch from comment #39 was requested by mario_prkos.

Status: Needs review » Needs work

The last submitted patch, 2009.09.07-public_misc03.patch, failed testing.

arhak’s picture

Status: Needs work » Postponed (maintainer needs more info)

@#62 as far as I know, none of the attached patches aims D7
Damien asked if this is present in D7 @#59, but no one has answered that
don't waste time hitting the test bot if a D7 patch doesn't exists on this thread

kewlguy’s picture

Status: Postponed (maintainer needs more info) » Needs review

#39: 2009.09.07-public_misc03.patch queued for re-testing.

arhak’s picture

@#65 don't bother the bot with D6 patches (I forgot to put a .D6.patch suffix)
I can't hide those attachments by editing the comments
whoever wants more info regarding it #572516: make private download method support css/js aggregation, color module and js translations

OTOH the issue discussed in this thread has no proposed patch for D7
patch @#6 was for color module only (don't know if aimed D6 or D7)

arhak’s picture

Status: Needs review » Postponed (maintainer needs more info)
arhak’s picture

Issue tags: +Ancient

tag

cog.rusty’s picture

There is nothing to wait from Drupal 7. Drupal 7 has solved this kind of problems by supporting public and private downloads at the same time. If it has been decided that the feature won't be added to Drupal 6, then the issue can be set to "won't fix" and closed.

Of course the submitted patch can be useful to someone in any case.

Damien Tournoud’s picture

Version: 7.x-dev » 6.x-dev
Status: Postponed (maintainer needs more info) » Closed (won't fix)
Issue tags: -Ancient

Agreed with #69.

guidot’s picture

Version: 6.x-dev » 6.17
FileSize
12.13 KB

Updated patch from #38 against Drupal 6.17. Now with Unix line endings. Still some fuzz in it. Didn't brake my site yet, BUT might break your's! So use with care. Apply with patch -p0 < css+js-aggregation-D6.17.patch.

arhak’s picture

z7’s picture

FileSize
1.23 KB

Some time ago I created a module that does basically 2 things: allows including 3rd party libraries, like Zend Framework easily and enables dynamically generated CSS and JS while using private download mode. It is all done through Drupal hooks, so no core patch is required and should work in any environment. The only thing to consider is that you'll need some .htaccess files to disallow access to private files in the download folder.