Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

placeholder images not accessible in joomla #406

Conversation

deepak-srivastava
Copy link
Collaborator

@deepak-srivastava deepak-srivastava commented Oct 30, 2020

Since placeholder images were permissioned, placeholder images have gone missing for Joomla installs.

This is mainly because in Joomla backend backend url (example.org/administrator) is separate from frontend url (example.org/index.php) and civicrm/mosaico/img url is frontend. See following screenshot:

image

The issue was also discussed here.

UPDATE:

@mattwire
Copy link
Collaborator

mattwire commented Nov 1, 2020

I'm not able to suggest an alternative but I do have concerns with this as I do see regular attempts to abuse this endpoint on a couple of (wordpress) client sites. I guess the issue is that the placeholder URL needs to be frontend when viewing the email but not when composing? Could we do some postprocessing on the template instead to work around this?

@aydun
Copy link
Contributor

aydun commented Nov 2, 2020

Back here @deepak-srivastava said:

Now civicrm/mosaico/img is a <is_public> url and therefore generates a frontend url like:
https://example.org/index.php?option=com_civicrm&task=civicrm/mosaico/img&method=placeholder&params=166%2C130

Since the placeholder images aren't intended to be in public mails and cause some DoS concerns, how about creating a civicrm/mosaico/admin-img for placeholder images with permissions and leaving truly public images in the current location?

@deepak-srivastava
Copy link
Collaborator Author

@aydun thanks. Initially i wasn't sure how difficult it would be because of interference with mosaico library. But turns out it's not that difficult, as library already has a separate section for placeholder images - civicrm/mosaico#6.

@mattwire
PR has been updated to use a new placeholder image url with it's own permission and exposed as backend url.
Also see dependent PR civicrm/mosaico#6.

@eileenmcnaughton
Copy link
Collaborator

Seems OK to me ... @aydun @mattwire ?

if (!CRM_Core_Permission::check([['access CiviMail', 'create mailings', 'edit message templates']])) {
CRM_Utils_System::permissionDenied();
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check that the xml access_arguments mean we don't need the check here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't require the check in php for sure.
I did test it for Joomla which seemed to work due to backend url login prompt. And I just tried it for drupal to confirm, and it wasn't working as expected.
Unlike other urls in mosaico menu xml file, it seems to require a access_callback, may be because in the tree mosaico/img url is public, and is somehow being derived (as public).
Have explicitly set the access_callback and tested. Seems to work now. Don't need the check in php.
Thanks for the headsup.

@mattwire
Copy link
Collaborator

mattwire commented Nov 6, 2020

This seems ok yes, just one comment for @deepak-srivastava to confirm

@deepak-srivastava
Copy link
Collaborator Author

@mattwire do you need anything else?

@mattwire
Copy link
Collaborator

@deepak-srivastava this seems good. Could just do with someone confirming they've run and tested this.

@mattwire mattwire merged commit 5324999 into veda-consulting-company:2.x Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants