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

Choose the correct protocol in route::link when force https is disabled #24089

Merged
merged 11 commits into from Jun 5, 2019

Conversation

Projects
None yet
10 participants
@HLeithner
Copy link
Member

commented Mar 4, 2019

Pull Request for Issue #23046 .

Summary of Changes

Use the correct http schema.

Testing Instructions

Test user registration with and without forced https on http and https

Expected result

  • Registration email is http if you are on http when force http is "none"
  • Registration email is https if you are on https when force http is "none"
  • Registration email is https if you are on https when force http is "Entire Site"

Actual result

  • Registration email is http if you are on https when force http is "none"
@SharkyKZ

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

It's not documented anymore, but $ssl with value -1 should force HTTP. It's what menu items have:

name="secure"
type="list"
label="COM_MENUS_ITEM_FIELD_SECURE_LABEL"
description="COM_MENUS_ITEM_FIELD_SECURE_DESC"
default="0"
filter="integer"
>
<option value="-1">JOFF</option>
<option value="1">JON</option>
<option value="0">COM_MENUS_FIELD_VALUE_IGNORE</option>

Basing protocol on request would require another option, I think.

@HLeithner

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2019

Value 2 is forcing http and that should still works.

@SharkyKZ

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

Value -1 should do that also, as described above. With this patch, menu item settings aren't respected anymore.

@HLeithner

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2019

The API for route::link is different then the https force options.

We convert it before we call route::link
you will find this in the registration.php:
$linkMode = $config->get('force_ssl', 0) == 2 ? 1 : -1;

@SharkyKZ

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

Secure menu item option is different from Force SSL option in global configuration. Somewhere between 3.1 and 3.2 the description of $ssl param was changed but behavior remained the same. This is from 3.1.0:

/**
 * Translates an internal Joomla URL to a humanly readible URL.
 *
 * @param   string   $url    Absolute or Relative URI to Joomla resource.
 * @param   boolean  $xhtml  Replace & by &amp; for XML compilance.
 * @param   integer  $ssl    Secure state for the resolved URI.
 *                             1: Make URI secure using global secure site URI.
 *                             0: Leave URI in the same secure state as it was passed to the function.
 *                            -1: Make URI unsecure using the global unsecure site URI.
 *
 * @return  The translated humanly readible URL.
 *
 * @since   11.1
 */
public static function _($url, $xhtml = true, $ssl = null)

So it's technically correct that -1 forces HTTP. But we do need a new option to use protocol from request.

@HLeithner

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2019

-1 doesn't force it, -1 uses the current protocol. It's used because it's int and not 0 but don't ask me why not NULL is used.

@SharkyKZ

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

Without patch value -1 forces HTTP and this is fine. Changing this is going to break secure option in menu items. Also with this patch value 2 doesn't force HTTP when current URL is HTTPS.

A B/C safe solution is to introduce another value to replace -1 wherever we want to use protocol from request.

@SharkyKZ

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

To be more clear, behavior with both -1 and 2 is exactly the same and it must remain so.

@HLeithner

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2019

So I rewrote route::_ and route::link to support absolute parameter.

And also deprecated -1 and added constants to the route class.

Show resolved Hide resolved libraries/src/Router/Route.php Outdated
*
* @return string The translated humanly readable URL.
*
* @since 1.7.0
*/
public static function _($url, $xhtml = true, $ssl = null)
public static function _($url, $xhtml = true, $tls = self::TLS_IGNORE, $absolute = false)

This comment has been minimized.

Copy link
@wilsonge

wilsonge Mar 5, 2019

Contributor

Do we really need an $absolute param here?

This comment has been minimized.

Copy link
@HLeithner

HLeithner Mar 5, 2019

Author Member

yes, route::_ is now a short cut to the current client router so should have the same features as route::link

This comment has been minimized.

Copy link
@SharkyKZ

SharkyKZ Mar 5, 2019

Contributor

New parameter is not necessary. Just have 4 acceptable values for $ssl. I.e. 0 keep relative URL, 1 force HTTPS, 2 force HTTP, 3 use protocol from request.

This comment has been minimized.

Copy link
@HLeithner

HLeithner Mar 5, 2019

Author Member

Thats not true, its a pain everytime you try to create a link that can't be relative (like all I added true in this PR) or if you create a link for an external system (api, json, mail you name it)

This comment has been minimized.

Copy link
@SharkyKZ

SharkyKZ Mar 5, 2019

Contributor

What I mean is you just need another constant/value for absolute URL with protocol from request, e.g.:

RELATIVE = 0;
HTTPS = 1;
HTTP = 2;
REQUEST = 3;

And then in the code use:

$linkMode = $config->get('force_ssl', 0) == 2 ? Route::HTTPS : Route::REQUEST;

This comment has been minimized.

Copy link
@HLeithner

HLeithner Mar 6, 2019

Author Member

In my opinion this are 2 complete different things. Thats the reason for an extra parameter.

This comment has been minimized.

Copy link
@SharkyKZ

SharkyKZ Mar 28, 2019

Contributor

Having another constant would be cleaner, in my opinion. A separate parameter won't have any effect when $tls is 1 or 2. But you're the lead, you decide. If you do go with a parameter, please add a note that it only takes effect when $tls = 0.

Now fix the conflicts please and let's have this tested.

HLeithner added some commits Mar 5, 2019

cs
@franz-wohlkoenig

This comment has been minimized.

@chrisxchrisxchrisx

This comment has been minimized.

Copy link

commented Apr 24, 2019

Thank you all for your work. The test was not successful but it is the first time I have tested a Joomla patch so please bear with me if it was my fault:

Steps to test the patch

In Joomla "staging" from https://github.com/joomla/joomla-cms
With release patch installed but no patch applied from https://github.com/joomla-extensions/patchtester/releases/tag/2.0.1

Users: Options: Allow User Registration: Yes

Users: Options: New User Account Activation: Administrators

include {loadmodule mod_login} in an article

link a published menu module to a visible menu

publish a single article menu item for the article in the visible menu

create a hidden menu not linked to any menu module

publish a menu item alias menu item for the single article menu item in the hidden menu

publish a users registration form menu item with the menu item alias menu item as owner

set all the above menu items Metadata Secure = On

select the single article menu item and select Create an account

note that registration links emailed to users and administrators do not honour the security setting but instead use http:

apply patch 24089

clear Joomla cache and expired cache and browser cache

select the single article menu item and select Create an account

Expected result

registration links emailed to users and administrators honour the security setting by using https:

Actual result

https://localhost/joomla-cms-staging/index.php/registration-form?task=registration.register
Error message: The requested page cannot be found
Error: 0 Class 'Route' not found

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

@chrisxchrisxchrisx

This comment has been minimized.

Copy link

commented Apr 25, 2019

I have tested this item 🔴 unsuccessfully on bb54c76

Thank you all for your work. The test was not successful but it is the first time I have tested a Joomla patch so please bear with me if it was my fault:

Steps to test the patch

In Joomla "staging" from https://github.com/joomla/joomla-cms
With release patch installed but no patch applied from https://github.com/joomla-extensions/patchtester/releases/tag/2.0.1

Users: Options: Allow User Registration: Yes

Users: Options: New User Account Activation: Administrators

include {loadmodule mod_login} in an article

link a published menu module to a visible menu

publish a single article menu item for the article in the visible menu

create a hidden menu not linked to any menu module

publish a menu item alias menu item for the single article menu item in the hidden menu

publish a users registration form menu item with the menu item alias menu item as owner

set all the above menu items Metadata Secure = On

select the single article menu item, select Create an account, complete form and submit

note that registration links emailed to users and administrators do not honour the security setting but instead use http:

apply patch 24089

clear Joomla cache and expired cache and browser cache

select the single article menu item, select Create an account, complete form and submit

Expected result

registration links emailed to users and administrators honour the security setting by using https:

Actual result

at index.php/registration-form?task=registration.register
Error message: The requested page cannot be found
Error: 0 Class 'Route' not found


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

HLeithner added some commits Apr 25, 2019

Merge branch 'staging' into fix-route-link-ssl
# Conflicts:
#	administrator/components/com_actionlogs/helpers/actionlogs.php
@HLeithner

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

thx @chrisxchrisxchrisx fixed now

@HLeithner HLeithner added this to the Joomla 3.9.7 milestone Apr 25, 2019

@chrisxchrisxchrisx

This comment has been minimized.

Copy link

commented Apr 26, 2019

I have tested this item successfully on 4610fe4

Test as before yields expected result. Well done guys.


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

@alikon

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

I have tested this item successfully on 4610fe4


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

@joomla-cms-bot joomla-cms-bot removed this from the Joomla 3.9.7 milestone Apr 26, 2019

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

Status "Ready To Commit".

Show resolved Hide resolved libraries/src/Router/Route.php
}
catch (\RuntimeException $e)
{
// Before 3.9.0 this method failed silently on router error. This B/C will be removed in Joomla 4.0.
// @deprecated 4.0 Before 3.9.0 this method failed silently on router error. This B/C will be removed in Joomla 4.0.
return null;

This comment has been minimized.

Copy link
@zero-24

zero-24 Apr 27, 2019

Contributor

same here

This comment has been minimized.

Copy link
@HLeithner

HLeithner May 1, 2019

Author Member

Any suggestion how todo this without flooding the log?

This comment has been minimized.

Copy link
@zero-24

zero-24 May 1, 2019

Contributor

hmm in case of an error i would expect an log, else we can't blame extension devs not seeing this deprecation? IIRC we only collect the logs in case there is debug enabled right?

'index.php?option=com_users&task=registration.activate&token=' . $data['activation'],
false,
$linkMode,
true

This comment has been minimized.

Copy link
@zero-24

zero-24 Apr 27, 2019

Contributor

just to be sure here. We now force JRoute::link to be called with $absolute = ture in order to work like before that patch here? If yes isn't that a B/C change?

This comment has been minimized.

Copy link
@HLeithner

HLeithner May 1, 2019

Author Member

The original request was -1 or 1 to $ssl this means it always returned the full url

		if ((int) $ssl || $uri->isSsl())
		{
...
			$scheme = array_merge($scheme, array('host', 'port', 'scheme'));
		}

		$url = $uri->toString($scheme);

So this should not change anything.

@HLeithner HLeithner modified the milestones: Joomla 3.9.6, Joomla 3.9.7 Apr 30, 2019

HLeithner added some commits May 1, 2019

@HLeithner HLeithner merged commit 8dba753 into joomla:staging Jun 5, 2019

4 checks passed

Hound No violations found. Woof!
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr Build is passing
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joomla-cms-bot joomla-cms-bot removed the RTC label Jun 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.