Backend redis improvements #1451
Conversation
… and improved row operations
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (value === undefined) { | ||
| throw new Error('Field "value" is required for setting Redis string'); | ||
| } | ||
| await redisClient.set(metadata.redisKey, String(value)); |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
To fix this issue, user-controlled values used to construct Redis key names must be strictly validated before being used. Accept only key names (from tableName) that match an allowlist of valid Redis keys or enforce a safe naming policy (e.g., only alphanumeric keys with restricted length, disallowing special characters). If your app cannot know all possible keys in advance, then at least strictly validate them and avoid privileged internal keys. This fix should occur in (or immediately before) the parseTableName method in shared-code/src/data-access-layer/data-access-objects/data-access-object-redis.ts. Specifically:
- Add a validation function (e.g.,
isSafeRedisKey) to check that the extracted key (match[2]ortableNamefor prefixed keys) only contains safe characters. - Reject or sanitize any unsafe values.
This will ensure that only safe keys can be constructed and used in Redis queries.
| @@ -1315,19 +1315,28 @@ | ||
| } | ||
|
|
||
| private parseTableName(tableName: string): RedisTableMetadata { | ||
| const isSafeRedisKey = (key: string): boolean => { | ||
| // Only allow alphanumerics, underscores, hyphens, max length 128 | ||
| return /^[a-zA-Z0-9_\-]{1,128}$/.test(key); | ||
| }; | ||
| const standalonePattern = /^\[(list|set|zset|string|hash)\](.+)$/; | ||
| const match = tableName.match(standalonePattern); | ||
|
|
||
| if (match) { | ||
| const type = match[1] as 'list' | 'set' | 'zset' | 'string' | 'hash'; | ||
| const redisKey = match[2]; | ||
| if (!isSafeRedisKey(redisKey)) { | ||
| throw new Error('Invalid or unsafe Redis key supplied: ' + redisKey); | ||
| } | ||
| return { | ||
| tableName, | ||
| tableType: RedisTableType[type.toUpperCase() as keyof typeof RedisTableType], | ||
| redisKey, | ||
| }; | ||
| } | ||
|
|
||
| if (!isSafeRedisKey(tableName)) { | ||
| throw new Error('Invalid or unsafe Redis key supplied: ' + tableName); | ||
| } | ||
| return { | ||
| tableName, | ||
| tableType: RedisTableType.PREFIXED_KEYS, |
| if (newValue === undefined) { | ||
| throw new Error('Field "value" is required for updating Redis string'); | ||
| } | ||
| await redisClient.set(metadata.redisKey, String(newValue)); |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
The problem is that tableName—which is ultimately user-controlled—is used to construct metadata.redisKey, which is then used directly as a Redis key argument in commands like set, lSet, etc., with no validation or restriction. The recommended fix is to validate that tableName corresponds only to known, expected tables, and/or sanitize it so it cannot cause key collisions or arbitrary access.
Single best fix:
- Whitelist
tableNameagainst an allowed set of table names/keys before using it to constructmetadata.redisKey, or ensure that the parsed key is not allowing arbitrary Redis key manipulation. - If a whitelist is unavailable (dynamic tables), at least check that
tableNameis a string and matches a safe expected pattern. - In the region where
parseTableNameis called (and/or before usingmetadata.redisKey), validate/sanitizetableNameto ensure it cannot be used to access arbitrary Redis keys. - Add a function, e.g.
isSafeRedisKey(tableName: string): boolean, that enforces pattern/whitelist checking and call it before proceeding. If not valid, throw an error.
File/region to change:
Edit the updateRowInTable method and the updateRowInStandaloneTable method in shared-code/src/data-access-layer/data-access-objects/data-access-object-redis.ts to validate tableName. Also, add a helper to enforce key safety.
Required imports/methods:
Add helper method in the file to check safety, no new external dependencies required.
| @@ -999,11 +999,25 @@ | ||
| } | ||
| } | ||
|
|
||
| private isSafeRedisKey(tableName: string): boolean { | ||
| // Accept pattern: [type]keyname or keyname; disallow control chars/whitespace | ||
| // Adjust this according to your deployment needs! | ||
| // For example, allow only a-z A-Z 0-9 _ - . : and brackets for [type] format | ||
| return ( | ||
| typeof tableName === 'string' && | ||
| tableName.length > 0 && | ||
| /^[\w\-\.:\[\]]+$/.test(tableName) | ||
| ); | ||
| } | ||
|
|
||
| public async updateRowInTable( | ||
| tableName: string, | ||
| row: Record<string, unknown>, | ||
| primaryKey: Record<string, unknown>, | ||
| ): Promise<Record<string, unknown>> { | ||
| if (!this.isSafeRedisKey(tableName)) { | ||
| throw new Error('Unsafe tableName specified'); | ||
| } | ||
| const tableMetadata = this.parseTableName(tableName); | ||
|
|
||
| if (tableMetadata.tableType !== RedisTableType.PREFIXED_KEYS) { | ||
| @@ -1018,6 +1027,9 @@ | ||
| row: Record<string, unknown>, | ||
| primaryKey: Record<string, unknown>, | ||
| ): Promise<Record<string, unknown>> { | ||
| if (!this.isSafeRedisKey(metadata.redisKey)) { | ||
| throw new Error('Unsafe Redis key specified'); | ||
| } | ||
| const redisClient = await this.getClient(); | ||
|
|
||
| switch (metadata.tableType) { |
There was a problem hiding this comment.
Pull request overview
This PR enhances Redis backend support by adding native handling for Redis data structures (lists, sets, sorted sets, strings, and hashes) as standalone tables, in addition to the existing prefixed-key pattern.
Key Changes
- Introduced
RedisTableTypeenum andRedisTableMetadatainterface to distinguish between different Redis data structure types - Refactored CRUD operations (
addRowInTable,deleteRowInTable,getRowByPrimaryKey,updateRowInTable,getRowsFromTable) to support both prefixed-key tables and standalone Redis data structures - Improved JSON parsing robustness to handle non-object values (arrays, primitives) by wrapping them in
{key, value}structures
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case RedisTableType.HASH: { | ||
| const field = primaryKey.field; | ||
| if (field) { | ||
| await redisClient.hDel(metadata.redisKey, String(field)); | ||
| } else { | ||
| await redisClient.del(metadata.redisKey); | ||
| } | ||
| return primaryKey; | ||
| } |
There was a problem hiding this comment.
Inconsistent HASH primary key handling: getTablePrimaryColumns returns the first field name from the structure (e.g., 'name'), but deleteRowFromStandaloneTable checks for primaryKey.field. This means if you try to delete a row using the declared primary key column, it will delete the entire hash instead of the specific field. The field name should match between the two methods, or the delete logic should check for the actual primary key field name rather than hardcoding 'field'.
| } | ||
|
|
There was a problem hiding this comment.
Missing validation for HASH table type: The validateSettings method doesn't include a case for RedisTableType.HASH to prevent excluding the primary key field. This inconsistency means HASH tables won't validate if users try to exclude required fields, unlike the other Redis table types.
| } | |
| case RedisTableType.HASH: | |
| if (settings.excluded_fields?.includes('key')) { | |
| errors.push('Cannot exclude the "key" field in Redis hash tables'); | |
| } | |
| break; |
No description provided.