search

marcoCasamento / Hangfire.Redis.StackExchange

HangFire Redis storage based on original (and now unsupported) Hangfire.Redis but using lovely StackExchange.Redis client
Other
448 stars 108 forks source link

Consider not using the ConnectionString on .ToString() #115

Closed danspark closed 1 year ago

danspark commented 2 years ago

This causes the full connection string to be logged when the server is being started:

Starting Hangfire Server using job storage: 'redis://host.docker.internal:6379/0'

If there is a password in the connection string, it is not filtered and it will be logged.

I don't know if there's a reason for logging it, but if there isn't, another field can be used, or the password can be filtered out.

marcoCasamento commented 2 years ago

I agree. What about something like String.Join(',', cnnString.Split(',').Where(x=> !x.Contains("password", StringComparison.InvariantCultureIgnoreCase))) ?

danspark commented 2 years ago

https://github.com/marcoCasamento/Hangfire.Redis.StackExchange/blob/7019ede12837a89ee32ebf2e3e4ee65bc4341730/Hangfire.Redis.StackExchange/RedisStorage.cs#L55

The configuration is parsed when the storage is constructed, maybe it would be better to store it and replace ConnectionString with redisOptions.ToString(includePassword: false).

Since you agree, I'm opening a PR with that change.

erikhajsakvojtko commented 1 year ago

Any progress on this issue? @danspark I can't seem to find the mentioned PR.

matheusriboli commented 1 year ago

Hello @marcoCasamento, I opened a PR fixing this issue. Could you review it? https://github.com/marcoCasamento/Hangfire.Redis.StackExchange/pull/116

glen-84 commented 1 year ago

@marcoCasamento Should this issue be closed now?

marcoCasamento commented 1 year ago

Absolutely. It has been merged and as far as I remember, tested.