User tests: Successful: Unsuccessful:
Pull Request for Issue #24132 .
check if we are on preview mode
click preview on unpublished article from admin
404
preview works
Status | New | ⇒ | Pending |
Category | ⇒ | Front End com_content |
open to impovements of course
Im confused why you ever thought this would work? as the preview modal doesnt have code to even add preview=1
to the url?
This needs some kind of cryptographic hashes as tokens as a preview param, or a better way is to check Admin Sessions from the frontend just before going to the 404 exception.
funny phil I tested this PR and found out the same ;-) it's a complete security nightmare and by the way the issue is not an issue if you have shared sessions. Then you should have to see the article in frontend even if it's unpublished...
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-09-06 18:35:34 |
Closed_By | ⇒ | HLeithner | |
Labels |
Added:
?
|
@alikon well that quoted method still doesnt add preview=1
// Add a preview button.
$bar->appendButton('Popup', $icon, 'Preview', $url . '&task=preview', 640, 480, $bodyHeight, $modalWidth);
Im sorry @HLeithner but this is a genuine issue that needs resolving (there are open issues from me about it) conceptually if you are logged into the admin console and there is a preview button it should be useable WITHOUT SHARED SESSIONS because shared sessions IS NOT THE DEFAULT for Joomla.
if you are logged into the admin console and there is a preview button it should be useable WITHOUT SHARED SESSIONS because shared sessions IS NOT THE DEFAULT for Joomla.
So basically what you are saying @HLeithner - is that for "some Joomla admin features" to work, you first need to be logged into the frontend as well...
So that begs the question, why we have split sessions at all... would you force me to be logged into the frontend to change a users password in admin? or to install an extension? No of course not, and there is no reason I should need to be logged into a different application (client id) in order to preview the content Im creating in admin console.
If the preview button shows - it should be useable, without making someone think.
Maybe we just show the preview button if the user has a valid frontend (ie. shared) session? and hide the button if they dont? thats better than showing a button and having it broken (404) when pressed?
if you have a shared session or loggedin in frontend the preview works even for an unpublished article.
and if you have not ?
So maybe we should disable the preview button on not shared session, adding a magic token to the request is possible but needs more work then just add a parameter.
So maybe we should disable the preview button on not shared session,
Thats one solution, but it hides an otherwise nice feature, a new feature of Joomla 4.
adding a magic token to the request is possible but needs more work then just add a parameter.
Agreed.
What do other CMS's and applications do for preview modes? might be worth checking out, I think WP just uses shared session, so thats how they get around it
Whats the reason we dont use shared session by default?
@PhilETaylor I don't have a strong opinion if it should be disabled or have a secure way to view it in the frontend without being logged in. If we add a secured parameter we should do this in a generic way so it works for all components out of the box (or with little effort)
What do other CMS's and applications do for preview modes? might be worth checking out, I think WP just uses shared session, so thats how they get around it
I think that's also how it's done in typo3 and drupal but don't nail me.
Whats the reason we dont use shared session by default?
I would expect for legacy reasons? I see no problem activating this per default for new installations. (if it's a problem it shouldn't exists)
. If we add a secured parameter we should do this in a generic way so it works for all components out of the box (or with little effort)
Well thats what the preview button is currently trying to do by adding task=preview
... that could become secureToken=tokenhere...
to make it generic.
/**
* Writes a preview button for a given option (opens a popup window).
*
* @param string $url The name of the popup file (excluding the file extension)
* @param bool $updateEditors Unused
* @param string $icon The image to display.
* @param integer $bodyHeight The body height of the preview popup
* @param integer $modalWidth The modal width of the preview popup
*
* @return void
*
* @since 1.5
*/
public static function preview($url = '', $updateEditors = false, $icon = 'preview', $bodyHeight = null, $modalWidth = null)
{
$bar = Toolbar::getInstance('toolbar');
// Add a preview button.
$bar->appendButton('Popup', $icon, 'Preview', $url . '&task=preview', 640, 480, $bodyHeight, $modalWidth);
}
I see no problem activating this per default for new installations. (if it's a problem it shouldn't exists)
Well I guess thats a decision for someone with a higher pay grade than me :)
just checking the documentation for the edit article page - https://docs.joomla.org/Help4.x:Articles:_Edit/en and there is not really help there:
"Preview. Opens a modal dialog showing a site view of this article."
I guess as a "quick fix" that could be improved to say that shared sessions needs to be enabled for that to work.
I would suggest to use the shared session (because it's already proofed to work) and activate it in new installations starting with 4.1.
I will bring this into PD tomorrow.
And hide the button if shared sessions are disabled. I think we don't have a way to detect if the user is logged in in frontend right?
Im thinking this is probably the best solution. Shared Sessions that is.
A secure token would have to be time based, because it could not be stored in a session for checking - cause - eh - different sessions :)
We could also check the popup url for the preview var, and if 1) We are just about to go to the 404 exception, AND 2) We have a preview var in the url AND 3) shared sessions is NOT enabled, THEN we show a helpful "In order to preview content you need shared sessions enabled (or logged into the frontend)...
This is then backward compatible and Joomla 4.0.x sites can have that fix while waiting for the 4.1 shared session as default (because a 4.0.1 site upgraded to 4.1.0 will not have shared session... so yeah, this is probably what needs to be done as well.)
The method that IS used is in libraries/src/Toolbar/CoreButtonsTrait.php
/**
* Writes a preview button for a given option (opens a popup window).
*
* @param string $url The name of the popup file (excluding the file extension)
* @param string $text The text of button.
* @param bool $newWindow Whether to open the preview in _blank or just a modal
*
* @return PopupButton|LinkButton
*
* @since 4.0.0
*/
public function preview(string $url, string $text = 'JGLOBAL_PREVIEW', $newWindow = false)
{
if ($newWindow === true)
{
$button = $this->linkButton('link', $text)
->url($url)
->attributes(['target' => '_blank'])
->icon('icon-eye');
}
else
{
$button = $this->popupButton('preview', $text)
->url($url)
->iframeWidth(640)
->iframeHeight(480)
->icon('icon-eye');
}
return $button;
}
and thats called in the HtmlView.php as
$url = Route::link(
'site',
RouteHelper::getArticleRoute($this->item->id . ':' . $this->item->alias, $this->item->catid, $this->item->language),
true
);
$toolbar->preview($url, 'JGLOBAL_PREVIEW')
->bodyHeight(80)
->modalWidth(90);
I'm not sure but I think task=preview was used in earlier versions of joomla? Adding a preview=1 parameter and show a proper error message should be possible, at least we should not check if the article exists at this time, but you say to simple write it to the 404 page so it should be fine.
I tried to edit the wiki page but failed :-(
at least we should not check if the article exists at this time, but you say to simple write it to the 404 page so it should be fine.
Correct, as to do it any other way could introduce a security issue where a hacker could enumerate which article ids exist based on the response.
My suggestion, doing it just before throwing the 404 exception around line 233 - so hacker with preview=1 would see "you need to login to preview, or use shared sessions help text...." (And prefer a HTTP header of 404 so that google doesnt index that url) for ALL ids he tries to access - regardless if the article exists in the db or not.
did some of you ever tryied that silly code . and maybe even on debug ? or it's asking too much ?
Im not sure what point you are trying to make but, in summary:
task=preview
and not preview=INT
like your PR assumes.did some of you ever tryied that silly code
Yes, I set all my test articles to unpublished - logged out of the admin console and the frontend, and appended &preview=1
to all urls
http://127.0.0.1:4444/index.php?option=com_content&view=article&id=1&Itemid=102&preview=1
http://127.0.0.1:4444/index.php?option=com_content&view=article&id=2&Itemid=102&preview=1
http://127.0.0.1:4444/index.php?option=com_content&view=article&id=3&Itemid=102&preview=1
http://127.0.0.1:4444/index.php?option=com_content&view=article&id=4&Itemid=102&preview=1
and I could view all my unpublished content - like a hacker would. Therefore you were introducing a security hole into Joomla. Basically this PR would allow anyone unauthenticated to view any content that was not published, just by doing like I did above, and incrementing and iterating through all &id=
numbers...
I saw the mail and checked the code and see that it doesn't looked good, so I tested the patch on one of my dev instances to be sure. So yes I tested it and was able to view the unpublished article without being logged in (frontend and backend).
my point was only that there is some kind of "preview" info
...not more nor less ...
looking forward to see a good 1 pr
I tried to edit the wiki page but failed :-(
Why you failed, find the chunk? If so i can do it and change the text if i get the new text.
Welcome back?
Why you failed, find the chunk? If so i can do it and change the text if i get the new text.
I already asked Sandra and she explained me how to edit this subset safesubst, hopefully I did it right... but thanks
@PhilETaylor the feed back from the PD was to not activate shared sessions as default. So at the first set we should add a proper error message and help to user to understand why he or she can't preview the article and have to login to the frontend or have to activate shared sessions.
In the mid/long term a drafting system should be introduced into joomla (maybe as part of the workflow).
the feed back from the PD was to not activate shared sessions as default.
Was there a documentable reason for this decision?
So at the first set we should add a proper error message and help to user to understand why he or she can't preview the article and have to login to the frontend or have to activate shared sessions.
Cool.
In the mid/long term a drafting system should be introduced into joomla (maybe as part of the workflow).
its bad, but this kind of draft is proposed.... #35385
It will be in the notes but no motion has been done.
I have seen the pr and talked to the students, sadly at the end of the course. I would have something different in mind but not ready to write about it.
Would you like and have the time to create the preview information 404 pr?
Doesnt work for me.
However what this PR does do is introduce a huge security issue which allows ANY UNPUBLISHED ARTICLE to be viewed, without login to either the frontend or admin just by appending
?preview=1
to any url (and because you can guess the url of content items because you can increment id numbers, you can now view all unpublished content with no ACL checks at all...So yeah... This PR in its current state should be rejected :)