feat: add Telegram user/group access control#117
feat: add Telegram user/group access control#117jjacke13 wants to merge 3 commits intomemovai:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Telegram allowed-users whitelist: config macros and example secret, NVS-backed storage and parser, runtime setter API, enforcement in update processing, and a serial CLI command to set and display allowed user IDs. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Serial CLI
participant Telegram as Telegram Bot
participant NVS as NVS Storage
participant Processor as Message Processor
CLI->>Telegram: telegram_set_allowed_users("id1,id2")
Telegram->>NVS: store "id1,id2" under allowed_users key
NVS-->>Telegram: ack
Telegram->>Telegram: parse_allowed_users() -> update s_allowed_users
Telegram-->>CLI: return success
Note over Processor,Telegram: Later, when updates arrive
Processor->>Telegram: deliver update (sender/chat ID)
Telegram->>Telegram: is_id_allowed(id)
alt allowed
Telegram->>Processor: allow processing
else not allowed
Telegram->>Processor: ignore update / log warning
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/cli/serial_cli.c`:
- Around line 273-275: The call to
telegram_set_allowed_users(allowed_users_args.user_ids->sval[0]) currently
ignores its return value and always prints "Allowed users set."; update the code
to capture the function's return (e.g., result or rc), check for failure, and
only print success when it returns success; on failure log or fprintf an error
with the provided input value and return a non-zero error code so
callers/operators see the failure; reference telegram_set_allowed_users and the
input allowed_users_args.user_ids->sval[0] when adding the check.
In `@main/telegram/telegram_bot.c`:
- Around line 71-76: The current check in the nvs load path ignores an
explicitly stored empty allowlist because of the "&& buf[0]" guard; change the
logic so that a successful nvs_get_str(nvs, MIMI_NVS_KEY_ALLOWED_USERS, buf,
&len) == ESP_OK is treated as "value present" even if buf is empty, call
parse_allowed_users(buf) (allowing it to interpret empty as "allow all"), close
nvs and log s_allowed_users_count accordingly (do not fall back to build-time
allowlist when NVS returned an empty string); update both the initial load
branch around nvs_get_str/MIMI_NVS_KEY_ALLOWED_USERS/buf/parse_allowed_users and
the similar fallback block later so NVS-present-but-empty is honored as an
explicit persisted value.
- Around line 35-43: The allowlist access is racy: is_id_allowed reads
s_allowed_users_count and s_allowed_users without synchronization, allowing a
transient auth bypass when another thread sets s_allowed_users_count = 0 or
updates the array. Fix by adding a mutex (e.g., s_allowed_users_mutex) and
ensure all writers and readers of s_allowed_users and s_allowed_users_count
acquire it; specifically modify is_id_allowed to take the lock, perform an
atomic snapshot (copy s_allowed_users_count and the relevant entries into local
variables or a stack buffer), release the lock, then check the snapshot;
likewise protect any code that sets s_allowed_users_count or updates
s_allowed_users so updates and reads cannot interleave.
- Around line 54-59: Update the allowlist parsing to validate each token using
strtoll with an endptr and errno checks: for each tok, call strtoll(tok,
&endptr, 10), ensure endptr != tok (some digits consumed), check errno for
ERANGE and that the parsed uid fits expected range, then only append to
s_allowed_users and increment s_allowed_users_count up to
MIMI_TG_MAX_ALLOWED_USERS; on any malformed token return a suitable esp_err_t
(e.g., ESP_ERR_INVALID_ARG) instead of silently skipping. Change the setter’s
signature from void to esp_err_t (or the public API return type used elsewhere)
and replace ESP_ERROR_CHECK calls in the public API with propagating/returning
the esp_err_t so callers can handle failures; update callers accordingly to
handle the returned errors. Ensure all references to s_allowed_users,
s_allowed_users_count, MIMI_TG_MAX_ALLOWED_USERS, the strtoll call, and
ESP_ERROR_CHECK sites are adjusted to use the new validation and error-return
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37faa0dd-b08c-49bc-b0c2-cd96f1e6534c
📒 Files selected for processing (5)
main/cli/serial_cli.cmain/mimi_config.hmain/mimi_secrets.h.examplemain/telegram/telegram_bot.cmain/telegram/telegram_bot.h
| static bool is_id_allowed(int64_t user_id, int64_t chat_id) | ||
| { | ||
| if (s_allowed_users_count == 0) return true; | ||
| for (size_t i = 0; i < s_allowed_users_count; i++) { | ||
| if (s_allowed_users[i] == user_id) return true; | ||
| if (s_allowed_users[i] == chat_id) return true; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Synchronize allowlist reads/writes to avoid transient auth bypass.
Line 47 sets s_allowed_users_count = 0, while Line 37 interprets 0 as allow-all. Without locking, an update can interleave with Line 428 checks and temporarily allow unauthorized messages.
Suggested fix (atomic snapshot + lock)
+static portMUX_TYPE s_allowed_users_mux = portMUX_INITIALIZER_UNLOCKED;
+
+static void set_allowed_users_snapshot(const int64_t *ids, size_t count)
+{
+ taskENTER_CRITICAL(&s_allowed_users_mux);
+ memset(s_allowed_users, 0, sizeof(s_allowed_users));
+ memcpy(s_allowed_users, ids, count * sizeof(int64_t));
+ s_allowed_users_count = count;
+ taskEXIT_CRITICAL(&s_allowed_users_mux);
+}
+
static bool is_id_allowed(int64_t user_id, int64_t chat_id)
{
- if (s_allowed_users_count == 0) return true;
- for (size_t i = 0; i < s_allowed_users_count; i++) {
- if (s_allowed_users[i] == user_id) return true;
- if (s_allowed_users[i] == chat_id) return true;
+ int64_t ids[MIMI_TG_MAX_ALLOWED_USERS] = {0};
+ size_t count = 0;
+ taskENTER_CRITICAL(&s_allowed_users_mux);
+ count = s_allowed_users_count;
+ memcpy(ids, s_allowed_users, sizeof(ids));
+ taskEXIT_CRITICAL(&s_allowed_users_mux);
+
+ if (count == 0) return true;
+ for (size_t i = 0; i < count; i++) {
+ if (ids[i] == user_id) return true;
+ if (ids[i] == chat_id) return true;
}
return false;
}Also applies to: 45-62, 415-433, 640-641
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/telegram/telegram_bot.c` around lines 35 - 43, The allowlist access is
racy: is_id_allowed reads s_allowed_users_count and s_allowed_users without
synchronization, allowing a transient auth bypass when another thread sets
s_allowed_users_count = 0 or updates the array. Fix by adding a mutex (e.g.,
s_allowed_users_mutex) and ensure all writers and readers of s_allowed_users and
s_allowed_users_count acquire it; specifically modify is_id_allowed to take the
lock, perform an atomic snapshot (copy s_allowed_users_count and the relevant
entries into local variables or a stack buffer), release the lock, then check
the snapshot; likewise protect any code that sets s_allowed_users_count or
updates s_allowed_users so updates and reads cannot interleave.
| char *tok = strtok(buf, ","); | ||
| while (tok && s_allowed_users_count < MIMI_TG_MAX_ALLOWED_USERS) { | ||
| int64_t uid = strtoll(tok, NULL, 10); | ||
| if (uid != 0) { | ||
| s_allowed_users[s_allowed_users_count++] = uid; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find and read the relevant file
find . -name "telegram_bot.c" -path "*/telegram/*"Repository: memovai/mimiclaw
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Read the file to examine the problematic code
wc -l main/telegram/telegram_bot.cRepository: memovai/mimiclaw
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Read lines 40-70 to see the parse_allowed_users function
sed -n '40,70p' main/telegram/telegram_bot.cRepository: memovai/mimiclaw
Length of output: 854
🏁 Script executed:
#!/bin/bash
# Read lines 620-650 to see the telegram_set_allowed_users function and ESP_ERROR_CHECK usage
sed -n '620,650p' main/telegram/telegram_bot.cRepository: memovai/mimiclaw
Length of output: 1017
Validate allowlist input and return errors instead of panicking in setter.
Lines 54-59 silently ignore malformed tokens (strtoll with NULL endpoint means "abc" returns 0, indistinguishable from valid input, and is skipped without error). The function has no way to report parsing failures (void return). Additionally, lines 632-639 use ESP_ERROR_CHECK in a public API returning esp_err_t, which panics instead of propagating errors. This should validate input, reject malformed entries, and return proper error codes from the public API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/telegram/telegram_bot.c` around lines 54 - 59, Update the allowlist
parsing to validate each token using strtoll with an endptr and errno checks:
for each tok, call strtoll(tok, &endptr, 10), ensure endptr != tok (some digits
consumed), check errno for ERANGE and that the parsed uid fits expected range,
then only append to s_allowed_users and increment s_allowed_users_count up to
MIMI_TG_MAX_ALLOWED_USERS; on any malformed token return a suitable esp_err_t
(e.g., ESP_ERR_INVALID_ARG) instead of silently skipping. Change the setter’s
signature from void to esp_err_t (or the public API return type used elsewhere)
and replace ESP_ERROR_CHECK calls in the public API with propagating/returning
the esp_err_t so callers can handle failures; update callers accordingly to
handle the returned errors. Ensure all references to s_allowed_users,
s_allowed_users_count, MIMI_TG_MAX_ALLOWED_USERS, the strtoll call, and
ESP_ERROR_CHECK sites are adjusted to use the new validation and error-return
behavior.
|
Thanks for the review! Pushed fixes for 3 of the 4 findings: #1 Check CLI return value — Fixed. #3 ESP_ERROR_CHECK in public API — Fixed. #4 Empty NVS whitelist — Fixed. Removed the #2 Mutex for allowlist access — Respectfully disagree. The allowlist is written at init (before the polling task starts) or via CLI |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
main/telegram/telegram_bot.c (1)
31-43:⚠️ Potential issue | 🟠 MajorRace condition on allowlist access remains unaddressed.
The
is_id_allowed()function readss_allowed_users_countands_allowed_users[]without synchronization, whileparse_allowed_users()(called fromtelegram_set_allowed_users()) can modify these concurrently from a different task context (CLI task vs. polling task). This can cause:
- Transient auth bypass if
s_allowed_users_countis set to 0 mid-update- Reading partially-updated array entries
Given that this is a security-sensitive access control path, consider adding synchronization.
,
🔒 Suggested fix using critical section
+static portMUX_TYPE s_allowed_mux = portMUX_INITIALIZER_UNLOCKED; + static bool is_id_allowed(int64_t user_id, int64_t chat_id) { - if (s_allowed_users_count == 0) return true; - for (size_t i = 0; i < s_allowed_users_count; i++) { - if (s_allowed_users[i] == user_id) return true; - if (s_allowed_users[i] == chat_id) return true; + taskENTER_CRITICAL(&s_allowed_mux); + size_t count = s_allowed_users_count; + int64_t ids[MIMI_TG_MAX_ALLOWED_USERS]; + memcpy(ids, s_allowed_users, sizeof(ids)); + taskEXIT_CRITICAL(&s_allowed_mux); + + if (count == 0) return true; + for (size_t i = 0; i < count; i++) { + if (ids[i] == user_id) return true; + if (ids[i] == chat_id) return true; } return false; }And wrap the writes in
parse_allowed_users():static void parse_allowed_users(const char *str) { - s_allowed_users_count = 0; + int64_t tmp_ids[MIMI_TG_MAX_ALLOWED_USERS] = {0}; + size_t tmp_count = 0; if (!str || !str[0]) return; // ... parsing into tmp_ids/tmp_count ... + taskENTER_CRITICAL(&s_allowed_mux); + memcpy(s_allowed_users, tmp_ids, sizeof(s_allowed_users)); + s_allowed_users_count = tmp_count; + taskEXIT_CRITICAL(&s_allowed_mux); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/telegram/telegram_bot.c` around lines 31 - 43, The access-control globals s_allowed_users and s_allowed_users_count are read in is_id_allowed() without synchronization while parse_allowed_users() (via telegram_set_allowed_users()) may write them concurrently, creating a race that can lead to transient bypasses or partial reads; fix by protecting all accesses (reads and writes) to s_allowed_users and s_allowed_users_count with a single synchronization primitive (e.g., a mutex, spinlock, or critical section) and hold it in is_id_allowed() while iterating the array and in parse_allowed_users()/telegram_set_allowed_users() while modifying the array or count so updates are atomic and readers cannot see intermediate state.
🧹 Nitpick comments (1)
main/telegram/telegram_bot.c (1)
648-659: Inconsistent error handling betweentelegram_set_tokenandtelegram_set_allowed_users.
telegram_set_allowed_users()now properly returns errors, buttelegram_set_token()still usesESP_ERROR_CHECK()which will panic on NVS failures. Consider aligning the error handling approach for consistency.♻️ Suggested refactor for consistency
esp_err_t telegram_set_token(const char *token) { nvs_handle_t nvs; - ESP_ERROR_CHECK(nvs_open(MIMI_NVS_TG, NVS_READWRITE, &nvs)); - ESP_ERROR_CHECK(nvs_set_str(nvs, MIMI_NVS_KEY_TG_TOKEN, token)); - ESP_ERROR_CHECK(nvs_commit(nvs)); + esp_err_t err = nvs_open(MIMI_NVS_TG, NVS_READWRITE, &nvs); + if (err != ESP_OK) return err; + err = nvs_set_str(nvs, MIMI_NVS_KEY_TG_TOKEN, token); + if (err != ESP_OK) { nvs_close(nvs); return err; } + err = nvs_commit(nvs); nvs_close(nvs); + if (err != ESP_OK) return err; strncpy(s_bot_token, token, sizeof(s_bot_token) - 1); ESP_LOGI(TAG, "Telegram bot token saved"); return ESP_OK; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/telegram/telegram_bot.c` around lines 648 - 659, telegram_set_token currently uses ESP_ERROR_CHECK (which aborts on failure) while telegram_set_allowed_users returns esp_err_t; change telegram_set_token to mirror telegram_set_allowed_users by removing ESP_ERROR_CHECK calls and instead capture and return esp_err_t from nvs_open, nvs_set_str, and nvs_commit; ensure nvs_close is called on error paths, only copy the token into s_bot_token with strncpy after successful commit, log failures with ESP_LOGE including the esp_err_t, and return the error code rather than aborting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/telegram/telegram_bot.c`:
- Around line 45-62: The parse_allowed_users function silently drops IDs beyond
MIMI_TG_MAX_ALLOWED_USERS; update parse_allowed_users to detect when more tokens
are present after filling s_allowed_users up to MIMI_TG_MAX_ALLOWED_USERS and
emit a clear warning (e.g. via the program's logging facility or stderr) that N
entries were ignored, and include which limit was hit; keep using
s_allowed_users_count and s_allowed_users for storage and ensure the function
still bounds copies into buf safely.
---
Duplicate comments:
In `@main/telegram/telegram_bot.c`:
- Around line 31-43: The access-control globals s_allowed_users and
s_allowed_users_count are read in is_id_allowed() without synchronization while
parse_allowed_users() (via telegram_set_allowed_users()) may write them
concurrently, creating a race that can lead to transient bypasses or partial
reads; fix by protecting all accesses (reads and writes) to s_allowed_users and
s_allowed_users_count with a single synchronization primitive (e.g., a mutex,
spinlock, or critical section) and hold it in is_id_allowed() while iterating
the array and in parse_allowed_users()/telegram_set_allowed_users() while
modifying the array or count so updates are atomic and readers cannot see
intermediate state.
---
Nitpick comments:
In `@main/telegram/telegram_bot.c`:
- Around line 648-659: telegram_set_token currently uses ESP_ERROR_CHECK (which
aborts on failure) while telegram_set_allowed_users returns esp_err_t; change
telegram_set_token to mirror telegram_set_allowed_users by removing
ESP_ERROR_CHECK calls and instead capture and return esp_err_t from nvs_open,
nvs_set_str, and nvs_commit; ensure nvs_close is called on error paths, only
copy the token into s_bot_token with strncpy after successful commit, log
failures with ESP_LOGE including the esp_err_t, and return the error code rather
than aborting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28c5d805-ac7a-4aca-b0eb-e13d49bf0238
📒 Files selected for processing (2)
main/cli/serial_cli.cmain/telegram/telegram_bot.c
| static void parse_allowed_users(const char *str) | ||
| { | ||
| s_allowed_users_count = 0; | ||
| if (!str || !str[0]) return; | ||
|
|
||
| char buf[256]; | ||
| strncpy(buf, str, sizeof(buf) - 1); | ||
| buf[sizeof(buf) - 1] = '\0'; | ||
|
|
||
| char *tok = strtok(buf, ","); | ||
| while (tok && s_allowed_users_count < MIMI_TG_MAX_ALLOWED_USERS) { | ||
| int64_t uid = strtoll(tok, NULL, 10); | ||
| if (uid != 0) { | ||
| s_allowed_users[s_allowed_users_count++] = uid; | ||
| } | ||
| tok = strtok(NULL, ","); | ||
| } | ||
| } |
There was a problem hiding this comment.
Silent truncation when exceeding max allowed users.
If more than MIMI_TG_MAX_ALLOWED_USERS (8) IDs are provided, entries beyond the limit are silently discarded. Operators may not realize their full whitelist wasn't applied.
💡 Suggested improvement
char *tok = strtok(buf, ",");
while (tok && s_allowed_users_count < MIMI_TG_MAX_ALLOWED_USERS) {
int64_t uid = strtoll(tok, NULL, 10);
if (uid != 0) {
s_allowed_users[s_allowed_users_count++] = uid;
}
tok = strtok(NULL, ",");
}
+ if (tok) {
+ ESP_LOGW(TAG, "Allowed users list truncated at %d entries", MIMI_TG_MAX_ALLOWED_USERS);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/telegram/telegram_bot.c` around lines 45 - 62, The parse_allowed_users
function silently drops IDs beyond MIMI_TG_MAX_ALLOWED_USERS; update
parse_allowed_users to detect when more tokens are present after filling
s_allowed_users up to MIMI_TG_MAX_ALLOWED_USERS and emit a clear warning (e.g.
via the program's logging facility or stderr) that N entries were ignored, and
include which limit was hit; keep using s_allowed_users_count and
s_allowed_users for storage and ensure the function still bounds copies into buf
safely.
jjacke13
force-pushed
the
feat/telegram-user-filter
branch
from
27d1406 to
57d4985
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/mimi_secrets.h.example`:
- Around line 34-35: Restore the missing example macro by adding a
MIMI_SECRET_SEARCH_KEY definition next to the existing MIMI_SECRET_TAVILY_KEY in
the mimi_secrets.h.example so users see the Brave Search API option; ensure the
macro name exactly matches MIMI_SECRET_SEARCH_KEY (used in tool_web_search.c and
serial_cli.c) and provide an empty string value like the Tavily entry to mirror
the intended example configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93d8ebc4-b853-4320-8d44-f768a292f54d
📒 Files selected for processing (5)
main/channels/telegram/telegram_bot.cmain/channels/telegram/telegram_bot.hmain/cli/serial_cli.cmain/mimi_config.hmain/mimi_secrets.h.example
🚧 Files skipped from review as they are similar to previous changes (1)
- main/mimi_config.h
Restrict the Telegram bot to specific users and/or group chats via a configurable whitelist. Positive IDs filter by sender (message.from.id), negative IDs allow all users in that group chat (message.chat.id). - Build-time: MIMI_SECRET_ALLOWED_USERS in mimi_secrets.h - Runtime: NVS via `set_allowed_users` CLI command (applies immediately) - Empty whitelist = allow all (backward compatible) - Up to 8 entries, loaded with NVS > build-time fallback Related: memovai#57 — this implementation extends the single-owner approach with multi-user support, group chat filtering, and runtime configuration.
- Honor empty NVS whitelist as explicit "allow all" (remove && buf[0] guard) - Replace ESP_ERROR_CHECK with proper error propagation in setter - Check and surface return value in CLI command
The Brave Search key macro was accidentally removed during rebase. Restored it alongside the Tavily key under a unified "Web Search API" section.
jjacke13
force-pushed
the
feat/telegram-user-filter
branch
from
f84a9ce to
3222f90
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main/cli/serial_cli.c (1)
522-544:⚠️ Potential issue | 🟠 MajorTreat an empty NVS value as a real override for
Allowed IDs.
set_allowed_users ""is a documented runtime state, butprint_config()only treats non-empty NVS strings as present. If a build-time allowlist exists,config_showwill still print the build value even though the active config is the empty NVS override. This call needs a helper path that preserves empty-string values forMIMI_NVS_KEY_ALLOWED_USERS.Also applies to: 592-592
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/cli/serial_cli.c` around lines 522 - 544, The print_config function currently treats only non-empty NVS strings as overrides; change the NVS-detection logic so a successful nvs_get_str() is considered an override even if the returned string is empty when the key is MIMI_NVS_KEY_ALLOWED_USERS. Concretely, in print_config replace the condition `if (nvs_get_str(...) == ESP_OK && nvs_val[0])` with a branch that on ESP_OK sets source="NVS" and display=nvs_val when either nvs_val[0] is non-zero OR strcmp(key, MIMI_NVS_KEY_ALLOWED_USERS) == 0; keep the existing nvs_close and the subsequent fallback to build_val. Apply the same change where the same detection pattern is used for Allowed IDs elsewhere in the file.
♻️ Duplicate comments (2)
main/channels/telegram/telegram_bot.c (2)
35-37:⚠️ Potential issue | 🟠 MajorReject malformed and over-limit allowlists before saving them.
strtoll(tok, NULL, 10)will accept partial tokens like123abc, and a fully invalid value likeabcleavess_allowed_users_count == 0, which this file interprets as allow-all. IDs pastMIMI_TG_MAX_ALLOWED_USERSare also silently dropped even though the full raw string is still persisted. Validate into a temporary list first, return an error on malformed/overflowing/extra tokens, and only write to NVS after the parse succeeds.In ISO C / POSIX, how does `strtoll` behave for "abc", "123abc", and overflowed values when `endptr` is NULL, and what checks are required to reject malformed numeric input?Also applies to: 45-62, 632-645
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/channels/telegram/telegram_bot.c` around lines 35 - 37, The parsing logic for the Telegram allowlist is accepting malformed/overflowed tokens and treating a failed parse as an empty list (allow-all); fix this by parsing into a temporary array first and performing strict validation: when calling strtoll use an endptr and check that *endptr points to only whitespace or '\0', check errno for ERANGE to detect overflow, ensure the parsed count does not exceed MIMI_TG_MAX_ALLOWED_USERS, and reject the entire input on any malformed/overflow/extra-token condition; only on successful validation assign the temporary list to s_allowed_users and update s_allowed_users_count and NVS storage (affects is_id_allowed, the allowlist parsing/writing code referenced by s_allowed_users_count and MIMI_TG_MAX_ALLOWED_USERS).
35-37:⚠️ Potential issue | 🔴 CriticalPublish allowlist updates atomically.
parse_allowed_users()clearss_allowed_users_countbefore repopulating it, andis_id_allowed()treats0as allow-all. During a runtime update, the polling task can observe that transient state and admit unauthorized messages. Build the new list in locals and swap the array/count under a lock or critical section.Also applies to: 45-47, 632-644
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/channels/telegram/telegram_bot.c` around lines 35 - 37, The allowlist update is not atomic: parse_allowed_users() zeroes s_allowed_users_count then repopulates, and is_id_allowed() treats count==0 as allow-all, creating a window that admits unauthorized messages; fix by building the new allowed IDs array and new_count in local stack/heap variables inside parse_allowed_users(), then lock the shared mutex (or enter the existing critical section) and atomically swap pointers/counts by assigning s_allowed_users and s_allowed_users_count to the new values before releasing the lock, freeing the old array after the swap; apply the same swap-under-lock approach wherever the code mutates s_allowed_users/s_allowed_users_count (parse_allowed_users, and the other update locations referenced alongside is_id_allowed) to ensure no transient allow-all state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@main/cli/serial_cli.c`:
- Around line 522-544: The print_config function currently treats only non-empty
NVS strings as overrides; change the NVS-detection logic so a successful
nvs_get_str() is considered an override even if the returned string is empty
when the key is MIMI_NVS_KEY_ALLOWED_USERS. Concretely, in print_config replace
the condition `if (nvs_get_str(...) == ESP_OK && nvs_val[0])` with a branch that
on ESP_OK sets source="NVS" and display=nvs_val when either nvs_val[0] is
non-zero OR strcmp(key, MIMI_NVS_KEY_ALLOWED_USERS) == 0; keep the existing
nvs_close and the subsequent fallback to build_val. Apply the same change where
the same detection pattern is used for Allowed IDs elsewhere in the file.
---
Duplicate comments:
In `@main/channels/telegram/telegram_bot.c`:
- Around line 35-37: The parsing logic for the Telegram allowlist is accepting
malformed/overflowed tokens and treating a failed parse as an empty list
(allow-all); fix this by parsing into a temporary array first and performing
strict validation: when calling strtoll use an endptr and check that *endptr
points to only whitespace or '\0', check errno for ERANGE to detect overflow,
ensure the parsed count does not exceed MIMI_TG_MAX_ALLOWED_USERS, and reject
the entire input on any malformed/overflow/extra-token condition; only on
successful validation assign the temporary list to s_allowed_users and update
s_allowed_users_count and NVS storage (affects is_id_allowed, the allowlist
parsing/writing code referenced by s_allowed_users_count and
MIMI_TG_MAX_ALLOWED_USERS).
- Around line 35-37: The allowlist update is not atomic: parse_allowed_users()
zeroes s_allowed_users_count then repopulates, and is_id_allowed() treats
count==0 as allow-all, creating a window that admits unauthorized messages; fix
by building the new allowed IDs array and new_count in local stack/heap
variables inside parse_allowed_users(), then lock the shared mutex (or enter the
existing critical section) and atomically swap pointers/counts by assigning
s_allowed_users and s_allowed_users_count to the new values before releasing the
lock, freeing the old array after the swap; apply the same swap-under-lock
approach wherever the code mutates s_allowed_users/s_allowed_users_count
(parse_allowed_users, and the other update locations referenced alongside
is_id_allowed) to ensure no transient allow-all state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f9eb68a-6ec0-47c9-8543-b98377088586
📒 Files selected for processing (5)
main/channels/telegram/telegram_bot.cmain/channels/telegram/telegram_bot.hmain/cli/serial_cli.cmain/mimi_config.hmain/mimi_secrets.h.example
2 participants
Summary
Restrict the Telegram bot to specific users and/or group chats via a configurable whitelist.
123456789) filter bymessage.from.id— allow this user in any chat-1001234567890) filter bymessage.chat.id— allow all users in that groupConfiguration
Build-time (
mimi_secrets.h):Runtime (CLI, applies immediately without restart):
View current config:
Implementation
tg_config/allowed_users), with build-time fallbackMIMI_TG_MAX_ALLOWED_USERS)process_updates()after dedup, before message dispatchtelegram_set_allowed_users()public API for runtime updatesFiles changed
main/mimi_config.hMIMI_SECRET_ALLOWED_USERS,MIMI_TG_MAX_ALLOWED_USERS,MIMI_NVS_KEY_ALLOWED_USERSmain/mimi_secrets.h.exampleMIMI_SECRET_ALLOWED_USERStemplatemain/telegram/telegram_bot.cmain/telegram/telegram_bot.htelegram_set_allowed_users()main/cli/serial_cli.cset_allowed_userscommand +config_showentryRelated
Extends the approach in #57 with:
message.from.id(correct for group chats wherechat.idis the group, not the sender)Summary by CodeRabbit
New Features
Documentation