? Pending

User tests: Successful: Unsuccessful:

avatar alikon
alikon
6 Sep 2021

Pull Request for Issue #24132 .

Summary of Changes

check if we are on preview mode

Testing Instructions

click preview on unpublished article from admin

Actual result BEFORE applying this Pull Request

404

Expected result AFTER applying this Pull Request

preview works

avatar alikon alikon - open - 6 Sep 2021
avatar alikon alikon - change - 6 Sep 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 6 Sep 2021
Category Front End com_content
avatar PhilETaylor
PhilETaylor - comment - 6 Sep 2021

Doesnt work for me.

  1. Ensure logged out of frontend
  2. Login to admin
  3. Ensure logged out of frontend (no mixed use session)
  4. Edit an article and unpublished it. Save.
  5. Click preview
  6. Still see a 404 error on my iframe url http://127.0.0.1:4444/index.php/asdf where asdf is my article title/slug.

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 :)

avatar alikon
alikon - comment - 6 Sep 2021

open to impovements of course

avatar PhilETaylor
PhilETaylor - comment - 6 Sep 2021

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.

avatar HLeithner
HLeithner - comment - 6 Sep 2021

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...

avatar alikon
alikon - comment - 6 Sep 2021

public static function preview($url = '', $updateEditors = false, $icon = 'preview', $bodyHeight = null, $modalWidth = null)

avatar HLeithner
HLeithner - comment - 6 Sep 2021

@alikon I close this because it's not needed, if you have a shared session or loggedin in frontend the preview works even for an unpublished article. Or do I miss something?

avatar HLeithner HLeithner - close - 6 Sep 2021
avatar HLeithner HLeithner - change - 6 Sep 2021
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2021-09-06 18:35:34
Closed_By HLeithner
Labels Added: ?
avatar PhilETaylor
PhilETaylor - comment - 6 Sep 2021

@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.

avatar PhilETaylor
PhilETaylor - comment - 6 Sep 2021

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?

avatar alikon
alikon - comment - 6 Sep 2021

if you have a shared session or loggedin in frontend the preview works even for an unpublished article.

and if you have not ?

avatar HLeithner
HLeithner - comment - 6 Sep 2021

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.

avatar PhilETaylor
PhilETaylor - comment - 6 Sep 2021

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?

avatar HLeithner
HLeithner - comment - 6 Sep 2021

@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)

avatar HLeithner
HLeithner - comment - 6 Sep 2021

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)

avatar PhilETaylor
PhilETaylor - comment - 6 Sep 2021

. 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);
	}
avatar PhilETaylor
PhilETaylor - comment - 6 Sep 2021

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 :)

avatar PhilETaylor
PhilETaylor - comment - 6 Sep 2021

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.

avatar HLeithner
HLeithner - comment - 6 Sep 2021

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?

avatar PhilETaylor
PhilETaylor - comment - 6 Sep 2021

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.)

avatar PhilETaylor
PhilETaylor - comment - 6 Sep 2021

Also important to note, that preview function cited by @alikon and referenced by me IS NOT USED for this toolbar button!!! so that &task=preview is NOT appended. (tested by deleting that method from the ToolbarHelper.php !!!)

avatar PhilETaylor
PhilETaylor - comment - 6 Sep 2021

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);
avatar HLeithner
HLeithner - comment - 6 Sep 2021

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 :-(

avatar PhilETaylor
PhilETaylor - comment - 6 Sep 2021

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.

avatar alikon
alikon - comment - 6 Sep 2021

did some of you ever tryied that silly code . and maybe even on debug ? or it's asking too much ?
Screenshot from 2021-09-06 21-08-13

avatar PhilETaylor
PhilETaylor - comment - 6 Sep 2021

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:

  1. That is your code in your PR. "article.preview" doesnt show anywhere in Joomla 4 other than in your PR.
  2. I use debug mode = on and error_reporting = development (and php.ini display_errors=on and error_reporting=E_ALL) all the time, every day.
  3. Asking too much? I tested the PR, I actually looked at the PR on the sofa on the mobile when the notification came through, saw that it was obviously wrong and a security risk, and walked to my mac to test to ensure I was right - and it was a security issue. I tested the PR as presented.
  4. The code you linked to thinking it generated the Preview button was actually the wrong code. even the code you thought was generating the url used task=preview and not preview=INT like your PR assumes.
  5. The actual Preview Button code used by Joomla 4 in CoreButtonsTrait.php doesnt append any preview type variable to the url
  6. The code generating the preview url, that is passed to the button, also doesnt append any preview type variable, in HtmlView.php

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...

avatar HLeithner
HLeithner - comment - 6 Sep 2021

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).

avatar alikon
alikon - comment - 6 Sep 2021

my point was only that there is some kind of "preview" info
...not more nor less ...
looking forward to see a good 1 pr
?

avatar Franzwohlkoenig
Franzwohlkoenig - comment - 7 Sep 2021

@HLeithner

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.

avatar HLeithner
HLeithner - comment - 7 Sep 2021

@Franzwohlkoenig

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

avatar HLeithner
HLeithner - comment - 7 Sep 2021

@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).

avatar PhilETaylor
PhilETaylor - comment - 7 Sep 2021

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

avatar HLeithner
HLeithner - comment - 7 Sep 2021

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?

avatar PhilETaylor
PhilETaylor - comment - 7 Sep 2021

Would you like and have the time to create the preview information 404 pr?

on the back burner yes, this week Im hoping to get my teeth into the more important CloudFlare issue #35244

Add a Comment

Login with GitHub to post a comment