Skip to content
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

Prevent nowProvider from being passed to authorize endpoint #840

Merged
merged 3 commits into from
Nov 16, 2021

Conversation

stevehobbsdev
Copy link
Contributor

Changes

This PR prevents nowProvider from being passed through the URL when logging in. The fundamental change is that nowProvider is added to the destructure list in Auth0Client._getParams.

In addition, I've attemped to help prevent this in the future by adding a test that includes all the client options and verifying they're not in the URL. Adding this test flagged up a couple other properties that should have been excluded, so these have also been added.

Also did some refactoring in _getParams to tidy up the code a bit.

References

Fixes #836

Testing

  • This change adds unit test coverage
  • This change adds integration test coverage
  • This change has been tested on the latest version of the platform/language

Checklist

@stevehobbsdev stevehobbsdev added CH: Fixed PR is fixing a bug review:small Small review labels Nov 16, 2021
@stevehobbsdev stevehobbsdev marked this pull request as ready for review November 16, 2021 14:39
@stevehobbsdev stevehobbsdev requested a review from a team as a code owner November 16, 2021 14:39
Comment on lines -36 to -43
const assertUrlEquals = (actualUrl, host, path, queryParams) => {
const url = new URL(actualUrl);
expect(url.host).toEqual(host);
expect(url.pathname).toEqual(path);
for (let [key, value] of Object.entries(queryParams)) {
expect(url.searchParams.get(key)).toEqual(value);
}
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was duplicated from the one in helpers.ts, so I removed it.

Comment on lines +4 to +5
"noImplicitAny": false,
"target": "es6"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

es6 for testing so that I can get access to URLSearchParams.entries(), shouldn't cause any problems with the SDK at runtime?

@stevehobbsdev
Copy link
Contributor Author

Regarding coverage, I've looked at it locally and the level of coverage hasn't changed from master, so I'm not sure why the tool is reporting otherwise.

@stevehobbsdev stevehobbsdev merged commit db178a3 into master Nov 16, 2021
@stevehobbsdev stevehobbsdev deleted the fix/now-provider branch November 16, 2021 16:36
@stevehobbsdev stevehobbsdev mentioned this pull request Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CH: Fixed PR is fixing a bug review:small Small review
Projects
None yet
2 participants