php-shopify icon indicating copy to clipboard operation
php-shopify copied to clipboard

AuthHelper::getCurrentUrl() doesn't strip out querystring

Open syosoft opened this issue 4 years ago • 5 comments

Found a bug in AuthHelper related to getCurrentUrl() (the querystring isn't stripped out). It's easy enough to fix, however - If static::getCurrentUrl() was used in place of self::getCurrentUrl() in \PHPShopify\AuthHelper::createAuthRequest() I could simply:

class PHPShopify_AuthHelper_BugFix extends \PHPShopify\AuthHelper {
        public static function getCurrentUrl() {
                $redirectUrl = parent::getCurrentUrl();

                // Shopify doesn't want a querystring, just the protocol and store host
                // The returned URL *MUST* match one of the app redirect URLs
                return FALSE !== ($qs_pos = strpos($redirectUrl, '?')) ? substr($redirectUrl, 0, $qs_pos) : $redirectUrl;
        }
}

Instead of having to duplicate the \PHPShopify\AuthHelper::createAuthRequest() method in this PHPShopify_AuthHelper_BugFix class just to have my overloaded method called because an inconvenient static scope was used.

syosoft avatar Dec 03 '20 18:12 syosoft

Got the same issue. Your solution is removing all data after ? symbol. What if we have a redirect_uri with query params? P.S. Temporarily used a hack in a second argument of AuthHelper::createAuthRequest: (isset($_GET['code']) ? null : 'https://example.com/current_uri')

waleev avatar May 13 '21 09:05 waleev

In current state you can't use the same page for authorize and redirect_uri. Because Shopify throws an error about redirect_uri is not whitelisted. And you cannot whitelist this uri with generated query params, such as hmac and etc.

waleev avatar May 13 '21 09:05 waleev

Created a PR #227

waleev avatar May 13 '21 10:05 waleev

This issue can be closed. PR #227 has been accepted.

waleev avatar Jun 17 '21 10:06 waleev

@tareqtms

waleev avatar Jun 28 '21 04:06 waleev