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

[5.1] Allow PDF embeding again #43716

Merged
merged 5 commits into from
Jul 2, 2024

Conversation

bembelimen
Copy link
Contributor

@bembelimen bembelimen commented Jun 27, 2024

Pull Request for Issue #43407 & #43365 .

Summary of Changes

The security hardening adds an empty "sandbox" attribute to every iframe (read more). This prevents PDFs insert by the media manager loaded (it needs more fine granulated sandbox values)

With TinyMCE 7 you can whitelist certain domains, which will overcome this problem, but till then, this filter is super strict and not usable.

Testing Instructions

Use the media manager to embed a PDF. Save the article and open article in frontpage.

Actual result BEFORE applying this Pull Request

  • PDF is blocked

Expected result AFTER applying this Pull Request

  • Go to the TinyMCE configuration and disable the following option:
    grafik

  • PDF is visible

@drmenzelit
Copy link
Contributor

I have tested this item ✅ successfully on 46a404f

Thank you


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43716.

@Fedik Fedik added the bug label Jun 28, 2024
@SniperSister
Copy link
Contributor

Revert broken security fix

It's not broken and it's not a "bug". It's exactly doing what it's supposed to do: it limits the possible actions that an iframe can perform to protect the user from unexpected script execution.

So, the question actually is: does the CMS want to enforce the strictest possible security configuration for the editor, at least by default? Or is the comfort in the content creation process more important?

That's a question that's probably worth a vote in the production department meeting.

@drmenzelit
Copy link
Contributor

I'm for security, of course, but it should at least be possible to embed a local PDF ...
#43407 (comment)

@RickR2H
Copy link
Member

RickR2H commented Jun 28, 2024

I have tested this item ✅ successfully on 46a404f


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43716.

@RickR2H
Copy link
Member

RickR2H commented Jun 28, 2024

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43716.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 28, 2024
@bembelimen bembelimen marked this pull request as draft June 28, 2024 08:57
@Fedik
Copy link
Member

Fedik commented Jun 28, 2024

TBH, I have no idea why it is called security,
Crossorigin is not easy to bypass without extra effort, by just embed iframe,
And all local embendings, well, they will be need to be enabled in one or in another way.

We just making life for average User more complicated. They can just switch to codemirror, or none editor, and all this "theorethical security" will disapears.

@SniperSister
Copy link
Contributor

We are not talking about a CORS but about user experience and perception. Imagine an editor copy&pasting an iframe tag to an external site into the editor window of his companies intranet. On paste, the iframe will be loaded automatically - and without sandboxing, that iframe could i.e. trigger a file download. The user is now under the impression that this download is triggered by a trusted source, which is the Joomla administrator interface of his intranet - whereas the download is actually triggered by the untrusted iframe site.

I'm not necessarily saying, that this is an important threat scenario for the average user, I just want to point out that the issue is not related to crossorigin or samesite policies.

@Fedik
Copy link
Member

Fedik commented Jun 28, 2024

We can make a TinyMCE plugin, that shows an alert for potentialy dangerous iframe (and let User decide what to do), when User insert something in the editor. instead of totaly blocking everything.

@RickR2H
Copy link
Member

RickR2H commented Jun 28, 2024

Maybe its an option to extend the TinyMCE plugin with the basic configuration for the sandbox parameters so a domain could be added to allow the PDF iframe?

@richard67 richard67 added this to the Joomla! 5.1.2 milestone Jun 28, 2024
@Quy Quy removed RTC This Pull Request is Ready To Commit bug PR-5.1-dev labels Jun 29, 2024
@joomla-cms-bot joomla-cms-bot removed this from the Joomla! 5.1.2 milestone Jun 29, 2024
@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Jun 29, 2024
@bembelimen bembelimen marked this pull request as ready for review June 29, 2024 09:55
@bembelimen
Copy link
Contributor Author

Now with parameter. Ready to test again :)

@bembelimen bembelimen changed the title [5.1] Revert broken security fix preventing PDF embeding [5.1] Allow PDF embeding again Jun 29, 2024
@bembelimen bembelimen added this to the Joomla! 5.1.2 milestone Jun 29, 2024
@RickR2H
Copy link
Member

RickR2H commented Jun 29, 2024

I have tested this item ✅ successfully on 12ac335


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43716.

1 similar comment
@SniperSister
Copy link
Contributor

I have tested this item ✅ successfully on 12ac335


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43716.

@joomla-cms-bot joomla-cms-bot removed this from the Joomla! 5.1.2 milestone Jul 1, 2024
@chmst
Copy link
Contributor

chmst commented Jul 1, 2024

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43716.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 1, 2024
@chmst chmst added this to the Joomla! 5.1.2 milestone Jul 1, 2024
@LadySolveig LadySolveig merged commit 801f178 into joomla:5.1-dev Jul 2, 2024
3 checks passed
@LadySolveig
Copy link
Contributor

Thank you @bembelimen and also for testing and review @drmenzelit @SniperSister @RickR2H @Fedik @brianteeman

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet