Allow overriding of host, port, protocol nsdr url path for URL building #175

pitbulk · 2016-11-13T20:01:40Z

A developer can use setSelfProtocol, setSelfHost, setSelfPort and getBaseURLPath to define a specific value to be returned by isHTTPS, getSelfHost, getSelfPort and getBaseURLPath. And define a setBasePath to be used on the getSelfURL and getSelfRoutedURLNoQuery to replace the data extracted from $_SERVER["REQUEST_URI"].

At the settings the developer will be able to set a 'baseurl' parameter that automatically will use setBaseURL to set values for setSelfProtocol, setSelfHost, setSelfPort and setBaseURLPath.

- Added ability to set custom host/port/protocol - Updated test coverage to include testing HTTP_X_FORWARDED_PORT - Split port detection into its own method

…saml into dhensby-pulls/url-injection

… parameter on the settings the Base URL to be used instead of guessing the URL of the currentURL where SAML messages are processed.

pitbulk · 2016-11-13T20:02:24Z

@dhensby, sorry for the delay, can you review this PR?

dhensby

One blocker and a few other suggestions

dhensby · 2016-11-13T21:28:43Z

lib/Saml2/Utils.php

+        if (!empty($baseurl)) {
+            $baseurlpath = '/';
+            if (preg_match('#^https?:\/\/([^\/]*)\/?(.*)#i', $baseurl, $matches)) {
+                if (strpos($baseurl, 'https://') == False) {


this needs to be type equality (===) and you should use false not False

dhensby · 2016-11-13T21:30:16Z

lib/Saml2/Utils.php

+     */
+    public static function setBaseURLPath($baseurlpath)
+    {
+        if (!isset($baseurlpath) || empty($baseurlpath)) {


you can just used empty as isset will only return false if $baseurlpath is explicitly null, but empty will return true for null anyway.

dhensby · 2016-11-13T21:31:37Z

lib/Saml2/Utils.php

+            $baseurlpath = '/';
+        }
+
+        self::$_baseurlpath = '/'. ltrim(rtrim($baseurlpath, '/') . '/', '/');


Could you not do '/'.trim($baseurlpath, '/') . '/';

I wanted to assure that all start and end with a /. What the reason for avoiding that?

Won't my suggestion have the same effect? Trim / off both ends then spend it to both?

haha, yes, sorry, yesterday was so late when I reviewed that

Ohh, when $baseurlpath ==> '/'

'/'.trim($baseurlpath, '/') . '/'; returns //
but '/'. ltrim(rtrim($baseurlpath, '/') . '/', '/'); returns /

I will use

if (empty($baseurlpath) || $baseurlpath == '/') { $baseurlpath = '/'; } else { self::$_baseurlpath = '/' . trim($baseurlpath, '/') . '/'; }

Ah, good point and that solution is more readable too, so cool.

dhensby · 2016-11-13T21:32:56Z

lib/Saml2/Utils.php

        $selfURLhost = self::getSelfURLhost();
-        $selfURLNoQuery = $selfURLhost . $_SERVER['SCRIPT_NAME'];
+
+        if (!empty(self::getBaseURLPath())) {


you can't use empty with method in php < 5.5 so you need to assign a var first as the module supports 5.3+

Sure, I noticed that on travis

dhensby · 2016-11-13T21:33:59Z

lib/Saml2/Utils.php

@@ -493,6 +564,17 @@ public static function getSelfRoutedURLNoQuery()
            }
        }

+        if (!empty(self::getBaseURLPath())) {


This code is repeated and could be pulled into a protected method and re-used

pitbulk · 2016-11-13T23:47:57Z

Thanks, I will fix that.

pitbulk · 2016-11-14T11:25:07Z

any scenario we are not covering? Are we now ready to merge?

dhensby

Looks good to me

patricknelson · 2017-08-18T01:26:49Z

For the life of me, I can't seem to really figure out why on earth why Util::getSelfRoutedURLNoQuery() was setup to call ::buildWithBaseURLPath() here.

Also, why is it that ::getSelfRoutedURLNoQuery() strips out the entire request URI except for the last URL segment? This causes URL's like: https://domain/saml/acs to be converted to https://domain/acs (i.e. request URI becomes just that last segment) simply because your base path is / and thus you end up with an error resulting during response validation:

A valid SubjectConfirmation was not found on this Response

This is because the $currentURL (from here) now contains https://domain/acs even though the Recipient in SubjectConfirmationData contains the full URL and fails the check on this line: https://github.com/onelogin/php-saml/blob/master/lib/Saml2/Response.php#L306

It seems as if this is missing a thorough explanation for the thought process behind changing the request URI like this when generating the current URL. What am I missing here?

patricknelson · 2017-08-18T01:27:15Z

p.s. I first mentioned this here on this comment and tracked it down to this issue. silverstripe/silverstripe-activedirectory#104 (comment)

…id URL parsing, documented here: SAML-Toolkits/php-saml#175 (comment)

dhensby and others added 4 commits September 8, 2016 20:38

NEW Allow overriding of host, port and protocol for URL building

93b35c9

- Added ability to set custom host/port/protocol - Updated test coverage to include testing HTTP_X_FORWARDED_PORT - Split port detection into its own method

Merge branch 'pulls/url-injection' of https://github.com/dhensby/php-…

b1e2bda

…saml into dhensby-pulls/url-injection

Merge PR Allow overriding of host, port and protocol for URL building

50984cb

Let the developer set by methods at the Utils class or by a 'baseurl'…

d848523

… parameter on the settings the Base URL to be used instead of guessing the URL of the currentURL where SAML messages are processed.

pitbulk mentioned this pull request Nov 13, 2016

SAML response is valid using www.samltool.com but fails on php-saml #167

Closed

Fix codestyle

bdd88b1

pitbulk force-pushed the url-injection branch from 79addd1 to bdd88b1 Compare November 13, 2016 20:12

dhensby suggested changes Nov 13, 2016

View reviewed changes

Suggested changes

0258d28

dhensby approved these changes Nov 14, 2016

View reviewed changes

pitbulk merged commit a18bb25 into master Nov 15, 2016

patricknelson added a commit to patricknelson/silverstripe-saml that referenced this pull request Aug 18, 2017

Working around 'SubjectConfirmation was not found' issue due to inval…

3532565

…id URL parsing, documented here: SAML-Toolkits/php-saml#175 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow overriding of host, port, protocol nsdr url path for URL building #175

Allow overriding of host, port, protocol nsdr url path for URL building #175

pitbulk commented Nov 13, 2016

pitbulk commented Nov 13, 2016

dhensby left a comment

dhensby Nov 13, 2016

pitbulk Nov 13, 2016

dhensby Nov 13, 2016

pitbulk Nov 13, 2016

dhensby Nov 13, 2016

pitbulk Nov 13, 2016

dhensby Nov 14, 2016

pitbulk Nov 14, 2016

pitbulk Nov 14, 2016 •

edited

Loading

dhensby Nov 14, 2016

dhensby Nov 13, 2016

pitbulk Nov 13, 2016

dhensby Nov 13, 2016

pitbulk Nov 13, 2016

pitbulk commented Nov 13, 2016

pitbulk commented Nov 14, 2016

dhensby left a comment

patricknelson commented Aug 18, 2017

patricknelson commented Aug 18, 2017

Allow overriding of host, port, protocol nsdr url path for URL building #175

Allow overriding of host, port, protocol nsdr url path for URL building #175

Conversation

pitbulk commented Nov 13, 2016

pitbulk commented Nov 13, 2016

dhensby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitbulk Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitbulk commented Nov 13, 2016

pitbulk commented Nov 14, 2016

dhensby left a comment

Choose a reason for hiding this comment

patricknelson commented Aug 18, 2017

patricknelson commented Aug 18, 2017

pitbulk Nov 14, 2016 •

edited

Loading