$path is relative to DRUPAL_ROOT.

Adding base_path() [a URL basepath] in between DRUPAL_ROOT and $path
produces an incorrect absolute path on the system level.

DRUPAL_ROOT, by definition, is "getcwd()" on bootstrap.
see http://api.drupal.org/api/drupal/index.php/constant/DRUPAL_ROOT

== Example ==
- Drupal site is installed as follows:
/var/www/drupal7site/public/
/var/www/drupal7site/public/logo.png
with this VirtualHost baseurl... http://example.com/drupal7/

- This image tag is placed in an email:
<img src="/drupal7/logo.png" />

DRUPAL_ROOT = /var/www/drupal7site/public
base_path() = /drupal7/
$path = logo.png

Bad absolute path produced:
DRUPAL_ROOT . base_path() . $path
... becomes => /var/www/drupal7site/public/drupal7/logo.png

Good absolute path produced:
DRUPAL_ROOT . DIRECTORY_SEPERATOR . $path
... becomes => /var/www/drupal7site/public/logo.png

== Tested using ==
Drupal 7.14, 7.19

== urldecode also needed ==
Without a urldecode, any number of generated-URLs ready for browser consumption may result in file-not-found errors. Typically, having the following fallback when attempting image embedding is sufficient:

    - <img src="/%C3%89l%C3%A9phant" />      BECOMES "/abspath/%C3%89l%C3%A9phant" AND "/abspath/Éléphant"
    - <img src="/Éléphant%20X" />            BECOMES "/abspath/Éléphant%20X" AND "/abspath/Éléphant X"
    - <img src="/%C3%89l%C3%A9phant%20X" />  BECOMES "/abspath/%C3%89l%C3%A9phant%20X" AND "/abspath/Éléphant X"
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

starlocke’s picture

Here's a patch in attachment, and on github (https://github.com/starlocke/drupal-mailmime/) as commit cbabd4615c.

I also did a merge for 8.x-2.x and 7.x-2.x branches in my copy, if you want to just do a "git pull" with a fast-forward for those branches.

Copy/paste convenience:
git apply -v mailmime-url_to_realpath_fix-1935610-1.patch

salvis’s picture

Version: 7.x-1.2 » 7.x-2.x-dev

Thank you for the analysis and patch.

The 1.x branches are obsolete. Let's see whether this applies to 7.x-2.x.

salvis’s picture

Status: Needs review » Active
salvis’s picture

Status: Active » Needs review
salvis’s picture

Seems like you have to re-upload for the testbot to catch on.

starlocke’s picture

Status: Needs review » Active
starlocke’s picture

Re-uploading the patch... O_o

starlocke’s picture

Status: Active » Needs review
salvis’s picture

I'm puzzled and disappointed that the testbot doesn't catch on. Sorry about the trouble...

Can someone confirm this, please?

I wonder... You're mapping http://example.com/drupal7/ to http://example.com/ in Apache. What if you didn't do this?

Is there really no configuration where base_path() (the current code) is needed?

starlocke’s picture

base_path() has to do with URLs on the web. Since we're trying to get absolute filesystem paths, base_path() has absolutely no place in Mail MIME, ever. This was already illustrated in the original description.

starlocke’s picture

Version: 7.x-2.x-dev » 8.x-2.x-dev
FileSize
1.15 KB

A replacement patch that also covers the urldecode problem.

starlocke’s picture

In our code review, we noted that we should probably drop the first attempt, and keep only urldecode(); but, what about cases like:

<img src="/pokémon+red/team%20rocket.jpg" />

Should it translate to:

"/var/www/pokémon+red/team rocket.jpg" or "/var/www/pokémon red/team rocket.jpg" ?

What if my target is really "/var/www/pokémon+red/team rocket.jpg" ?
What if my target is really "/var/www/pokémon red/team rocket.jpg" ?

According to http://stackoverflow.com/questions/1005676/urls-and-plus-signs, plus signs are literal, so... Ultimately, we should be using rawurldecode(); because the QUERY part is naturally not part of a filename in our present context.

Now, what if your server actually uses a proxy for serving images, like:

<img src="/silph+co.php?mugshot=team+rocket.jpg" />

I would tend to think that Mail MIME won't embed these kinds of images, today.

If we were to solve that by having Mail MIME download images to some cache and encoding based on those, then we might be getting closer to the desired generic effect of "embed images".

Requesting for comments.

starlocke’s picture

Here's a simplified patch that rolls up DIRECTORY_SEPERATOR and rawurldecode().

salvis’s picture

Version: 8.x-2.x-dev » 7.x-2.x-dev

(Maintaining the D8 version is too ambitious for what I can invest here.)

I would really like to create new releases for Mail MIME, but we have three patches in the queue that should probably be committed:

#1813542: Multipart/Alternative with line break (introduced by PEAR) breaks drupal core and results in unreadable message (D6 — what about D7???)
#1929678: Mail MIME inline images using image_style's do not work with Drupal 7.20 (D7, maybe D6, too)
#1935610: url_to_realpath fix (D7, maybe D6, too)

I'm not using Mail MIME myself, and I need someone to review these patches. Maybe the three authors could trade reviews? Or someone else who is actively using Mail MIME?

fenstrat’s picture

Status: Needs review » Reviewed & tested by the community

Tested #13.

The DIRECTORY_SEPARATOR change looks and works fine. I can't really comment on the rawurldecode changes but I can verify that they do not break existing image embedding. Tentatively marking as RTBC.

salvis’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x-2.x-dev. Thanks!

Does this need to be backported to D6?

salvis’s picture

Version: 7.x-2.x-dev » 6.x-2.x-dev
Status: Fixed » Patch (to be ported)

Let's put it this way:

I'd like to create a new releases for Mail MIME for D6, too, but we have two D7 patches in the queue which may or may not need to be back-ported to D6:

#1929678: Mail MIME inline images using image_style's do not work with Drupal 7.20
#1935610: url_to_realpath fix (this one)

I'm not using Mail MIME myself, and I need people to either port and review, or to tell me that the patches are not needed for D6.

salvis’s picture

Issue summary: View changes

Added secondary issue requiring urldecode to fix; depends on having the right abspath.

salvis’s picture

Version: 6.x-2.x-dev » 7.x-1.x-dev
Status: Patch (to be ported) » Closed (fixed)

Porting to D6 is not going to happen anymore, so we'll let this RIP.