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

Update Factory.php to fix issue #62 #63

Closed
wants to merge 1 commit into from

Conversation

bennnjamin
Copy link

@bennnjamin bennnjamin commented Aug 20, 2020

Fixed authentication issue when username/password has special characters, see issue #62

Fixed authentication issue when username/password has special characters
@clue clue added this to the v1.1.0 milestone Aug 20, 2020
Copy link
Owner

@clue clue left a comment

Choose a reason for hiding this comment

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

@bennnjamin Thanks for looking into this, I can confirm this looks like a bug! 👍

Can you update this with a test case to ensure we do not introduce any regressions in the future?

It also looks like this should use rawurldecode(), but this should also be covered by some tests.

@bennnjamin
Copy link
Author

You mean to use both rawurlencode() and rawurldecode()? I was not aware of this function but yes it looks like the more correct one. I'm not not sure where to start writing tests. Can you point me to an existing test for logging onto AMI and maybe I can add some additional cases for special usernames and passwords?

@clue clue removed this from the v1.1.0 milestone Oct 9, 2020
@clue
Copy link
Owner

clue commented Oct 9, 2020

@bennnjamin Thanks for looking into this, I've just filed #66 to support authentication with URL-encoded special characters. The code change looks very similar to your suggestion, most of the work was indeed adding some tests to verify this works as expected. Thanks for sparking this idea!

@clue clue closed this Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants