[WIP] Introduce __conn_id__ placeholder #321
Conversation
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
|
cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| m_event_base = event_base; | ||
|
|
||
| // Initialize conn_id string and value | ||
| m_conn_id_str = std::to_string(conn_id); |
There was a problem hiding this comment.
Inconsistent conn_id string format between constructors
Medium Severity
The two client constructors initialize m_conn_id_str with different formats. The constructor taking client_group* (line 118) uses "user" + std::to_string(conn_id) producing strings like "user1", while the constructor taking event_base* (line 141) uses just std::to_string(conn_id) producing strings like "1". This causes the __conn_id__ placeholder to produce inconsistent values depending on which code path creates the client (e.g., verify_client uses the second constructor).
Additional Locations (1)
| "$%zu\r\n" | ||
| "%s\r\n", | ||
| user_len, | ||
| (int) user_len, |
There was a problem hiding this comment.
Authentication silently ignores configured username from credentials
High Severity
When users configure authentication with --authenticate user:password format, the username portion is silently ignored and replaced with the conn_id_string (e.g., "user1"). The redis_protocol::authenticate function receives the conn_id as the user parameter and the configured credentials separately, then extracts only the password portion from credentials while discarding the configured username. This breaks backward compatibility - users expecting to authenticate as "myuser" would instead authenticate as "user1".
Additional Locations (1)
| object_generator* m_obj_gen; | ||
| std::string m_conn_id_str; | ||
| const char* m_conn_id_value; | ||
| unsigned int m_conn_id_value_len; |
There was a problem hiding this comment.
Unused member variable m_conn_id_value_len is never read
Low Severity
The member variable m_conn_id_value_len is declared in client.h and assigned in both client constructors, but it is never read or used anywhere in the codebase. This is dead code that adds unnecessary maintenance burden. Either remove it or use it where the connection ID length is needed.
Additional Locations (2)
| } | ||
|
|
||
| private: | ||
| unsigned int m_conn_id; |
There was a problem hiding this comment.
Member variable m_conn_id only used during construction
Low Severity
The member variable m_conn_id in shard_connection is stored as a class member but only used once in the constructor to compute m_conn_id_string. After initialization, m_conn_id is never accessed again. This could be simplified to a local variable in the constructor since the value doesn't need to persist.
Note
Medium Risk
Touches connection setup and authentication formatting across protocols, which can break login flows or connection initialization if
conn_id/user handling is incorrect. Also adds new placeholder substitution logic in arbitrary command generation, affecting request encoding and debug output.Overview
Adds support for a new arbitrary-command placeholder,
__conn_id__, by extendingcommand_arg_type(conn_id_type) and substituting the placeholder at request-build time inclient::create_arbitrary_request.Propagates a per-client connection identifier through
client_group/client/cluster_clientintoshard_connection, and updatesabstract_protocol::authenticate(and implementations) to accept an explicituserparameter so connection setup can authenticate using that per-client id (with updated debug logging and setup flow).Written by Cursor Bugbot for commit b45dbf4. This will update automatically on new commits. Configure here.