OfficeDev / office-js-helpers

[ARCHIVED] A collection of helpers to simplify development of Office Add-ins & Microsoft Teams Tabs
MIT License
126 stars 56 forks source link

Parameter extraction does not expect leading front slash in url #90

Open Seebiscuit opened 6 years ago

Seebiscuit commented 6 years ago

This is for a Word Taskpane Add-in.

I am trying to set up Authentication. I get as far as selecting a user inthe auth popup and the popup redirecting to the add-in again.

In Authenticator.prototype._handleTokenResult the method parses a result object with the following shape:

result = {
  /access_token: [string]
  expires_in: [string]
  scope: [string]
  state: [string]
  token_type: [string]
}

You'll notice that the first property includes a / (front slash). That's throwing off Authenticator.prototype._handleTokenResult and not passing

else if ('access_token' in result)

I followed the result parsing and it seems that because my client router injects the hash at the end of the host url (e.g., https://localhost:8080/#/) the redirected url ends up looking like

https://localhost:8080/#/access_token=..&token_type=bearer&expires_in=...&scope=...&state=...

And the regex splitter that extracts params (/([^&=]+)=([^&]*)/g) ends up globbing the initial /.

Of course, I can look into removing the / in my code, but I think this is something that may occurr to others and can be avoided altogether (especially since the Error message was completely vague about the error).

I propose submitting a PR to change the current regex tokenizer to

 /[\/]?([^&=]+)=([^&]*)/g

which simply checks for the existence of a possible front slash at the beginning of any parameter.

Seebiscuit commented 6 years ago

After writing this comment I noticed this inside Authenticator.extractParams

if (matchParts[1] === '/state') {
  matchParts[1] = matchParts[1].replace('/', '');
}

Looks like state might've been at the top of the param list in other runs.

The regex I propose above would generalize this exact case.

An edge case it creates is that if any param is supposed to start with a / it will only match the part of the param name w/o the slash. But I'm not sure prop names starting with / are allowed anyhow