Race condition in Redis#ReadAndDestroy

[dillonstreator]

There is a race condition in the Redis ReadAndDestroy implementation that can allow multiple clients to retrieve the same secret value. This happens because the 'read and destroy' operation is not currently atomic. Redis Lua scripts do execute in an atomic fashion and and would be a good use case for this operation and solve for the race condition. https://redis.io/docs/latest/develop/interact/programmability/eval-intro/

ots/pkg/storage/redis/redis.go

Lines 79 to 93 in 8fadf72

	func (s storageRedis) ReadAndDestroy(id string) (string, error) {
	secret, err := s.conn.Get(context.Background(), s.redisKey(id)).Result()
	if err != nil {
	if errors.Is(err, redis.Nil) {
	return "", storage.ErrSecretNotFound
	}
	return "", fmt.Errorf("getting key: %w", err)
	}
	err = s.conn.Del(context.Background(), s.redisKey(id)).Err()
	if err != nil {
	return secret, fmt.Errorf("deleting key: %w", err)
	}
	return secret, nil
	}

[Luzifer]

I'm not sure if it's worth the investment in time and complexity, as to trigger this race-condition you would need to have a sub-second precision in requesting the same secret to trigger this. The two requests need to be virtually at the same point of time, before the first request can delete the secret.

Looking at my hosted instance, the request to retrieve a secret typically takes between 2 and 4 milliseconds, so you would have a less-than-2ms-window to trigger this.

IMHO this is nothing we need to handle.

[dillonstreator]

Hi @Luzifer, thanks for reviewing this issue.

I want to emphasize how critical this is from the user’s perspective: because the read and delete aren’t atomic, two parallel requests can both fetch the same secret—yet the successful client call gives no indication that it wasn’t the only one and that the secret is compromised. There is no way to guarantee the secret is not compromised with the current system. I understand that this is an corner case but it's a flawed system design that erodes trust in the 'read once' expectation.

Would it be possible to:
1. Document this limitation clearly in the README, so users understand the risk?
2. Consider a longer-term fix—such as moving the read-and-delete logic into a Redis Lua script, making it truly atomic?

This could be crucial for users relying on single-read guarantees in highly sensitive environments. Thank you for considering it!

FWIW - I have a similar project (crypt.fyi) that uses a Lua script for atomic read/delete that you'd be able to draw inspiration from.