Problem/Motivation

Currently, images are only allowed on drupal.org to users with the "documentation team" role or higher (a role that is manually assigned). This creates the following problems:

  1. The masses not having images cripples our collaboration opportunities. We block participation of designers to play "Photoshop ping-pong", new members of our community adding helpful screenshots to our documentation, users are prevented from posting screenshots of errors they're experiencing, and module developers can't add screenshots to their own documentation.
  2. Our documentation has a reputation for generally looking like ass, because only a handful of people can put pictures in it. And once they do put pictures in it, only a very small group of people can edit it. Yet another lost collaboration opportunity.
  3. Because the "documentation team" role grants access to a "Documentation" input format with <img> turned on, membership in that role is almost entirely bogus. It's overloaded with people who do absolutely nothing on the docs team, but needed to post a screenshot somewhere at some point. This is problematic for the docs team in figuring out who's who.

Opening image posting up more has traditionally been opposed by the security and infrastructure teams, because:

  1. We want to switch Drupal.org to HTTPS at some point soon, and we don't want to deal with mixed content errors.
  2. Not-nice people can spy on drupal.org visitors by linking to remote images on their servers.
  3. People can upload not-nice images, such as porn.

Luckily, sun has worked out a solution for #1 and #2 (and we always have users.status = 0 for #3).

Proposed resolution

The Local image input filter module, which provides an input filter to munge img srcs to ensure that images are only rendered if they point to a locally hosted image. See http://drupal.org/node/528682#comment-4968978 for the full "spec" we came up with in IRC.

Here's a screenshot of it in action:

Only images that have relative paths are shown; the rest are red Xs.

Remaining tasks

Mainly what we need now are code reviews and testing. There is a test site set up at:

http://issue-summaries-drupal.redesign.devdrupal.org
Apache user/pass: drupal/drupal
Drupal user/pass: bananas/bananas

Specifically:

Deployment steps

  1. Add Local image input filter module to BZR.
  2. Commit and deploy the patch in #1274938: Un-hide the file attachments form against the Drupal.org customizations project.
  3. Enable the Local image input filter module.
  4. Go to admin/settings/filters/1 and turn on the "Restrict images to this site" filter.
  5. Go to admin/settings/filters/1/order and move "Restrict images to this site" just above "HTML corrector"
  6. Go to admin/settings/filters/1/configure and add <img> to the list of allowed tags.
  7. Go to admin/project/project-issue-settings and blank out the field for "Issue directory".
  8. Sing, dance, and rejoice! :D

User interface changes

  • Images that are referenced by the input format without a local, relative path, would turn into red Xs.
  • Book nodes would un-hide the "File attachments" fieldset on the add/edit form.
  • A new filter tip would appear in the "Input format" fieldset, describing this filter.

API changes

None.

Original report by mfer

Users can upload images to a comment on a issue. But, they aren't displayed. You need to be a site admin or on the documentation team to add the html to the body of the comment for it to display. This isn't a great workflow and you have to rely on the person inputting the image to be concerned with size, etc.

In an effort to work with design/UX people who do upload images and work visually can we get a different workflow that's better for images.

Here is what I envision.

  • A new upload field on issues and comments for images (gif, jpg, png).
  • These images are displayed as thumbnail at the bottom of the issue/comment.
  • When clicked the images are displayed in a modal/pop-up.

Thoughts?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

+918729381923 to this. AFAIK there's a module that does just this, although I can't remember what it's called....

eaton’s picture

Inline module. I've used it on a number of sites, and while its extended options are a little trippy, the basic functionality of entering [inline:1] and [inline:2] to embed attached files is a life saver. It also allows us to (potentially) ensure that only locally uploaded files are embedded. That's much safer than just allowing any image file from the web to be stuck into a queue.

tsi’s picture

Searched for the policy about embedding images into issues and got here.
+1 from me too.

dww’s picture

Title: Image displays on issues » Inline image displays without special permissions

About to mark #950414: Provide an inline filter for placing attached images into text areas duplicate with this. Trying to make this slightly more general -- I could see wanting to do similar things on book pages and other node types on d.o...

jhodgdon’s picture

tagging with the tags from the issue marked as duplicate

jhodgdon’s picture

Priority: Normal » Major

arianek told me in IRC today that this is the single-most important thing we need for the Docs team currently, so I'm bumping up the priority.

By the way, I've used the Insert module to insert images into WYSIWYG editors. But it just puts in an IMG tag, so that wouldn't help the problem of the input format not allowing IMG tags.

I guess the Inline module is something else -- would need to make sure its filter runs after the one that filters out the HTML tags that are not allowed.

arianek’s picture

Just came hunting for this issue to say what jhodgdon just posted. Docs admin role has really become completely meaningless as so many people now have it so they can maintain the ever growing number of docs pages with images. We really need to get this done.

I know webchick has a mostly done prototype on her local, which we tried to export into a feature a while back, but there was something that wasn't quite working. I will try and get a version of whatever she has in-progress if possible (after the conference, etc.) If for some reason she hasn't still got that, then we can start anew...

Note: the second part of this will be to reform the docs admin role. Once everyone can embed images, we'll probably want to revoke the role for nearly all users and figure out what permissions it makes sense to have for the role (or whether it's necessary at all).

MGParisi’s picture

Assigned: Unassigned » MGParisi

I will research a possible solution. Will respond when I find out if the idea I have works.

MGParisi’s picture

Assigned: MGParisi » Unassigned

HTML Purifier works at removing all tags that I could put into the image tag. It really does a number on it. It will also protect from hundreds of other attacks and will make our documents well formed.

http://drupal.org/project/htmlpurifier
http://htmlpurifier.org/

The possible deal breaker is that its "GNU Lesser" and that it takes a significant amount of processing compared to light weight solutions, but the number of Book Changes is relatively small so performance should not be an issue.

If you know Regular Expressions a custom filter will work and will be very simple solution to the security issue. A file attachment field that only accepts .png, .jpg and .gif will be a great way to store images locally (preventing dead links and remote website monitoring).

I can implement the HTMLPurifier module as I have used it for many years, but I don't feel comfortable moving beyond that in scope.

xjm’s picture

Ah, I've been looking for this issue as well. This would be very useful in issue queues as well as handbook pages.

sun’s picture

I don't understand what the issue with just adding the IMG tag to the existing HTML filter of core is?

HTML filter invokes filter_xss() already.

MGParisi’s picture

Edit: Conflict is fun! *cry*

LOL

greggles’s picture

There's a lot of potential problems with the img tag. Set src="http://drupal.org/logout" for fun. There's also a few dozen XSS methods possible with img tags depending on the browser of the visitor. filter_xss catches a handful of them, but not all. At this point we are relatively safe given the age of the browsers vulnerable to those attacks but it's still a slight concern.

There's also an invasion of privacy component with the normal img tag - someone can point the src to an image on their server and then known the IP of anyone who visits drupal.org.

There's also the issue of https on drupal.org which is harder to deploy if the images are on an http page.

g.d.o allows images because we're willing to take the risks I identified above in exchange for enhanced user experience. When we have found external images we ask people to move them to g.d.o, and we'll just do a giant mysql update to content to replace src="http: with src="https: when we switch to that.

I am not a huge fan of the htmlpurifier module. I think the administration screen for it is way more complex than it should be which makes it really easy to configure inappropriately. If someone made a simplified interface on it that included a few sane defaults and then validated it against http://drupal.org/node/1127876 I could definitely be swayed.

xjm’s picture

What about the inline module as eaton suggested? Then people would only be able to post images inline that they attach to the node or comment, and we save a lot of validation.

Edit: Just realized I don't know if this wouldn't work for adding issues inline in summaries, since those attachments are set on creation at the moment and I am not sure whether it would work for images attached to comments.

webchick’s picture

Inline inserts the img tag into the post. If you can do that, you can also manually write the img tag yourself. So it doesn't solve the problem.

However! We've come up with a plan that does: http://lullapad.com/l6Wt4uNOQY :)

sun is working on implementation now.

webchick’s picture

And I should really copy that plan here, for posterity.

(Minus the commentary)

---

In order to address both security (XSS) concerns, as well as support HTTPS on Drupal.org, in order to turn on <img> support for Filtered HTML, the following needs to happen:

We need a new input filter that can run after Filtered HTML, which restricts images to only being referenceable on the local server (relative path: /files/xxx.jpg).

Here's the spec for "Image restrict to domain" module:

Create a filter:

  • To run after HTML filter.
  • PCRE to find all src attributes of IMG/img tags.
  • Only if src is a relative path, and starts with "files/" (file_directory_path()), retain it.
  • Otherwise, delete the entire IMG tag.

Would be nice:

Debatable:

  • Use mime type validation to ensure that the content of <img src=""> is truly an image
  • Check whether the file exists in the local files directory.

A new, separate project on d.o should be created, since this sort of functionality could be broadly useful to lots of other Drupal sites. This module will require security team sign-off.

sun’s picture

sun’s picture

The module in the sandbox is ready for review now. Includes basic tests.

Note: It's KISS - we don't allow IMG tags to be used with Filtered HTML currently, so we don't have to care for existing IMG tag src URLs.

We can add a short filter tip to make users aware that only local images can be referenced.

But aside from that, I think it's reasonable to expect from people who'd like to use images in their posts to be HTML-savvy enough to understand and deal with this.

webchick’s picture

Rock! :D Thanks, sun!!

I have deployed and configured this module (thanks for the helpful readme!) to http://issue-summaries-drupal.redesign.devdrupal.org/ for testing.

(user/pass: drupal/drupal, then you can log in as bananas/bananas)

xjm’s picture

I started writing a post to the effect that it wasn't working, but looks like webchick was just cleaning up the configuration.
http://issue-summaries-drupal.redesign.devdrupal.org/node/1237693

Rock on!

webchick’s picture

Here is me testing some stuff:
http://issue-summaries-drupal.redesign.devdrupal.org/node/1237686#commen...

Text I tried was:


An image from Skitch:
<img src="https://img.skitch.com/20110531-jp7tms3e3cagsb34wb964me7s9.png" alt="Skitch!" />

An image from Drupal.org, absolute URL:
<img src="http://issue-summaries-drupal.redesign.devdrupal.org/files/issues/druplicon.png" alt="Absolute!" />

An image from Drupal.org, relative URL, leading slash:
<img src="/files/issues/druplicon.png" alt="Relative!" />

An image from Drupal.org, relative URL, no leading slash:
<img src="files/issues/druplicon.png" alt="Relative with a mistake!" />

An image from Groups.Drupal.org:
<img src="http://groups.drupal.org/files/bluebeach_logo.png" alt="Absolute!" />

Nefarious BS:
<img src="/files/<script>alert('ha!')</script>" alt="XSS!" />

So far, so good!

xjm’s picture

Only issue I've seen so far is that we might want to restrict image dimensions, or at least the inline rendering dimensions:
http://issue-summaries-drupal.redesign.devdrupal.org/node/1237693#commen...

sun’s picture

Note that some of this is already covered by tests: http://drupalcode.org/sandbox/sun/1274626.git/blob/refs/heads/6.x-1.x:/t...

Except for the last (which, again, is HTML filter's/filter_xss()'s responsibility), we can improve them. Just lemme know who needs to commit access...

scor’s picture

Great work sun!

note also that CSRFs a la /logout are prevented by requiring @src to start with /files. You can work around that limitation: #1274838: Prevent logout CSRF, rip host, allow any image inside this Drupal.

xjm’s picture

We'd want to add image attachments to handbook pages along with this change. Handbook pages work as expected other than that.:
http://issue-summaries-drupal.redesign.devdrupal.org/documentation/modul...

Filter tip proposal:

  • You may only use the <img> tag for images hosted on this site. Use a relative URL beginning with /.
    Example: <img src="/files/my_image.jpg" />
webchick’s picture

Good catch xjm @ #25.

Here's a one-liner patch to Drupal.org customizations module to get rid of that hiding of that fieldset.

webchick’s picture

Status: Active » Needs review
webchick’s picture

scor's CSRF bug has been addressed in the patch from chx at #1274838-5: Prevent logout CSRF, rip host, allow any image inside this Drupal, which is now deployed to the testing site.

Also updated http://issue-summaries-drupal.redesign.devdrupal.org/node/1237686#commen... with a few more test cases. Looking good so far! :D

webchick’s picture

So one other reeeeeeeeally annoying issue that's hitting beta testers atm is when uploading a file to the issue queue, it initially puts it at /files/XXX.jpg. You can even preview it and it looks great! However, once you hit "Submit", the path changes out from under you to /files/issues/XXX.jpg and your image breaks. :( This has absolutely nothing to do with this module, though. This is coming from #194449: Incorrect preview permalink when attaching files, which basically says "Yeah, core sucks." That was written back in Drupal 5 times though, so if someone wants to dig into D6 a bit and try their hand at fixing it that would be lovely! :) Should not be a blocker for this going live, though. People will eventually learn, just as we all had to. ;P

webchick’s picture

Title: Inline image displays without special permissions » Allow images to be posted to Drupal.org without any special permissions

This issue's changed quite a bit since its original incarnation, so changing the title as well.

Working on an issue summary.

webchick’s picture

Issue summary: View changes

Updated issue queue summary with current status, plus deployment steps.

webchick’s picture

Issue summary posted.

webchick’s picture

Issue summary: View changes

x

webchick’s picture

Issue summary: View changes

Adding a screenshot.

webchick’s picture

Issue summary: View changes

x

webchick’s picture

Issue summary: View changes

Splitting apart test instructions a bit.

webchick’s picture

Also, I spun off that patch in #26 to its own issue in the proper queue: #1274938: Un-hide the file attachments form since it might warrant further discussion on its own.

webchick’s picture

Issue summary: View changes

x

webchick’s picture

Issue summary: View changes

Adding reference to #1274938

arianek’s picture

Was just logging in to test this, and realized that we'll also need to address the already set input formats either with rolling this out or afterwards.

eg. http://issue-summaries-drupal.redesign.devdrupal.org/documentation/modul... because it already has images, it has the docs input format, and so can only be edited by docs admins even after this is rolled out.

We'd ideally want to somehow make all these existing posts with images accessible to regular users as well (and subsequently probably revoke all the unnecessary docs admin role assignments). My suggestion would be (unless there's something I'm overlooking):

1. Change all nodes with docs input format to this new local image input format.
2. Revoke docs admin role from all users.

And separately:

1. Review and update what docs admin role should actually have permissions for.
2. Add docs admin role back to any users that should have the new permission set.

Not ticketing these yet, as it'd be great to have input from y'all.

DID I MENTION YOU'RE MAKING MY DREAMS COME TRUE RIGHT NOW? THANK YOU!!! ;)

webchick’s picture

Ok, latest code from #1274838: Prevent logout CSRF, rip host, allow any image inside this Drupal is now deployed on the staging server. It now includes a nice fix for the "Haha, I break your site layout with my 9000px wide image of The Backstreet Boys." issue. ;)

dww’s picture

@arianek: From what I understand, some of the pages marked needing the "documentation" format are supposed to be "locked" pages that we don't want to behave as wikis (for whatever reason -- perhaps they're contentious, or they're the coding standards and we have a very specific process we go through if we're going to change them, etc). So, I'm slightly concerned about a blanket reset of all posts with that format. Then again, there might be so few pages that need to be locked that it'd be a lot easier to reset everything and handle those special-cases manually.

Otherwise, everything you're proposing makes perfect sense and I'd be happy to see tickets for those sub-tasks spawned off so they don't get lost in here.

Thanks,
-Derek

webchick’s picture

Started #1275424: Deal with documentation role and documentation input format to discuss the upgrade path, and it has some queries that help determine the scope of the mess on our hands. :P

webchick’s picture

The upgrade path discussion brought up an interesting point, and an interesting patch: http://drupal.org/node/1275424#comment-4972852

What's the general consensus on having the filter take any external images that it finds and download them locally, then switch the img src to point at them instead? This would hugely cut down on usability issues, IMO. But not sure if the idea of randomly downloading anything in an src="" value of an img tag is a good idea. :P

greggles’s picture

What's the general consensus on having the filter take any external images that it finds and download them locally, then switch the img src to point at them instead? This would hugely cut down on usability issues, IMO. But not sure if the idea of randomly downloading anything in an src="" value of an img tag is a good idea. :P

I think this is a good idea if someone can work on it. If the image has been on d.o for anything over a day then it's likely to be an image we want to keep around. If we find in this process or in the future that there are bad images we can just delete them.

I don't imagine the size of these images would be noticeable in any way compared to all the disk space we currently use.

killes@www.drop.org’s picture

I am not so much concerned about disk space but about possible abuses, ie spam etc.

We'd need to make sure that any images from deleted content get deleted when the content is deleted, we also need to make really, really sure that content uploaded in such a way is an actual image.

We may also want to explicitly whitelist certain domains for such uses.

pillarsdotnet’s picture

Linked from Screenshot sprint! group.

webchick’s picture

Ok, as an update...

1) sun's sandbox has been promoted to a real project at: http://drupal.org/project/filter_html_image_secure
2) This project's code is now deployed to the sandbox at: http://issue-summaries-drupal.redesign.devdrupal.org/ (drupal/drupal, then bananas/bananas)
3) There are some test cases at http://issue-summaries-drupal.redesign.devdrupal.org/node/1237686#commen... that show this module works well, even with some really sneaky stuff like img src="/files/../logout" and the like.
4) Some usability enhancements have been added:
- instead of stripping out the image entirely, it replaces it with a red "X" with a title that tells you what happened.
- if you enter a URL as http://drupal.org/files/something.jpg, it'll automatically convert it to a relative path.
- there is also a setting for max-width so if someone tries to upload a ginormous image like http://issue-summaries-drupal.redesign.devdrupal.org/node/1237686#commen... it will not break the page layout.

Please try and break it. :) And if you can't, let's get this sucker deployed!! WOOT!

webchick’s picture

Issue summary: View changes

x

chx’s picture

Re #39, the images will be attached to the comment so if the comment is deleted so will be the image. That's hardly an issue. For now, whitelist is not an issue because external images are not allowed, that's a separate issue.

jhodgdon’s picture

RE #41 - I went and tested it out...

It seems to work fine. Steps I used to add an image:
a) Choose a page http://issue-summaries-drupal.redesign.devdrupal.org/documentation/modul...
b) Click Edit
c) Upload an image in the attachments area.
d) After clicking Upload, copy the file URL that appears just below the image field.
e) Paste this into the SRC attribute of an IMG element in the text.
f) Save. Image appears. Phew!

I did notice that in the Edit field, the full URL of the image is still there, but after filtering, it is src="/files/xxx". Just to clarify that in point #4 above, this is only happening during input filtering.

I also added an image tag with an outside image, and it appeared as a red X as advertised.

So... The only thing I would suggest is some instructions added to the filter text and/or to the body tag or attachments field, explaining this procedure (not that anyone will read it, but still, at least then we can say RTFM).

Maybe something like this for the filter text:
Only local images are allowed. tags can only contain images uploaded to this site. Use the File Attachments section below to upload.

Other than that, I think it's fine. Thanks -- we've been needing this on drupal.org for ages!

jhodgdon’s picture

Uh oh. I just tried this with a comment and I got a big red X instead of an image:
http://issue-summaries-drupal.redesign.devdrupal.org/node/1237636#commen...

The problem is that the temporary URL that you get when uploading an image is not the final resting place for attached images on issue comments. So there is not a way to make this work for issue comment images?

dww’s picture

@jhodgdon: re: #44: see comment #29 and issue #194449: Incorrect preview permalink when attaching files

jhodgdon’s picture

Yeah. Well I don't think this is really deployable until that bug is fixed, is it? How would someone be able to post an IMG tag in a comment, unless they are psychic or something and can figure out where the file will land? Do we expect them to go back and edit the comment after the first post? That's not really OK.

pillarsdotnet’s picture

Do we expect them to go back and edit the comment after the first post?

Yes.

That's not really OK.

Why?

jhodgdon’s picture

You think it's OK??? I don't. It's totally lame, a WTF, or at least **a bit** awkward, and needs to be explained in the help/description text under the comment field?

silverwing’s picture

It's not okay. We need to remove barriers to contributing, not put up these roadblocks.

jhodgdon’s picture

Suggested help text for below the comment:

"If you want to add an image, you need to attach it, save your comment, then go back in and add the image tag in a second pass, because otherwise there is no way to know what the image URL will be. Yes, this is totally klunky and really lame, and really we should have fixed this ancient Drupal issue instead of adding this stupid help text that no one will read."

:)

pillarsdotnet’s picture

Status: Needs review » Postponed

Well, then I guess this should be marked "postponed" as it is blocked by #194449: Incorrect preview permalink when attaching files

jhodgdon’s picture

Maybe. You know, I've had no trouble like this with the Insert module, which I've used on a number of D6 sites....

Though I've only used it with CCK image fields and WYSIWYG editors... but it seems to insert the image tag and it ends up with the right path somehow (haven't looked at the code and maybe CCK doesn't have the same problem as Upload). But it might be worth testing out? It's also a bit more user-friendly, considering that maybe not everyone knows to copy/paste the image URL that magically appears if you happen to click the Upload button...

Steven Jones’s picture

@jhodgdon I'm fairly certain that insert module only interacts with CCK file widgets, not core upload module ones like the one's that project module is wanting to deal with, and that we use on d.o, so I'm not sure it's the answer here.

jhodgdon’s picture

RE #52 - sigh, you are right, according to the Insert module project page anyway.
http://drupal.org/project/insert
:(

chx’s picture

Status: Postponed » Needs review

I have deployed a solution on our sandbox.

Edit:#194449-10: Incorrect preview permalink when attaching files . Considering the involved parties, it's not even particularly ugly.

dozymoe’s picture

What's the verdict on images with data-uri?

<img src="data:image/gif;base64,R0lGODlhEAAOALMAAOazToeHh0tLS/7LZv/0jvb29t/f3//Ub/
/ge8WSLf/rhf/3kdbW1mxsbP//mf///yH5BAAAAAAALAAAAAAQAA4AAARe8L1Ekyky67QZ1hLnjM5UUde0ECwLJoExKcpp
V0aCcGCmTIHEIUEqjgaORCMxIC6e0CcguWw6aFjsVMkkIr7g77ZKPJjPZqIyd7sJAgVGoEGv2xsBxqNgYPj/gAwXEQA7" 
width="16" height="14" alt="embedded folder icon">

The resulting image is a folder icon (like the one from Windows 95).

Feature request: OnionHeads can be uploaded to my d.o account, and href-ed everywhere else.

jhodgdon’s picture

RE #56 - You might be the only person who would know how to do an image tag with encoded data enclosed. :) I don't think it's a huge burden to require that the little gif be uploaded and attached.

RE #56 - what are OnionHeads? I have no idea if this is a joke or a serious feature request... ???

RE #55 - it looks like #194449: Incorrect preview permalink when attaching files has been postponed in favor of #1282836: Stop trying to move issue attachments to a subdirectory of files, so I haven't tested the proposed fix.

dww’s picture

Re: #55/#57: Right -- the proposed fix only works for comments, not the original post. This whole "feature" of keeping the issue attachments in a subdir of files is a broken mess, and I'm in favor of stopping that instead of piling hack upon hack to continue to get it "working". As soon as we get an answer from nnewton at #1282836 we can quickly move this forward since we can just set a config knob that should disable the weirdness and then deploy the filter from sun and chx without any known UI problems.

jhodgdon’s picture

Sounds like a good plan!

jhodgdon’s picture

Status: Needs review » Postponed
dww’s picture

Status: Postponed » Needs review

No longer blocked. There's a setting at http://drupal.org/admin/project/project-issue-settings we can disable whenever we want on drupal.org and project_issue will stop trying to move stuff into a subdir. We've already got the blessings of other key members of the infra team to do so (nnewton, drumm, greggles). I might leave the code in project_issue that tries to support this, or we might rip it out as part of #1282836: Stop trying to move issue attachments to a subdirectory of files, but there's no reason to delay this on the outcome of that decision or pushing a new version of project_issue onto d.o. #194449: Incorrect preview permalink when attaching files is now "won't fix".

So, what else needs to happen here?

A) Review/test/commit for #1274938: Un-hide the file attachments form

B) Security Team review/audit of http://drupal.org/project/filter_html_image_secure

C) An official release of filter_html_image_secure to deploy from would be nice.

D) Actual d.o deployment of the new code, configure the filter in the default Filtered HTML input format, disable issue attachment moving at http://drupal.org/admin/project/project-issue-settings

Am I missing anything else?

Setting back to 'needs review' for B. Maybe it's RTBC, I've lost track (dealing with too many things right now).

Thanks,
-Derek

jhodgdon’s picture

Can we get all of this done on the demo site so we can test it out?

webchick’s picture

Ok, I've now changed that setting on the issue summaries sandbox, so you can test it out. Preview works great. I also tried editing an old comment that referenced a /files/issues/XXX.jpg and it came back fine.

webchick’s picture

Issue summary: View changes

Updating the issue summary.

webchick’s picture

Issue summary: View changes

Updating deployment steps about admin/project/project-issue-settings

xjm’s picture

I tested images in issue comments, issue summaries, and handbook pages using both new images and previously uploaded ones. Seems to work fine! The working preview is nice as well. It's definitely much easier to understand what to do with the file attachment listing the correct file path before the post is submitted.

webchick’s picture

One blocker at the moment is what to do about REALLY BIG IMAGES that break the site layout.

http://drupal.org/project/filter_html_image_secure for awhile had a feature where it would cap the max width of images by re-writing the img tag with height and width attributes. This was removed by sun in #1281194-6: Roll a 1.0 release of this module, on the grounds that a filter doesn't have context of what theme is running in the current request / how wide the content column is / etc. and therefore this belongs in the theme instead.

I then opened #1289434: Restrict user-uploaded images to a maximum width in the Bluecheese theme to handle max-width with CSS instead, and that got won't fixed by drumm, on the grounds that this should be done with server-side code, not client-side code, which is a hack.

I don't think either are wrong, though I also think not allowing untrusted users to bust the site layout with big images is a really nice feature. I would hate to give that up, but I also don't want to see this incredibly important issue blocked on arguing over whose responsibility it is to do this, and I really don't want to block it on a large-scale data migration effort of comment_upload to filefield + some kind of imagecache hack on D6. Maybe as a second iteration, but not now.

So I'm wondering if we can get away with deploying without worrying about images that break the site layout, since there are enough of us floating around who could fix that ourselves by editing a user's comment and adding height/width attributes.

Thoughts?

webchick’s picture

Issue summary: View changes

And also the help text patch.

webchick’s picture

Issue summary: View changes

Updating remaining tasks + deployment steps.

webchick’s picture

Also, I have updated the remaining tasks and deployment steps in the summary as best as I know them.

webchick’s picture

Issue summary: View changes

more.

chx’s picture

Sure there are enough of us. I am not too worried also users can edit their own so they can fix after applying a clue-by-four.

pillarsdotnet’s picture

Title: Allow images to be posted to Drupal.org without any special permissions » Allow inline images to be posted to Drupal.org project pages, docs pages, and comments without any special permissions

Of course, any user may already upload image files; this is about posting content with inline images.

skottler’s picture

Subscribe.

webchick’s picture

Ok, back in #1289434: Restrict user-uploaded images to a maximum width Neil confirms that dealing with really big images is not a blocker. That reduces the list of blockers to a security team audit and a 1.0 release.

webchick’s picture

Issue summary: View changes

Escaped Only local images are allowed.

webchick’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +needs drupal.org deployment

1.0 release: http://drupal.org/node/1292956 BAM! :D grendzy handled the security review.

Deployment steps at http://drupal.org/node/528682#summary-deployment

This should be good to go! Marking RTBC and tagging.

webchick’s picture

Issue summary: View changes

updating deployment steps.

webchick’s picture

Issue summary: View changes

Updating todos. YEAH!

webchick’s picture

Issue summary: View changes

link

dww’s picture

Assigned: Unassigned » dww

I'm starting the deployment now... stay tuned.

dww’s picture

Assigned: dww » Unassigned
Status: Reviewed & tested by the community » Fixed

Okay, we're now at step #8 in the deployment plan. ;)

"Sing, dance, and rejoice! :D"

Yay everyone!

Cheers,
-Derek

webchick’s picture

FileSize
59.48 KB

Hooray!! with smileys

YES!! Thank you SO MUCH!!

dww’s picture

Issue tags: -needs drupal.org deployment

Hah, sweet. ;)

Forgot to untag this in my previous comment...

DjebbZ’s picture

FileSize
106.03 KB

Excuse me, just testing so I understand how it works...


(I love this website :)

Hey hey !
DRUPAL TRANSFORMATION !

jhodgdon’s picture

YEAH!!!!!

Now we just need to figure out what to do about the Docs pages and Docs admins... but that is a separate issue (or at least we need to make it be a separate issue). THANK YOU sun, webchick, and dww for making this happen!

webchick’s picture

jhodgdon’s picture

Oh, we had a prior issue #995354: Docs format and Docs role - figure out what to do

One needs to be marked as a dup, I guess I'll do that one.

klonos’s picture

Just to understand how this works... Is it a two-step thing: first upload (image)file - then use the img tag?

webchick’s picture

Yep, that's correct. The img tag will only work for images on drupal.org, so they have to be uploaded first.

klonos’s picture

Thanx for the prompt reply Angie. I hope this becomes a one-step procedure in the future (is there any issue for that someplace btw?).

pillarsdotnet’s picture

@klonos:

Is it a two-step thing: first upload (image)file - then use the img tag?

They can be uploaded and inlined in the same post, but it takes more steps than two.

  1. Attach a file.
  2. Click on Preview
  3. Copy the uploaded file path into an inline image tag.
  4. Click on Preview again.
  5. If everything looks okay, click on Save.

I hope this becomes a one-step procedure in the future (is there any issue for that someplace btw?).

There appears to be strong community resistance against adding a WYSIWYG content editor to drupal.org infrastructure. Even Bueditor, which is not WYSIWYG, is having trouble getting accepted.

But by all means, please open an issue to integrate one of the methods listed at Working with images (Drupal 6).

Reference The Future of Image in Drupal 7 if you want your suggestion to survive a drupal.org D6 to D7 upgrade.

sun’s picture

The Preview issue is no longer an issue, since we also removed the custom issue attachment file path via #1282836: Stop trying to move issue attachments to a subdirectory of files

Dreditor has already been adjusted to account for these changes via #1289542: Fix image embed code to handle new drupal.org issue attachment paths -- thus, you get a 99% reliable "Embed" button with it.

(99%, because I think that image file paths are still not shown correctly, if you happen to upload a file name that already exists on d.o, say, screenshot.png, and so the resulting attachment file name will be screenshot_34.png, whereas the upload attachment widget suggests screenshot.png as filename, because the file was only uploaded to a temporary directory on the server and not ultimately saved into the files directory yet. This special case will probably be only obsolete after upgrading d.o to D7.)

jhodgdon’s picture

Here's a shorter procedure:
1) Attach a file (browse, then click Attach).
2) Copy the URL that appears below the attach field, and use that in the src attribute of your img tag.

pillarsdotnet’s picture

3) Preview to make sure that it works.
4) Save.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

fixed link to the filter project page in the deployment steps

itapplication’s picture

Status: Closed (fixed) » Needs work
FileSize
102.34 KB
95.24 KB

Hi,

On project page, image appears twice if we insert attached images through img tag. We need check box to toggle display/list attached image as we can do it here(on create/post issue page). Please see attached screenshots.
Project page of Theme

Project edit page

greggles’s picture

Status: Needs work » Closed (fixed)

A workaround is to upload the image in some other way, like to an issue in your project, and then embed the image from there.

This issue is closed/fixed for the functionality that was intended. If you'd like to add an additional feature, please open a new issue.

dww’s picture

What greggles said, and/or don't manually include an img tag for the image that's directly attached to the project page...

itapplication’s picture

@dww as greggles suggest I create new issue for this at here https://drupal.org/node/2040091

itapplication’s picture

Issue summary: View changes

Escaping Only local images are allowed. tag.