Skip to content

[PEGASUS-935] Redirect to default unauthorised zendesk url when SSO is disabled #159

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 2, 2020
Merged

[PEGASUS-935] Redirect to default unauthorised zendesk url when SSO is disabled #159

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fdfc1f1
Redirect to default un-authorized zendesk url when SSO is disabled
yoshdog Jun 30, 2020
9835707
Merge remote-tracking branch 'origin/master' into PEGASUS-935-investi…
yoshdog Jul 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/app/code/community/Zendesk/Zendesk/controllers/SsoController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@ public function loginAction()
{
$return_url = Mage::helper('core')->urlDecode($this->getRequest()->getParam('return_url', ""));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is meant to be decoding. Usually you would be taking the raw value and when adding it back to the URL below you would encode it.

Currently you could add additional query parameters to the resulting URL going to Zendesk due to lack of encoding and the manual query building. Probably benign but doesn’t seem intentional.

I would recommend http_build_query below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @driskell
is your comment in relation to lines 79 - 81?
https://github.com/zendesk/magento_extension/pull/159/files#diff-d8d07556aac1f9945f929f9079d3f0afR79-R81

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merging this as is... happy to look into this in a followup PR.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the return URL could contain ampersand. In which case rather than being elements of the return url they would become additional query parameters sent to the JWT endpoint. So could cause undesired behaviour.

For example If url is: http://hello/something?query&something=more

The resulting url in the redirect becomes:
https://zendeskdomain/access/jwt?jwt=accesstoken&return_to=http://hello/something?query&something=more
So not only not encoded but the final part won’t be part of the return_to and will be stripped when the return happens.

Can’t see if causing an issue except maybe breaking some return URLs. But I don’t know what parameters that endpoint accepts.

if(!Mage::getStoreConfig('zendesk/sso_frontend/enabled')) {
$this->_redirectUrl($return_url ? $return_url : Mage::helper('zendesk')->getZendeskUnauthUrl());
$this->_redirectUrl(Mage::helper('zendesk')->getZendeskUnauthUrl());
return $this;
}

$domain = Mage::getStoreConfig('zendesk/general/domain');
$token = Mage::getStoreConfig('zendesk/sso_frontend/token');

if(!Zend_Validate::is($domain, 'NotEmpty')) {
Mage::log(Mage::helper('zendesk')->__('Zendesk domain not set. Please add this to the settings page.'), null, 'zendesk.log');
$this->_redirect('/');
Expand Down Expand Up @@ -77,7 +77,7 @@ public function loginAction()

$jwt = JWT::encode($payload, $token);
$return_url = $return_url ? "&return_to=".$return_url : "";

$url = "https://".$domain."/access/jwt?jwt=" . $jwt.$return_url;

Mage::log('End-user URL: ' . $url, null, 'zendesk.log');
Expand Down