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

Unquoted AssertionConsumerServiceURL gives pareser error #201

Open
ebogaard opened this Issue Mar 13, 2017 · 12 comments

Comments

Projects
None yet
5 participants
@ebogaard

ebogaard commented Mar 13, 2017

Recently SuiteCRM implemented a news version of php-saml: 2.10.2.
This used to be a really old v1.x, which worked fine, but was unsafe.

This new version does lead to a problem, though: logging in doesn't work and Firefox SAMLtracer shows me I get a parsererror:

<parsererror>
    <sourcetext>    AssertionConsumerServiceURL=&quot;https://test.xxx.nl//index.php?action=Login&amp;module=Users"&gt;
-----------------------------------------------------------------------------------------------^</sourcetext>
</parsererror>

I suspect replacing the quotes with '&quot;' is the culprit, as without the quotes the content is seen as part of the xml and is then parsed (which probably doesn't work with characters like & and ?).
I can see these quotes are in place with the old version of php-saml in SuiteCRM.

See my original report as SuiteCRM issue #2819 and it's new issue: #3270.
It seems they think it's the php-saml library though.

@chris001

This comment has been minimized.

Show comment
Hide comment
@chris001

chris001 Apr 12, 2017

@pitbulk Any suggestion? Which code is adding the " around the URL? Which code is converting the " to &quot;? Is the " even necessary to have around the URL? Could the " around the URL be removed? Should it be removed?

chris001 commented Apr 12, 2017

@pitbulk Any suggestion? Which code is adding the " around the URL? Which code is converting the " to &quot;? Is the " even necessary to have around the URL? Could the " around the URL be removed? Should it be removed?

@pitbulk

This comment has been minimized.

Show comment
Hide comment
@pitbulk

pitbulk Apr 15, 2017

Contributor

Hi @chris001,

I was able to reproduce it on demo1, connecting it with simpleSAMLphp and see that error on SAMLTracer, adding an extra GET parameter on the ACSURL.

Are you adding extra GET parameters on ACS URL? Why you need to do that?

Contributor

pitbulk commented Apr 15, 2017

Hi @chris001,

I was able to reproduce it on demo1, connecting it with simpleSAMLphp and see that error on SAMLTracer, adding an extra GET parameter on the ACSURL.

Are you adding extra GET parameters on ACS URL? Why you need to do that?

@apano

This comment has been minimized.

Show comment
Hide comment
@apano

apano May 10, 2017

Hi @chris001 ,

I edited onelogin/php-saml/lib/Saml2/AuthnRequest.php. I replaced the var with AssertionConsumerServiceURL= {$spData['assertionConsumerService']['url']} but yet cannot get to excape & being parsed to &amp;

I also tried changing it in settings.php but no success yet. Current string shows: AssertionConsumerServiceURL= https://URL/index.php?action=Login&amp;module=Users

apano commented May 10, 2017

Hi @chris001 ,

I edited onelogin/php-saml/lib/Saml2/AuthnRequest.php. I replaced the var with AssertionConsumerServiceURL= {$spData['assertionConsumerService']['url']} but yet cannot get to excape & being parsed to &amp;

I also tried changing it in settings.php but no success yet. Current string shows: AssertionConsumerServiceURL= https://URL/index.php?action=Login&amp;module=Users

@chris001

This comment has been minimized.

Show comment
Hide comment
@chris001

chris001 May 10, 2017

@pitbulk @apano

Are you adding extra GET parameters on ACS URL? Why you need to do that?

SuiteCRM is based on SugarCRM, which is a php web application first developed in 2004, so practically every page of the app uses GET parameters, instead of rewritten SEF url's. So the login URL is https://www.mysuitecrmapp.com/index.php?action=Login&module=Users

Would it be accurate to say, the php-saml code is urlencoding the AssertionConsumerService URL (the return URL to go back to the application) when it should just add on its own get or post parameters and not urlencode the URL?

chris001 commented May 10, 2017

@pitbulk @apano

Are you adding extra GET parameters on ACS URL? Why you need to do that?

SuiteCRM is based on SugarCRM, which is a php web application first developed in 2004, so practically every page of the app uses GET parameters, instead of rewritten SEF url's. So the login URL is https://www.mysuitecrmapp.com/index.php?action=Login&module=Users

Would it be accurate to say, the php-saml code is urlencoding the AssertionConsumerService URL (the return URL to go back to the application) when it should just add on its own get or post parameters and not urlencode the URL?

@apano

This comment has been minimized.

Show comment
Hide comment
@apano

apano May 11, 2017

@chris001
In my case I am not adding extra GET parameters. The login URL is defined by the main config site_url and then from sp_base. The AssertionConsumerServiceURL I pasted, is the one I receive from Firefox's SAML Plugin, but I confirm that what arrives to ADFS is correctly parsed https://www.mysuitecrmapp.com/index.php?action=Login&module=Users. I have been checking older versions of SuiteCRM and SugarCRM to check if there is something else to change for the federation but no luck.

apano commented May 11, 2017

@chris001
In my case I am not adding extra GET parameters. The login URL is defined by the main config site_url and then from sp_base. The AssertionConsumerServiceURL I pasted, is the one I receive from Firefox's SAML Plugin, but I confirm that what arrives to ADFS is correctly parsed https://www.mysuitecrmapp.com/index.php?action=Login&module=Users. I have been checking older versions of SuiteCRM and SugarCRM to check if there is something else to change for the federation but no luck.

@pitbulk

This comment has been minimized.

Show comment
Hide comment
@pitbulk

pitbulk May 11, 2017

Contributor

I reviewed simpleSAMLphp and it do nothing related with encoding on assertionConsumerServiceURL.

I see that there is a problem with urls that includes GET parameters, but I don't see urlencoding the whole URL as a solution (I think that can carry new issues).

The demo1 uses ? and works, so I think the issue is only related with the use of &.

Have you checked to use as AssertionConsumerServiceURL
https://www.mysuitecrmapp.com/index.php?action=Login&amp;module=Users
(html entity encoded version?)

Contributor

pitbulk commented May 11, 2017

I reviewed simpleSAMLphp and it do nothing related with encoding on assertionConsumerServiceURL.

I see that there is a problem with urls that includes GET parameters, but I don't see urlencoding the whole URL as a solution (I think that can carry new issues).

The demo1 uses ? and works, so I think the issue is only related with the use of &.

Have you checked to use as AssertionConsumerServiceURL
https://www.mysuitecrmapp.com/index.php?action=Login&amp;module=Users
(html entity encoded version?)

pitbulk added a commit that referenced this issue May 18, 2017

@pitbulk

This comment has been minimized.

Show comment
Hide comment
@pitbulk

pitbulk May 18, 2017

Contributor

@apano @chris001 @ebogaard

Can you verify #218 solve the issue?

Contributor

pitbulk commented May 18, 2017

@apano @chris001 @ebogaard

Can you verify #218 solve the issue?

@ebogaard

This comment has been minimized.

Show comment
Hide comment
@ebogaard

ebogaard May 18, 2017

This fixes the problem.
Great!

ebogaard commented May 18, 2017

This fixes the problem.
Great!

pitbulk added a commit that referenced this issue May 18, 2017

Merge pull request #218 from onelogin/quotespurls
#201. Fix issues with SP entity_id, acs url and sls url that contains &
@jrossiter

This comment has been minimized.

Show comment
Hide comment
@jrossiter

jrossiter Jun 9, 2017

@pitbulk We have an IDP that fails to match the Destination URL set in the OneLogin_Saml2_AuthnRequest constructor.

Adding the following resolved it; could it be related to #218? This is also due to the & character being in their SSO URL.

php-saml\lib\Saml2\AuthnRequest.php
Line 118: $sso_url = htmlspecialchars($idpData['singleSignOnService']['url'], ENT_QUOTES);
...
Line 129: Destination="{$sso_url}"

jrossiter commented Jun 9, 2017

@pitbulk We have an IDP that fails to match the Destination URL set in the OneLogin_Saml2_AuthnRequest constructor.

Adding the following resolved it; could it be related to #218? This is also due to the & character being in their SSO URL.

php-saml\lib\Saml2\AuthnRequest.php
Line 118: $sso_url = htmlspecialchars($idpData['singleSignOnService']['url'], ENT_QUOTES);
...
Line 129: Destination="{$sso_url}"
@pitbulk

This comment has been minimized.

Show comment
Hide comment
@pitbulk

pitbulk Jun 9, 2017

Contributor

The SSO URL belong the IdP side.
What IdP software are you using that adds an & on its SSO URL?

Contributor

pitbulk commented Jun 9, 2017

The SSO URL belong the IdP side.
What IdP software are you using that adds an & on its SSO URL?

@jrossiter

This comment has been minimized.

Show comment
Hide comment
@jrossiter

jrossiter Jun 9, 2017

It's a customers IDP (possibly homegrown). Unfortunately due to the size of their company they won't change how their URLs are constructed so it was the only way to resolve the issue while maintaining compatibility with other IDPs.

jrossiter commented Jun 9, 2017

It's a customers IDP (possibly homegrown). Unfortunately due to the size of their company they won't change how their URLs are constructed so it was the only way to resolve the issue while maintaining compatibility with other IDPs.

@pitbulk

This comment has been minimized.

Show comment
Hide comment
@pitbulk

pitbulk Jun 9, 2017

Contributor

Ok, not very common to find that.

P.S if the SLO URL has also & you will need to transform them as well.

Contributor

pitbulk commented Jun 9, 2017

Ok, not very common to find that.

P.S if the SLO URL has also & you will need to transform them as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment