Choose the correct protocol in route::link when force https is disabled by HLeithner · Pull Request #24089 · joomla/joomla-cms

HLeithner · 2019-03-04T21:02:49Z

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 · 2019-03-04T22:05:34Z

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

joomla-cms/administrator/components/com_menus/models/forms/item_component.xml

Lines 133 to 142 in 7515081

    
           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 · 2019-03-05T06:32:31Z

Value 2 is forcing http and that should still works.

SharkyKZ · 2019-03-05T06:45:41Z

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

HLeithner · 2019-03-05T14:41:43Z

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 · 2019-03-05T15:20:14Z

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 · 2019-03-05T15:38:03Z

-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 · 2019-03-05T15:53:53Z

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 · 2019-03-05T16:33:27Z

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

HLeithner · 2019-03-05T19:42:41Z

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

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

libraries/src/Router/Route.php

wilsonge · 2019-03-05T19:54:00Z

libraries/src/Router/Route.php

 	 * @since   1.7.0
 	 */
-	public static function _($url, $xhtml = true, $ssl = null)
+	public static function _($url, $xhtml = true, $tls = self::TLS_IGNORE, $absolute = false)


Do we really need an $absolute param here?

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

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.

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)

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;

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

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.

ghost · 2019-04-24T07:02:01Z

@chrisxchrisxchrisx please test > https://docs.joomla.org/Testing_Joomla!_patches

chrisxchrisxchrisx · 2019-04-24T20:30:16Z

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

libraries/src/Router/Route.php

ghost · 2019-04-25T04:15:25Z

@chrisxchrisxchrisx please mark your test as unsuccessfully > https://docs.joomla.org/Testing_Joomla!_patches#Recording_test_results

chrisxchrisxchrisx · 2019-04-25T10:14:54Z

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

# Conflicts: # administrator/components/com_actionlogs/helpers/actionlogs.php

HLeithner · 2019-04-25T14:21:38Z

thx @chrisxchrisxchrisx fixed now

chrisxchrisxchrisx · 2019-04-26T06:02:31Z

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 · 2019-04-26T09:12:54Z

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

ghost · 2019-04-26T09:29:04Z

Status "Ready To Commit".

libraries/src/Router/Route.php

zero-24 · 2019-04-27T12:41:57Z

libraries/src/Router/Route.php

 		{
-			// 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;


Any suggestion how todo this without flooding the log?

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?

zero-24 · 2019-04-27T12:44:46Z

components/com_users/models/registration.php

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


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?

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.

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

f99e3da

joomla-cms-bot added the PR-staging label Mar 4, 2019

HLeithner mentioned this pull request Mar 4, 2019

registration form menu item metadata security https no longer honoured since 3.9.0 uses http for registration links in emails #23046

Closed

Add absolute parameter to route::_, link and introduce new TLS constants

f549b20

wilsonge reviewed Mar 5, 2019

View reviewed changes

libraries/src/Router/Route.php Outdated Show resolved Hide resolved

wilsonge reviewed Mar 5, 2019

View reviewed changes

HLeithner added 2 commits March 5, 2019 20:54

cs

e814241

Added deprecation tag

bb54c76

ghost added J3 Issue and removed J3 Issue labels Apr 5, 2019

ghost removed the J3 Issue label Apr 19, 2019

mbabker reviewed Apr 24, 2019

View reviewed changes

libraries/src/Router/Route.php Show resolved Hide resolved

HLeithner added 3 commits April 25, 2019 14:28

Add since tag for constants

1fe652e

Merge branch 'staging' into fix-route-link-ssl

93fe901

# Conflicts: # administrator/components/com_actionlogs/helpers/actionlogs.php

Include namespaces

4610fe4

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

Merge branch 'staging' into fix-route-link-ssl

05226d3

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

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 26, 2019

HLeithner added this to the Joomla 3.9.6 milestone Apr 26, 2019

HLeithner mentioned this pull request Apr 26, 2019

Make sure the reset token is invalidated when the email is changed and an token exists #22843

Closed

Merge branch 'staging' into fix-route-link-ssl

3194253

zero-24 reviewed Apr 27, 2019

View reviewed changes

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

HLeithner added 2 commits May 1, 2019 12:02

Add deprecation todo

2bf9019

Merge branch 'staging' into fix-route-link-ssl

7dba0f1

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

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 5, 2019

HLeithner mentioned this pull request Jun 16, 2019

Fix not converted boolean in route::_ and log warning #25225

Merged

HLeithner deleted the fix-route-link-ssl branch March 29, 2020 19:15

Uh oh!

Conversation

HLeithner commented Mar 4, 2019

Summary of Changes

Testing Instructions

Expected result

Actual result

Uh oh!

SharkyKZ commented Mar 4, 2019

Uh oh!

HLeithner commented Mar 5, 2019

Uh oh!

SharkyKZ commented Mar 5, 2019

Uh oh!

HLeithner commented Mar 5, 2019

Uh oh!

SharkyKZ commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HLeithner commented Mar 5, 2019

Uh oh!

SharkyKZ commented Mar 5, 2019

Uh oh!

SharkyKZ commented Mar 5, 2019

Uh oh!

HLeithner commented Mar 5, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SharkyKZ Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Apr 24, 2019

Uh oh!

chrisxchrisxchrisx commented Apr 24, 2019

Uh oh!

Uh oh!

ghost commented Apr 25, 2019

Uh oh!

chrisxchrisxchrisx commented Apr 25, 2019

Uh oh!

HLeithner commented Apr 25, 2019

Uh oh!

chrisxchrisxchrisx commented Apr 26, 2019

Uh oh!

alikon commented Apr 26, 2019

Uh oh!

ghost commented Apr 26, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

SharkyKZ commented Mar 5, 2019 •

edited

Loading

SharkyKZ Mar 5, 2019 •

edited

Loading