BingAds / BingAds-PHP-SDK

Other
56 stars 45 forks source link

AuthorizationEndpointUrl and URL encoding in general #124

Closed Mischosch closed 4 years ago

Mischosch commented 4 years ago

Hi there,

see https://github.com/BingAds/BingAds-PHP-SDK/blob/ad4f5881df7e171115fb2444b87ce562f76b81ba/src/Auth/UriOAuthService.php#L14

Using it ends up in a url like this: https://login.microsoftonline.com/common/oauth2/v2.0/authorize?scope=https://ads.microsoft.com/ads.manage offline_access&client_id=12345678-abcd-4321-dcba-abcdefghijkl&response_type=code&redirect_uri=https://login.microsoftonline.com/common/oauth2/nativeclient

There is white space inside the url between ads.manage and offline_access. Seems wrong and is breaking urls. The second url inside scope attribute should be completely url encoded: https%3A%2F%2Fads.microsoft.com%2Fads.manage+offline_access.

The redirect_uri attribute is not url encoded, too. Should be https%3A%2F%2Flogin.microsoftonline.com%2Fcommon%2Foauth2%2Fnativeclient. This could happen inside https://github.com/BingAds/BingAds-PHP-SDK/blob/ad4f5881df7e171115fb2444b87ce562f76b81ba/src/Auth/UriOAuthService.php#L139

The generated url might work in modern browsers, because they handle crazy urls quite well. It broke the link inside my tool. So it does not work for every usecase.

Can send a PR for it if this should get fixed in your opinion, too.

eric-urban commented 4 years ago

@Mischosch thanks for raising this issue. We'll add URL encoding next release (13.0.4). Thanks

eric-urban commented 4 years ago

@Mischosch this should be fixed in version 13.0.4. Thanks!