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 most Redis commands when subscribed to PubSub channel #219

Open
414owen opened this issue Jan 16, 2024 · 3 comments
Open

Prevent most Redis commands when subscribed to PubSub channel #219

414owen opened this issue Jan 16, 2024 · 3 comments

Comments

@414owen
Copy link

414owen commented Jan 16, 2024

I spent a long time tracking down a heisenbug recently, that I'm fairly confident comes down to the fact that you're not allowed to use most redis commands if you're subscribed to a pub/sub channel.

The docs say:

Once the client enters the subscribed state it is not supposed to issue any other commands, except for additional SUBSCRIBE, SSUBSCRIBE, PSUBSCRIBE, UNSUBSCRIBE, SUNSUBSCRIBE, PUNSUBSCRIBE, PING, RESET and QUIT commands. However, if RESP3 is used (see HELLO) it is possible for a client to issue any commands while in subscribed state.

Even if it didn't cause my specific bug, I think that it would be great for hedis to prevent this misuse of redis, preferably in the type system.

This limitation no longer applies to RESP3.

@qnikst
Copy link
Collaborator

qnikst commented Jan 29, 2024

Hello, thanks! I think we can do it, for example by introducing RedisPubSub context and using it instead of Redis, then we can allow only the allowed functions inside.
First of all let me thank you for rising this issue!

It will change the API, but we next version will be a major one anyways, so it should not be a problem.

At this point I don't see any potential drawback in this solution.

Just a question, would you like to lead this work? I'm not sure how much time will I able to spend on this, also I don't have a good use-case scenario where I can test the approach and I'm afraid that small tests and artificial cases may not work well to validate the sanity of the implementation. I not I'll try to spend some time on a weekend.

@414owen
Copy link
Author

414owen commented Jan 30, 2024

Yes, I'm happy to lead the work on this.
I'm already in the process of implementing it, as part of my RESP3 work.
With RESP3, one can run both pub/sub and req/reply over some types of connections...
My idea was to parameterize the operations one can perform over the connection type. (tbd)

One should be able to run redis request/reply commands over a pipelined connection, but not pub/sub.
One should be able to run both pub/sub and req/rep over a non-pipelined connection.
One should be able to run both pub/sub and req/rep over a combination of one pipelined connection and one not pipelined connection.

Adding the different types of connections is a bit unfortunate, but I think all the variations have a genuine use case.

@qnikst
Copy link
Collaborator

qnikst commented Jan 31, 2024

While I find the work magnificent, I'm not very easy on the massive change to the types in the library, so I want to spend some amount of time on understanding if this is necessary or if we can find some middle ground.

Unfortunately I'll do it only on weekend at best, so please sorry me for not spending as much attention to your work as it deserves.

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

No branches or pull requests

2 participants