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

[9.x] Ignore empty redis username string in PhpRedisConnector #41773

Merged
merged 2 commits into from
Apr 1, 2022

Conversation

seanmtaylor
Copy link
Contributor

@seanmtaylor seanmtaylor commented Apr 1, 2022

A username option was added to the PhpRedisConnector in #41683.

However, some services, such as Heroku, produce a REDIS_URL connection string that has an empty username part.

This is causing the following exception:

RedisException: WRONGPASS invalid username-password pair or user is disabled.

So given the following REDIS_URL format:

redis://username:password@host:port

The following url string, with a blank username, will thrown an error.

redis://:abc123@redishost.example:12345

Solution

I have added an extra check to ensure that the provided username isn't an empty string before attempting to use it.

Considerations

I have used a $config['username'] !== '' check rather than ! empty($config['username']) in the small off chance that the username value is falsey (eg 0).

You might just say why not update the REDIS_URL and replace the empty username part with null.

That's possible, but the issue is services such as Heroku will generate these REDIS_* variables at any time, such as when upgrading the redis instance, so it would need to be manually changed each time the credentials were generated.

Tests

Sorry, I couldn't see any existing tests for this class so wasn't quite sure where to begin.

Feel free to point me in the right direction if a test is required for this.

@seanmtaylor seanmtaylor marked this pull request as ready for review April 1, 2022 11:07
@taylorotwell taylorotwell merged commit 1d82ec6 into laravel:9.x Apr 1, 2022
@taylorotwell
Copy link
Member

Thanks!

@GrahamCampbell GrahamCampbell changed the title Ignore empty redis username string in PhpRedisConnector [9.x] Ignore empty redis username string in PhpRedisConnector Apr 1, 2022
@dwightwatson
Copy link
Contributor

@seanmtaylor did this fix the issue for you? I just upgraded to v9.7.0 which appears to have this fix but I'm still getting a NOAUTH error on Heroku.

Dropping back to 9.5.1 where this issue wasn't present.

@dwightwatson
Copy link
Contributor

dwightwatson commented Apr 6, 2022

If it's worth noting - I have 2 Redis instances connected to my Laravel app on Heroku. One has no username like above, and the other has the username h. No idea why - perhaps Heroku changed the auth scheme at some point.

However, it worked fine on 9.5.1 and not beyond.

Edit: for anyone else who runs across this - Heroku removed the h username by default for new Redis instances. I resolved this problem by replacing my Redis instances that still used the h username with new ones that had a blank username. Annoying, but I am now able to upgrade past Laravel 9.5.1.

@seanmtaylor seanmtaylor deleted the redis-empty-username branch April 6, 2022 12:38
@seanmtaylor
Copy link
Contributor Author

@seanmtaylor did this fix the issue for you? I just upgraded to v9.7.0 which appears to have this fix but I'm still getting a NOAUTH error on Heroku.

Dropping back to 9.5.1 where this issue wasn't present.

Yeah it has sorted it for me @dwightwatson

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants