fix(api): harden admin config and secret handling#371
Open
PyRo1121 wants to merge 7 commits intospacedriveapp:mainfrom
Open
fix(api): harden admin config and secret handling#371PyRo1121 wants to merge 7 commits intospacedriveapp:mainfrom
PyRo1121 wants to merge 7 commits intospacedriveapp:mainfrom
Conversation
Contributor
|
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:
WalkthroughAdds an admin helpers module and OAuth JSON helpers; migrates config mutations to use secret-backed references with persistence to a SecretsStore and hot-reload; enforces API auth on sensitive endpoints; threads per-runtime named permission registries through startup and watcher; implements memory deduplication; and updates many API/messaging/provider flows and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
src/api/settings.rs
Outdated
| return Err(StatusCode::INTERNAL_SERVER_ERROR); | ||
| } | ||
| require_api_auth_token(&state).await?; | ||
| let (_config_guard, config_path, _doc) = load_config_doc(&state).await?; |
Contributor
There was a problem hiding this comment.
load_config_doc() parses the existing file; if config.toml is already malformed, this blocks the raw editor from writing a fixed (validated) payload. You can keep the write mutex but skip parsing here.
Suggested change
| let (_config_guard, config_path, _doc) = load_config_doc(&state).await?; | |
| let _config_guard = state.config_write_mutex.lock().await; | |
| let config_path = state.config_path.read().await.clone(); | |
| if config_path.as_os_str().is_empty() { | |
| tracing::error!("config_path not set in ApiState"); | |
| return Err(StatusCode::INTERNAL_SERVER_ERROR); | |
| } |
Comment on lines
+109
to
+114
| let auth_token = state.auth_token.read().await; | ||
| if auth_token.is_some() { | ||
| Ok(()) | ||
| } else { | ||
| Err(StatusCode::FORBIDDEN) | ||
| } |
Contributor
There was a problem hiding this comment.
This treats Some("") / whitespace as configured, which makes the auth-gated endpoints + non-loopback startup checks effectively no-op if someone sets api.auth_token = "". I’d treat empty/whitespace as missing.
Suggested change
| let auth_token = state.auth_token.read().await; | |
| if auth_token.is_some() { | |
| Ok(()) | |
| } else { | |
| Err(StatusCode::FORBIDDEN) | |
| } | |
| let auth_token = state.auth_token.read().await; | |
| if auth_token | |
| .as_deref() | |
| .is_some_and(|token| !token.trim().is_empty()) | |
| { | |
| Ok(()) | |
| } else { | |
| Err(StatusCode::FORBIDDEN) | |
| } |
src/api/admin.rs
Outdated
| state: &Arc<ApiState>, | ||
| config_path: &Path, | ||
| ) -> Result<crate::config::Config, StatusCode> { | ||
| let new_config = crate::config::Config::load_from_path(config_path).map_err(|error| { |
Contributor
There was a problem hiding this comment.
Minor perf nit: Config::load_from_path() does sync fs IO (std::fs::read_to_string). Since this runs in async handlers, wrapping it in tokio::task::spawn_blocking would avoid blocking the runtime under load.
src/memory/maintenance.rs
Outdated
Comment on lines
+204
to
+205
| .then_with(|| right.updated_at.cmp(&left.updated_at)) | ||
| .then_with(|| right.created_at.cmp(&left.created_at)) |
Contributor
There was a problem hiding this comment.
I think these comparisons are flipped for max_by: right.updated_at.cmp(&left.updated_at) makes older timestamps "greater", so it can pick the least-recently-updated memory as canonical.
Suggested change
| .then_with(|| right.updated_at.cmp(&left.updated_at)) | |
| .then_with(|| right.created_at.cmp(&left.created_at)) | |
| .then_with(|| left.updated_at.cmp(&right.updated_at)) | |
| .then_with(|| left.created_at.cmp(&right.created_at)) |
src/oauth_storage.rs
Outdated
Comment on lines
+22
to
+29
| std::fs::write(path, &data).with_context(|| format!("failed to write {}", path.display()))?; | ||
|
|
||
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::fs::PermissionsExt; | ||
| std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)) | ||
| .with_context(|| format!("failed to set permissions on {}", path.display()))?; | ||
| } |
Contributor
There was a problem hiding this comment.
On Unix there’s a small window where the file is written with default perms before set_permissions(0o600) runs. If you want to avoid that for newly-created files, open with mode(0o600) and write through the handle.
Suggested change
| std::fs::write(path, &data).with_context(|| format!("failed to write {}", path.display()))?; | |
| #[cfg(unix)] | |
| { | |
| use std::os::unix::fs::PermissionsExt; | |
| std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)) | |
| .with_context(|| format!("failed to set permissions on {}", path.display()))?; | |
| } | |
| #[cfg(unix)] | |
| { | |
| use std::io::Write as _; | |
| use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; | |
| let mut file = std::fs::OpenOptions::new() | |
| .create(true) | |
| .truncate(true) | |
| .write(true) | |
| .mode(0o600) | |
| .open(path) | |
| .with_context(|| format!("failed to open {}", path.display()))?; | |
| file.write_all(data.as_bytes()) | |
| .with_context(|| format!("failed to write {}", path.display()))?; | |
| std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)) | |
| .with_context(|| format!("failed to set permissions on {}", path.display()))?; | |
| } | |
| #[cfg(not(unix))] | |
| { | |
| std::fs::write(path, &data) | |
| .with_context(|| format!("failed to write {}", path.display()))?; | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/config/watcher.rs (1)
223-243:⚠️ Potential issue | 🟠 MajorRefresh named-instance permissions even when the default adapter is absent.
Each block nests the named-instance reload under
Some(<platform>_permissions). In a named-only setup those globalOptions can beNone, so Lines 231-242, 252-261, 272-283, and 293-302 never run and the per-instanceArcSwaps stop hot-reloading entirely.Also applies to: 245-262, 264-284, 286-303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/watcher.rs` around lines 223 - 243, The per-instance permission refresh is currently nested inside the conditional that requires a default discord_permissions (the `if let Some(ref perms) = discord_permissions && let Some(discord_config) = &config.messaging.discord` block), which prevents named-instance `ArcSwap`s from being updated when the default adapter is absent; relocate the loop that reads `named_discord_permissions` and updates each registry entry using `binding_runtime_adapter_key("discord", Some(instance.name.as_str()))` and `DiscordPermissions::from_instance_config(instance, &config.bindings)` so it runs whenever `discord_config` is present (i.e., outside the `Some(discord_permissions)` guard), keeping the existing update of the global `perms.store(Arc::new(DiscordPermissions::from_config(...)))` where applicable.src/api/messaging.rs (2)
1548-1556:⚠️ Potential issue | 🟠 MajorDon't report success if the post-write reload failed.
If
reload_runtime_configserrors here, the instance is already persisted but the handler still returns success and leavesstate.bindingsstale. This should propagate the reload failure instead of treating it as best-effort. As per coding guidelines, "Don't silently discard errors. Nolet _ =on Results. Handle them, log them, or propagate them."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 1548 - 1556, The current post-write block treats reload_runtime_configs(&state, &config_path).await as best-effort and silently ignores failures; change it so reload_runtime_configs errors are propagated (or explicitly handled) and only update state.bindings after a successful reload. Specifically, replace the if let Ok(new_config) = reload_runtime_configs(...) pattern with a propagation (using ? on the Result) or explicit error handling, and move the bindings_swap.store(std::sync::Arc::new(new_config.bindings.clone())) call so it only runs when reload_runtime_configs returns Ok; ensure no Result is discarded and the function returns the reload failure to the caller instead of reporting success.
1558-1560:⚠️ Potential issue | 🟠 MajorNamed Twitch instances now use two different token-file names.
This path now relies on the watcher to start new adapters, and
src/config/watcher.rsbuilds named Twitch token files astwitch_token_<safe>_<hash>.json. The manual start/delete paths in this file still usetwitch_token_<safe>.json, so re-enable/delete won't reuse or clean up the same credential file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 1558 - 1560, The manual start/re-enable and delete credential paths in src/api/messaging.rs still construct token filenames as twitch_token_<safe>.json while the watcher in src/config/watcher.rs uses twitch_token_<safe>_<hash>.json; update the token-file naming logic used by the manual start/re_enable_adapter and delete_adapter (or equivalent start/delete handlers) to generate and look for twitch_token_{safe}_{hash}.json (matching the watcher’s safe+hash scheme) so re-enable/delete will find and remove the same credential files the watcher creates. Ensure any token-file lookup, creation, and removal code in messaging.rs uses the same safe+hash naming helper/function as watcher.rs.
🧹 Nitpick comments (3)
src/api/secrets.rs (1)
799-804: Exercise the configured-token branch too.This only proves the
FORBIDDENpath. If you also callstate.set_api_auth_token(Some("test".into())).awaitand assert the handler falls through to the next guard (SERVICE_UNAVAILABLEin this harness), you'll verify thatexport_secrets()is actually consulting the live auth token.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/secrets.rs` around lines 799 - 804, The test export_secrets_requires_configured_api_auth only checks the FORBIDDEN branch; update it to also exercise the configured-token branch by calling state.set_api_auth_token(Some("test".into())).await before invoking export_secrets(State(state)), then assert the response status matches the next guard path (SERVICE_UNAVAILABLE in this test harness). This verifies export_secrets actually consults the live auth token; keep the existing FORBIDDEN check and add the additional set_api_auth_token + response assertion in the same test (or a small follow-up test) referencing export_secrets and State(state).src/secrets/store.rs (1)
1103-1119: Add a webhook regression assertion.This registry change affects secret classification, but the nearby tests do not lock in
WEBHOOK_AUTH_TOKEN. A small assertion inauto_categorize_known_patterns()and/orsystem_secret_registry_contains_all_sections()would keep a future removal from silently reclassifying webhook tokens as tool secrets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/secrets/store.rs` around lines 1103 - 1119, Add a regression assertion that ensures the webhook auth token remains classified as a webhook secret: update the tests (preferably in auto_categorize_known_patterns() or system_secret_registry_contains_all_sections()) to assert that "WEBHOOK_AUTH_TOKEN" (or the constant used by WebhookConfig::secret_fields()) is present in the resulting registry/fields after calling the classification code; this locks in the behavior so future changes to DefaultsConfig/LlmConfig/other secret_fields() cannot silently reclassify the webhook token as a tool/internal secret.src/main.rs (1)
1509-1516: Consider using the type aliases fromwatcher.rsfor readability.The type aliases
NamedDiscordPermissions,NamedSlackPermissions, etc. are defined insrc/config/watcher.rs(lines 23-29) and match these exact types. Reusing them here would reduce verbosity and maintain consistency across the codebase.♻️ Proposed refactor using type aliases
At the top of the file, add imports:
use spacebot::config::{ NamedDiscordPermissions, NamedSlackPermissions, NamedTelegramPermissions, NamedTwitchPermissions };Or if the aliases aren't exported, define local type aliases to match
watcher.rs:+type NamedDiscordPermissions = + Arc<std::sync::RwLock<HashMap<String, Arc<ArcSwap<spacebot::config::DiscordPermissions>>>>>; +// ... similar for other types + let named_discord_permissions = - Arc::new(std::sync::RwLock::new(std::collections::HashMap::new())); + NamedDiscordPermissions::default();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 1509 - 1516, Replace the verbose Arc<RwLock<HashMap<...>>> declarations with the existing type aliases to improve readability: import or reference NamedDiscordPermissions, NamedSlackPermissions, NamedTelegramPermissions, and NamedTwitchPermissions (from config::watcher or re-exported module) and initialize named_discord_permissions, named_slack_permissions, named_telegram_permissions, and named_twitch_permissions using Arc::new(RwLock::new(<alias>::default() or HashMap::new() wrapped appropriately)) so the variables use the aliases instead of explicit Arc<RwLock<HashMap<...>>> types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/admin.rs`:
- Around line 58-95: Multiple call sites are currently discarding the Result
from reload_runtime_configs; update each call to either handle errors locally or
propagate them: replace bare .await calls with either if let Err(e) =
reload_runtime_configs(...).await { tracing::warn!(%e, "reload_runtime_configs
failed for context: {context_info}"); } using relevant context (agent_id,
provider id, or config_path) for the log, or if the caller is an HTTP handler
that returns Result<..., StatusCode> propagate with ? so the error flows to the
client; ensure you reference the reload_runtime_configs(...) invocation and add
a tracing::warn!(%e, ...) or ? as appropriate at each site.
In `@src/api/bindings.rs`:
- Around line 495-503: The handler currently ignores failures from
reload_runtime_configs after write_config_doc (using if let Ok(...)), which can
leave on-disk state inconsistent with in-memory state; change the logic to
propagate the error instead of treating reload as best-effort — call
reload_runtime_configs(&state, &config_path).await and propagate any Err (via ?
or explicit match returning the error) so the handler returns a failure if
reload fails, ensuring write_config_doc, reload_runtime_configs, and any use of
new_config are handled correctly.
In `@src/api/providers.rs`:
- Around line 327-328: The handlers finalize_openai_oauth, update_provider, and
delete_provider currently discard the Config returned by reload_runtime_configs
and either re-read disk or never refresh defaults_config; change each to capture
the returned new_config from reload_runtime_configs(state, &config_path).await
and immediately call
state.set_defaults_config(new_config.defaults.clone()).await instead of doing a
second disk read or skipping the update; apply the same change to the other
occurrences noted (the blocks around the other reload_runtime_configs calls) so
defaults_config is updated from the returned Config in all places.
In `@src/api/server.rs`:
- Around line 40-45: Normalize empty/blank auth tokens to None wherever auth is
read and when configs are reloaded so a blank string doesn't count as
configured; specifically update reload_runtime_configs to convert
Some("")/all-whitespace to None before writing into state.auth_token, and update
the places that read state.auth_token (the auth_token clone used with
should_warn_unprotected_bind and the runtime per-request check) to treat blank
tokens as None (e.g., map/normalize before as_deref()) so
should_warn_unprotected_bind(bind, auth_token.as_deref()) and the per-request
authorization logic never inadvertently accept an empty token and the startup
public-bind guard remains effective if auth is cleared.
In `@src/api/settings.rs`:
- Around line 323-325: The current write_config_doc call is followed by
discarding the Result from reload_runtime_configs (let _ =
reload_runtime_configs(&state, &config_path).await;) which both ignores errors
and fails to update the in-memory defaults cache; change this to capture the
returned Config, propagate any error instead of silencing it, and refresh the
in-memory defaults by calling
state.set_defaults_config(new_config.defaults.clone()).await using the returned
new_config from reload_runtime_configs (refer to reload_runtime_configs,
state.set_defaults_config, and write_config_doc to locate the change).
In `@src/config/types.rs`:
- Around line 2331-2350: The SystemSecrets impl for WebhookConfig was added but
migrate_secrets() still uses a hard-coded list of sections and never invokes
migrate_section_secrets::<WebhookConfig>(), leaving messaging.webhook.auth_token
un-migrated; update migrate_secrets() to include the webhook section by calling
migrate_section_secrets::<WebhookConfig>() (or add "messaging.webhook" to the
existing hard-coded migration flow) so the WebhookConfig secret_fields
(auth_token / WEBHOOK_AUTH_TOKEN) are processed during migration.
In `@src/memory/maintenance.rs`:
- Around line 151-163: The grouping key uses MemoryType in a HashMap key but
MemoryType isn't Hash, so update the MemoryType type's derive list (the
enum/struct that currently derives Debug, Clone, Copy, Serialize, Deserialize,
PartialEq, Eq) to also derive Hash so HashMap<(MemoryType, String), Vec<Memory>>
compiles; locate the MemoryType definition (the type that currently lists those
derives) and add Hash to its derive attributes.
- Around line 197-205: The selector select_canonical_memory prefers older
memories on ties because the updated_at and created_at comparisons are reversed;
change those comparisons so newer timestamps win by comparing
left.updated_at.cmp(&right.updated_at) and
left.created_at.cmp(&right.created_at) (keeping the existing
importance.total_cmp and access_count.cmp ordering) so ties pick the most
recently updated/created memory as canonical.
In `@src/oauth_storage.rs`:
- Around line 19-29: save_json_credentials currently writes the serialized JSON
via std::fs::write and then calls set_permissions, which can leave the file
world-readable if permissions change fails; instead open the file atomically
with restrictive permissions using std::fs::OpenOptions in
save_json_credentials: create an OpenOptions builder, set
write(true).create(true).truncate(true) and on Unix call .mode(0o600) before
opening, then write the serialized data into that file handle (replacing
std::fs::write) so the credential file is created with 0o600 from the start and
avoids the race/failure window where set_permissions might fail.
---
Outside diff comments:
In `@src/api/messaging.rs`:
- Around line 1548-1556: The current post-write block treats
reload_runtime_configs(&state, &config_path).await as best-effort and silently
ignores failures; change it so reload_runtime_configs errors are propagated (or
explicitly handled) and only update state.bindings after a successful reload.
Specifically, replace the if let Ok(new_config) = reload_runtime_configs(...)
pattern with a propagation (using ? on the Result) or explicit error handling,
and move the
bindings_swap.store(std::sync::Arc::new(new_config.bindings.clone())) call so it
only runs when reload_runtime_configs returns Ok; ensure no Result is discarded
and the function returns the reload failure to the caller instead of reporting
success.
- Around line 1558-1560: The manual start/re-enable and delete credential paths
in src/api/messaging.rs still construct token filenames as
twitch_token_<safe>.json while the watcher in src/config/watcher.rs uses
twitch_token_<safe>_<hash>.json; update the token-file naming logic used by the
manual start/re_enable_adapter and delete_adapter (or equivalent start/delete
handlers) to generate and look for twitch_token_{safe}_{hash}.json (matching the
watcher’s safe+hash scheme) so re-enable/delete will find and remove the same
credential files the watcher creates. Ensure any token-file lookup, creation,
and removal code in messaging.rs uses the same safe+hash naming helper/function
as watcher.rs.
In `@src/config/watcher.rs`:
- Around line 223-243: The per-instance permission refresh is currently nested
inside the conditional that requires a default discord_permissions (the `if let
Some(ref perms) = discord_permissions && let Some(discord_config) =
&config.messaging.discord` block), which prevents named-instance `ArcSwap`s from
being updated when the default adapter is absent; relocate the loop that reads
`named_discord_permissions` and updates each registry entry using
`binding_runtime_adapter_key("discord", Some(instance.name.as_str()))` and
`DiscordPermissions::from_instance_config(instance, &config.bindings)` so it
runs whenever `discord_config` is present (i.e., outside the
`Some(discord_permissions)` guard), keeping the existing update of the global
`perms.store(Arc::new(DiscordPermissions::from_config(...)))` where applicable.
---
Nitpick comments:
In `@src/api/secrets.rs`:
- Around line 799-804: The test export_secrets_requires_configured_api_auth only
checks the FORBIDDEN branch; update it to also exercise the configured-token
branch by calling state.set_api_auth_token(Some("test".into())).await before
invoking export_secrets(State(state)), then assert the response status matches
the next guard path (SERVICE_UNAVAILABLE in this test harness). This verifies
export_secrets actually consults the live auth token; keep the existing
FORBIDDEN check and add the additional set_api_auth_token + response assertion
in the same test (or a small follow-up test) referencing export_secrets and
State(state).
In `@src/main.rs`:
- Around line 1509-1516: Replace the verbose Arc<RwLock<HashMap<...>>>
declarations with the existing type aliases to improve readability: import or
reference NamedDiscordPermissions, NamedSlackPermissions,
NamedTelegramPermissions, and NamedTwitchPermissions (from config::watcher or
re-exported module) and initialize named_discord_permissions,
named_slack_permissions, named_telegram_permissions, and
named_twitch_permissions using Arc::new(RwLock::new(<alias>::default() or
HashMap::new() wrapped appropriately)) so the variables use the aliases instead
of explicit Arc<RwLock<HashMap<...>>> types.
In `@src/secrets/store.rs`:
- Around line 1103-1119: Add a regression assertion that ensures the webhook
auth token remains classified as a webhook secret: update the tests (preferably
in auto_categorize_known_patterns() or
system_secret_registry_contains_all_sections()) to assert that
"WEBHOOK_AUTH_TOKEN" (or the constant used by WebhookConfig::secret_fields()) is
present in the resulting registry/fields after calling the classification code;
this locks in the behavior so future changes to DefaultsConfig/LlmConfig/other
secret_fields() cannot silently reclassify the webhook token as a tool/internal
secret.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1dbc3aba-8b94-47d7-a9ed-3d0f6fa74f19
📒 Files selected for processing (18)
src/api.rssrc/api/admin.rssrc/api/bindings.rssrc/api/messaging.rssrc/api/providers.rssrc/api/secrets.rssrc/api/server.rssrc/api/settings.rssrc/api/state.rssrc/auth.rssrc/config/types.rssrc/config/watcher.rssrc/lib.rssrc/main.rssrc/memory/maintenance.rssrc/oauth_storage.rssrc/openai_auth.rssrc/secrets/store.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/api/settings.rs (2)
323-324:⚠️ Potential issue | 🟡 MinorSilently discarding
reload_runtime_configsresult violates coding guidelines.The
Resultfromreload_runtime_configsis discarded withlet _ =. Per coding guidelines, errors must not be silently discarded. Additionally, the returnedConfigcould be used to refreshstate.defaults_config.🛡️ Proposed fix to handle errors and use returned config
write_config_doc(&config_path, &doc).await?; - let _ = reload_runtime_configs(&state, &config_path).await; + match reload_runtime_configs(&state, &config_path).await { + Ok(new_config) => { + state.set_defaults_config(new_config.defaults.clone()).await; + } + Err(status) => { + tracing::warn!(?status, "config reload failed after settings update"); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/settings.rs` around lines 323 - 324, The call to reload_runtime_configs currently discards its Result with "let _ =", which violates error-handling rules and misses the returned Config; change the call to properly handle the Result from reload_runtime_configs(&state, &config_path).await by matching or using ?/map_err to propagate or log errors instead of ignoring them, and when it succeeds use the returned Config to update state.defaults_config (or otherwise refresh state) so the in-memory defaults reflect the written doc; ensure you reference reload_runtime_configs, state.defaults_config, write_config_doc, and config_path when making the change.
432-432:⚠️ Potential issue | 🟡 MinorSame issue: silently discarding
reload_runtime_configsresult.Same fix pattern should be applied here.
🛡️ Proposed fix
- let _ = reload_runtime_configs(&state, &config_path).await; + match reload_runtime_configs(&state, &config_path).await { + Ok(new_config) => { + state.set_defaults_config(new_config.defaults.clone()).await; + } + Err(status) => { + tracing::warn!(?status, "config reload failed after raw config update"); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/settings.rs` at line 432, The call to reload_runtime_configs(&state, &config_path).await is currently ignoring its Result; update the call in settings.rs (where reload_runtime_configs is invoked with variables state and config_path) to handle the Result instead of discarding it — for example, match or use if let Err(err) = reload_runtime_configs(&state, &config_path).await { /* log error via your logger (e.g., error!("...: {:?}", err)) or handle failure appropriately */ } so failures are surfaced and not silently dropped.
🧹 Nitpick comments (2)
src/oauth_storage.rs (1)
37-38: Redundantset_permissionscall after atomic creation withmode(0o600).Since the file is already created with
mode(0o600)viaOpenOptionsat line 32, the subsequentset_permissionscall is unnecessary. The atomic creation ensures the file never exists with broader permissions.♻️ Proposed removal of redundant call
file.write_all(data.as_bytes()) .with_context(|| format!("failed to write {}", path.display()))?; - std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)) - .with_context(|| format!("failed to set permissions on {}", path.display()))?; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/oauth_storage.rs` around lines 37 - 38, Remove the redundant std::fs::set_permissions(...) call in src/oauth_storage.rs that follows atomic creation via OpenOptions::mode(0o600); specifically delete the std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)).with_context(|| format!("failed to set permissions on {}", path.display()))?; line(s) and keep the existing OpenOptions creation/error handling intact so no additional permission-setting or context wrapping remains.src/main.rs (1)
2382-2401: Consider a type alias to reduce signature complexity.The
initialize_agentsfunction signature is quite verbose with four nearly-identical permission registry types. A type alias could improve readability without changing behavior.♻️ Example type alias
// At module level: type NamedPermissionsMap<T> = Arc<std::sync::RwLock<std::collections::HashMap<String, Arc<ArcSwap<T>>>>>; // Then in the signature: named_discord_permissions: NamedPermissionsMap<spacebot::config::DiscordPermissions>, named_slack_permissions: NamedPermissionsMap<spacebot::config::SlackPermissions>, // etc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 2382 - 2401, The function signature for initialize_agents contains four repeated verbose permission registry types; define a module-level generic type alias (e.g., NamedPermissionsMap<T>) that maps T to Arc<RwLock<HashMap<String, Arc<ArcSwap<T>>>>>, then replace the explicit types for named_discord_permissions, named_slack_permissions, named_telegram_permissions, and named_twitch_permissions in initialize_agents with NamedPermissionsMap<spacebot::config::DiscordPermissions> (and the corresponding SlackPermissions, TelegramPermissions, TwitchPermissions) and update any matching parameter usages to use the alias; ensure the alias is declared in the same module so ArcSwap and the std types resolve without additional changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/memory/maintenance.rs`:
- Around line 233-235: The current upsert via
memory_store.create_association(Association::new(...).with_weight(...)) can
overwrite a stronger existing canonical edge with a weaker rewritten one; change
the logic to first read the existing association for the same (source_id,
target_id, relation_type), compute a deterministic combined weight (e.g.
max(existing.weight, association.weight)), set the new Association's weight to
that max via with_weight, and then call memory_store.create_association with the
combined-weight Association (or, if the store supports an upsert/combine API,
call that with a combine function that returns the max weight) so the canonical
stronger edge is preserved instead of last-writer-wins.
- Around line 241-268: The parser currently coerces unknown memory_type strings
to MemoryType::Fact; change parse_memory_type to return Result<MemoryType,
anyhow::Error> (or a suitable error type) and return Err when the input doesn't
match a known variant, then change row_to_memory to return Result<Memory,
anyhow::Error> and propagate that error (including the offending string and row
id) instead of constructing a Memory with a default; update all callers of
row_to_memory to propagate or abort the run (bubble the error up) so the process
stops on unknown/corrupt enum values rather than silently converting them.
- Around line 130-136: The function merge_similar_memories currently ignores the
similarity_threshold and only deduplicates exact normalized-string matches;
update the logic in merge_similar_memories (and the same pattern in the other
similar blocks) to compute a real similarity score for each candidate pair
(e.g., cosine similarity of stored embeddings or normalized string similarity
like Jaro-Winkler/Levenshtein) and compare that score to similarity_threshold
before merging, or if embeddings/similarity code is not available, rename the
function/parameter to indicate exact-match deduplication (e.g.,
merge_exact_matches and exact_match_only) and remove the misleading threshold
parameter; ensure you reference MemoryStore methods used to fetch
text/embeddings and adjust the merge path to use the computed score for the
comparison.
- Around line 151-163: The grouping currently collapses different channels
because groups is keyed only by (MemoryType, normalized) — change the grouping
key to include each Memory's channel identifier (e.g., use (memory.memory_type,
memory.channel_id.clone(), normalized) as the HashMap key), update the HashMap
type declaration accordingly, and adjust the .entry(...) call to use that
three-tuple so memories from different channels are kept in separate groups;
reference normalize_memory_content, groups, and the Memory struct's channel_id
when making these edits.
---
Duplicate comments:
In `@src/api/settings.rs`:
- Around line 323-324: The call to reload_runtime_configs currently discards its
Result with "let _ =", which violates error-handling rules and misses the
returned Config; change the call to properly handle the Result from
reload_runtime_configs(&state, &config_path).await by matching or using
?/map_err to propagate or log errors instead of ignoring them, and when it
succeeds use the returned Config to update state.defaults_config (or otherwise
refresh state) so the in-memory defaults reflect the written doc; ensure you
reference reload_runtime_configs, state.defaults_config, write_config_doc, and
config_path when making the change.
- Line 432: The call to reload_runtime_configs(&state, &config_path).await is
currently ignoring its Result; update the call in settings.rs (where
reload_runtime_configs is invoked with variables state and config_path) to
handle the Result instead of discarding it — for example, match or use if let
Err(err) = reload_runtime_configs(&state, &config_path).await { /* log error via
your logger (e.g., error!("...: {:?}", err)) or handle failure appropriately */
} so failures are surfaced and not silently dropped.
---
Nitpick comments:
In `@src/main.rs`:
- Around line 2382-2401: The function signature for initialize_agents contains
four repeated verbose permission registry types; define a module-level generic
type alias (e.g., NamedPermissionsMap<T>) that maps T to
Arc<RwLock<HashMap<String, Arc<ArcSwap<T>>>>>, then replace the explicit types
for named_discord_permissions, named_slack_permissions,
named_telegram_permissions, and named_twitch_permissions in initialize_agents
with NamedPermissionsMap<spacebot::config::DiscordPermissions> (and the
corresponding SlackPermissions, TelegramPermissions, TwitchPermissions) and
update any matching parameter usages to use the alias; ensure the alias is
declared in the same module so ArcSwap and the std types resolve without
additional changes.
In `@src/oauth_storage.rs`:
- Around line 37-38: Remove the redundant std::fs::set_permissions(...) call in
src/oauth_storage.rs that follows atomic creation via OpenOptions::mode(0o600);
specifically delete the std::fs::set_permissions(path,
std::fs::Permissions::from_mode(0o600)).with_context(|| format!("failed to set
permissions on {}", path.display()))?; line(s) and keep the existing OpenOptions
creation/error handling intact so no additional permission-setting or context
wrapping remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 332933c8-a80e-4f0c-88a1-4ccc1dea4bc0
📒 Files selected for processing (7)
src/api/admin.rssrc/api/server.rssrc/api/settings.rssrc/main.rssrc/memory/maintenance.rssrc/memory/types.rssrc/oauth_storage.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/api/server.rs (1)
334-347:⚠️ Potential issue | 🟠 MajorNormalize blank auth tokens in the middleware path too.
Line 335 still treats
Some("")/ whitespace as configured, so any direct write tostate.auth_tokencan makeAuthorization: Bearerpass even though the startup guard already treats blank tokens as missing. Trim here as well instead of relying on setter-only invariants.Suggested hardening
- let expected_token = state.auth_token.read().await.clone(); - let Some(expected_token) = expected_token.as_deref() else { + let expected_token = state.auth_token.read().await.clone().and_then(|token| { + let trimmed = token.trim(); + (!trimmed.is_empty()).then(|| trimmed.to_owned()) + }); + let Some(expected_token) = expected_token.as_deref() else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/server.rs` around lines 334 - 347, The middleware currently treats Some("") or whitespace-only auth tokens as configured; update the logic around state.auth_token/read() and the local expected_token handling so blank or all-whitespace tokens are treated as missing—i.e., after reading/cloning expected_token (the variable named expected_token created from state.auth_token.read().await.clone()), trim it and treat empty results as None before the early public-listener check that uses state.api_bind and before calling next.run(request). Replace the current Some(...) path with a trimmed-and-empty-filtered check so Authorization: Bearer (empty) no longer bypasses auth while keeping the existing api_bind and next.run(request) behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/admin.rs`:
- Around line 155-176: The code currently trims the input value into
trimmed_value and then persists trimmed_value, which can alter valid secrets;
change the logic in the handler so trim() is used only to check emptiness (use
trimmed_value.is_empty()) but pass the original unmodified value to store.set;
locate the variable/value handling around trimmed_value and the call
store.set(&secret_name, trimmed_value, category) and replace that argument with
the original value (or its owned String) while keeping the emptiness check on
trimmed_value, leaving secret_name, category deduction, and error handling
unchanged.
In `@src/api/bindings.rs`:
- Around line 553-599: The failure branches for Slack adapter construction/start
currently only log errors (tracing::error!) and let the handler continue
returning a success payload; update the SlackAdapter::new error branch and the
manager.register_and_start(adapter).await Err branch (and the analogous blocks
around lines 620-670) to surface the failure in the HTTP response: either return
an Err(StatusCode::INTERNAL_SERVER_ERROR) immediately or mark the response
payload's restart_required/warning field to true and include a descriptive
message so callers can distinguish "saved" vs "running"; ensure you use the same
response type the handler returns and keep the tracing::error! log but also set
the response accordingly so failed Slack adapter creation/start is not reported
as active.
- Line 207: load_config_doc(&state).await? returns a guard that holds
config_write_mutex, so keep holding it only for the write+reload work and
release it before calling register_and_start(...) (which runs adapter login).
After obtaining (_config_guard, config_path, mut doc) from load_config_doc,
explicitly drop the guard (or let it go out of scope by moving the write/reload
into a narrow block) before any loop or calls to register_and_start; update the
remaining occurrences in this file (also around the 503-673 region) to ensure
the config guard is released prior to starting adapters.
In `@src/api/server.rs`:
- Around line 367-381: The CORS predicate in allow_local_origin only checks for
"http://" prefixes and exact matches; update it to also accept the equivalent
"https://" origins (e.g., add starts_with checks for "https://127.0.0.1:",
"https://localhost:", "https://[::1]:" and equality checks for
"https://127.0.0.1", "https://localhost", "https://[::1]") so loopback HTTPS dev
setups are allowed; apply the same change to the other identical predicate
referenced in the same file (the second allow_local_origin-style check around
the later block).
---
Duplicate comments:
In `@src/api/server.rs`:
- Around line 334-347: The middleware currently treats Some("") or
whitespace-only auth tokens as configured; update the logic around
state.auth_token/read() and the local expected_token handling so blank or
all-whitespace tokens are treated as missing—i.e., after reading/cloning
expected_token (the variable named expected_token created from
state.auth_token.read().await.clone()), trim it and treat empty results as None
before the early public-listener check that uses state.api_bind and before
calling next.run(request). Replace the current Some(...) path with a
trimmed-and-empty-filtered check so Authorization: Bearer (empty) no longer
bypasses auth while keeping the existing api_bind and next.run(request)
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7703d7e6-3922-453b-a99d-f206455caf25
📒 Files selected for processing (8)
src/api/admin.rssrc/api/bindings.rssrc/api/messaging.rssrc/api/providers.rssrc/api/secrets.rssrc/api/server.rssrc/api/settings.rssrc/api/state.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/secrets.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/memory/maintenance.rs`:
- Around line 182-195: The current seed-only clustering in the loop that uses
normalized_similarity and similarity_threshold can miss transitive duplicates;
change the clustering in the function (the while let Some((seed_memory,
seed_normalized)) { ... } block) to perform BFS/expansion: initialize a queue
with the seed_normalized (and seed_memory), iterate popping items from the queue
and compare each remaining (memory, normalized) to the popped normalized, and
when similarity >= similarity_threshold push that memory into duplicates and
also enqueue its normalized for further comparisons; only after the expansion
finishes set remaining to the next_remaining of those not matched. This builds
connected components (or at least expands clusters) instead of only comparing to
the original seed.
- Around line 139-141: The code currently returns Ok(0) when
similarity_threshold is outside 0.0..=1.0 which hides misconfiguration; replace
that silent no-op with a proper error return. In the block testing
similarity_threshold (related to merge_similarity_threshold), return an Err with
a clear message including the invalid value (e.g., "invalid
merge_similarity_threshold: {value}, must be in 0.0..=1.0") using the function's
existing error type (e.g., anyhow::bail! or return Err(...) so the caller fails
fast) instead of Ok(0).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fba9e2e8-8886-4937-ba6e-bfd567b183b3
📒 Files selected for processing (1)
src/memory/maintenance.rs
Contributor
Author
|
Follow-up for commit This pass addressed the remaining active bot/review notes that were still actionable on the branch. What changed:
Local validation on this commit passed with a local
Result: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/messaging.rs (1)
1501-1533:⚠️ Potential issue | 🟠 MajorReject named webhook instances until the runtime supports them.
This writes
[[messaging.webhook.instances]], butWebhookConfighas noinstancesfield andis_named_adapter_platform()excludes"webhook". The API can therefore return success for an adapter that bindings, status, and runtime startup never actually use. Rejectrequest.nameforwebhookuntil multi-instance support exists end-to-end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 1501 - 1533, The code currently writes [[messaging.webhook.instances]] and accepts request.name for "webhook" even though WebhookConfig lacks instances and is_named_adapter_platform() excludes "webhook"; update the request validation to reject named webhook instances by checking if the platform equals "webhook" and request.name.is_some() (return an appropriate validation/error result), and prevent creating or appending an "instances" table for webhook in the block that handles the "webhook" match arm and the subsequent platform_table.instances insertion; reference the "webhook" match arm, the instance-table insertion logic (platform_table.insert/get_mut/...as_array_of_tables_mut/instances.push), WebhookConfig, is_named_adapter_platform(), and request.name to locate where to add this check.
🧹 Nitpick comments (1)
src/config/watcher.rs (1)
356-368: Minor:std::sync::RwLock::write()in async context.Using
std::sync::RwLockinsidert.spawn(async move { ... })blocks the async runtime while holding the lock. The critical section (a singleinsert) is tiny so this is unlikely to cause issues in practice, but if contention grows, consider switching totokio::sync::RwLockor wrapping the write inspawn_blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/watcher.rs` around lines 356 - 368, The code is holding a std::sync::RwLock write guard inside async work (the named_discord_permissions.write().expect("lock poisoned").insert(runtime_key.clone(), permissions.clone()) call inside the async hot-start path); replace this blocking lock with an async-aware lock or offload the write: either change named_discord_permissions to a tokio::sync::RwLock and use .write().await before .insert, or move the insert into a blocking task via tokio::task::spawn_blocking, ensuring runtime_key and permissions are cloned into that closure; keep the rest of the flow (creating DiscordAdapter and calling manager.register_and_start) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/server.rs`:
- Around line 370-390: The allow_local_origin function currently matches only
three literal hosts; change it to parse the origin into a URL (e.g., using
url::Url::parse) and validate scheme is http or https, then extract
url.host_str() and accept if the host is "localhost" or if
host.parse::<std::net::IpAddr>().map_or(false, |ip| ip.is_loopback()); keep the
existing port-agnostic behavior and return false on parse errors—update
allow_local_origin to use this host/is_loopback logic instead of the current
starts_with / equality checks.
In `@src/config/types.rs`:
- Around line 1557-1569: The function named_twitch_token_file_name currently
uses std::collections::hash_map::DefaultHasher which is unstable across Rust
versions; replace that with a stable cryptographic digest (e.g., blake3 or
SHA-256) to produce a deterministic filename. Keep the safe_name sanitization,
compute a stable hex digest from name.as_bytes() using a chosen crate (for
example blake3::hash or sha2::Sha256), optionally truncate to a fixed length for
readability, and use that hex digest in the existing
format!("twitch_token_{safe_name}_{name_hash:016x}.json") (adjust formatting
token to a hex string). Add the digest crate to Cargo.toml and update imports;
ensure the function name named_twitch_token_file_name remains unchanged.
In `@src/oauth_storage.rs`:
- Around line 19-43: save_json_credentials currently truncates the target file
before the new JSON is fully written, risking corruption; change it to write
atomically by creating a same-directory temporary file (e.g., path with a
random/suffix tmp name), write the pretty JSON to that temp file, call
file.sync_all() and set permissions (on unix set mode 0o600 via OpenOptionsExt)
before renaming, then atomically replace the original with
std::fs::rename(temp_path, path); after rename, sync the parent directory
(std::fs::File::open(dir)?.sync_all()) to ensure durability; keep this behavior
for both cfg(unix) and cfg(not(unix)) branches and ensure errors are propagated
from the functions used in save_json_credentials.
---
Outside diff comments:
In `@src/api/messaging.rs`:
- Around line 1501-1533: The code currently writes
[[messaging.webhook.instances]] and accepts request.name for "webhook" even
though WebhookConfig lacks instances and is_named_adapter_platform() excludes
"webhook"; update the request validation to reject named webhook instances by
checking if the platform equals "webhook" and request.name.is_some() (return an
appropriate validation/error result), and prevent creating or appending an
"instances" table for webhook in the block that handles the "webhook" match arm
and the subsequent platform_table.instances insertion; reference the "webhook"
match arm, the instance-table insertion logic
(platform_table.insert/get_mut/...as_array_of_tables_mut/instances.push),
WebhookConfig, is_named_adapter_platform(), and request.name to locate where to
add this check.
---
Nitpick comments:
In `@src/config/watcher.rs`:
- Around line 356-368: The code is holding a std::sync::RwLock write guard
inside async work (the named_discord_permissions.write().expect("lock
poisoned").insert(runtime_key.clone(), permissions.clone()) call inside the
async hot-start path); replace this blocking lock with an async-aware lock or
offload the write: either change named_discord_permissions to a
tokio::sync::RwLock and use .write().await before .insert, or move the insert
into a blocking task via tokio::task::spawn_blocking, ensuring runtime_key and
permissions are cloned into that closure; keep the rest of the flow (creating
DiscordAdapter and calling manager.register_and_start) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c0964ca3-e26b-4776-9a1a-c347e6259a91
📒 Files selected for processing (10)
src/api/admin.rssrc/api/bindings.rssrc/api/messaging.rssrc/api/secrets.rssrc/api/server.rssrc/config/types.rssrc/config/watcher.rssrc/main.rssrc/oauth_storage.rssrc/secrets/store.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/secrets/store.rs
Comment on lines
+370
to
+390
| fn allow_local_origin( | ||
| origin: &header::HeaderValue, | ||
| _request_parts: &axum::http::request::Parts, | ||
| ) -> bool { | ||
| let Ok(origin) = origin.to_str() else { | ||
| return false; | ||
| }; | ||
|
|
||
| origin.starts_with("http://127.0.0.1:") | ||
| || origin.starts_with("https://127.0.0.1:") | ||
| || origin.starts_with("http://localhost:") | ||
| || origin.starts_with("https://localhost:") | ||
| || origin.starts_with("http://[::1]:") | ||
| || origin.starts_with("https://[::1]:") | ||
| || origin == "http://127.0.0.1" | ||
| || origin == "https://127.0.0.1" | ||
| || origin == "http://localhost" | ||
| || origin == "https://localhost" | ||
| || origin == "http://[::1]" | ||
| || origin == "https://[::1]" | ||
| } |
Contributor
There was a problem hiding this comment.
Handle all loopback origins, not just three literals.
The helper says "localhost/loopback", but it only admits 127.0.0.1, localhost, and [::1]. Valid local origins like http://127.0.0.2:5173 or http://127.1:5173 are still rejected. Parsing the origin host and checking is_loopback() would match the bind policy and avoid surprising local-dev CORS failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/server.rs` around lines 370 - 390, The allow_local_origin function
currently matches only three literal hosts; change it to parse the origin into a
URL (e.g., using url::Url::parse) and validate scheme is http or https, then
extract url.host_str() and accept if the host is "localhost" or if
host.parse::<std::net::IpAddr>().map_or(false, |ip| ip.is_loopback()); keep the
existing port-agnostic behavior and return false on parse errors—update
allow_local_origin to use this host/is_loopback logic instead of the current
starts_with / equality checks.
Comment on lines
+1557
to
+1569
| /// Build the persisted token filename for a named Twitch adapter instance. | ||
| pub fn named_twitch_token_file_name(name: &str) -> String { | ||
| use std::hash::{Hash, Hasher}; | ||
|
|
||
| let safe_name: String = name | ||
| .chars() | ||
| .map(|ch| if ch.is_ascii_alphanumeric() { ch } else { '_' }) | ||
| .collect(); | ||
| let mut hasher = std::collections::hash_map::DefaultHasher::new(); | ||
| name.hash(&mut hasher); | ||
| let name_hash = hasher.finish(); | ||
|
|
||
| format!("twitch_token_{safe_name}_{name_hash:016x}.json") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Do the Rust standard library docs guarantee stable output for std::collections::hash_map::DefaultHasher across compiler or standard-library releases?
💡 Result:
No. The standard library docs explicitly say std::collections::hash_map::DefaultHasher’s internal algorithm is not specified, and therefore “it and its hashes should not be relied upon over releases” (i.e., they are not guaranteed stable across compiler/stdlib releases). (doc.rust-lang.org)
Also, because DefaultHasher is the hasher used by RandomState, and different RandomState instances are designed to produce different hash results for the same input, you shouldn’t expect stable/reproducible hash outputs in general. (doc.rust-lang.org)
Citations:
- 1: https://doc.rust-lang.org/std/collections/hash_map/struct.DefaultHasher.html
- 2: https://doc.rust-lang.org/std/hash/struct.RandomState.html?utm_source=openai
Use a stable digest for persisted Twitch token filenames.
DefaultHasher is not suitable for on-disk identifiers. The Rust standard library explicitly states that DefaultHasher's algorithm is unspecified and that "it and its hashes should not be relied upon over releases." A compiler or standard-library upgrade can change the hash output, invalidating existing token filenames and forcing named Twitch adapters to re-authenticate. Use a stable digest algorithm (e.g., SHA-256 or Blake3) instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config/types.rs` around lines 1557 - 1569, The function
named_twitch_token_file_name currently uses
std::collections::hash_map::DefaultHasher which is unstable across Rust
versions; replace that with a stable cryptographic digest (e.g., blake3 or
SHA-256) to produce a deterministic filename. Keep the safe_name sanitization,
compute a stable hex digest from name.as_bytes() using a chosen crate (for
example blake3::hash or sha2::Sha256), optionally truncate to a fixed length for
readability, and use that hex digest in the existing
format!("twitch_token_{safe_name}_{name_hash:016x}.json") (adjust formatting
token to a hex string). Add the digest crate to Cargo.toml and update imports;
ensure the function name named_twitch_token_file_name remains unchanged.
Comment on lines
+19
to
+43
| pub fn save_json_credentials<T: Serialize>(path: &Path, credentials: &T) -> Result<()> { | ||
| let data = serde_json::to_string_pretty(credentials) | ||
| .with_context(|| format!("failed to serialize {}", path.display()))?; | ||
|
|
||
| #[cfg(unix)] | ||
| { | ||
| use std::io::Write as _; | ||
| use std::os::unix::fs::OpenOptionsExt; | ||
|
|
||
| let mut file = std::fs::OpenOptions::new() | ||
| .create(true) | ||
| .truncate(true) | ||
| .write(true) | ||
| .mode(0o600) | ||
| .open(path) | ||
| .with_context(|| format!("failed to open {}", path.display()))?; | ||
| file.write_all(data.as_bytes()) | ||
| .with_context(|| format!("failed to write {}", path.display()))?; | ||
| } | ||
|
|
||
| #[cfg(not(unix))] | ||
| { | ||
| std::fs::write(path, &data) | ||
| .with_context(|| format!("failed to write {}", path.display()))?; | ||
| } |
Contributor
There was a problem hiding this comment.
Write OAuth credential updates atomically.
This truncates the live JSON before the replacement bytes are safely in place. A crash or concurrent save in that window leaves unreadable credentials on disk, and the next load_json_credentials() forces a fresh OAuth login. Persist to a same-directory temp file and rename it into place after the write succeeds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/oauth_storage.rs` around lines 19 - 43, save_json_credentials currently
truncates the target file before the new JSON is fully written, risking
corruption; change it to write atomically by creating a same-directory temporary
file (e.g., path with a random/suffix tmp name), write the pretty JSON to that
temp file, call file.sync_all() and set permissions (on unix set mode 0o600 via
OpenOptionsExt) before renaming, then atomically replace the original with
std::fs::rename(temp_path, path); after rename, sync the parent directory
(std::fs::File::open(dir)?.sync_all()) to ensure durability; keep this behavior
for both cfg(unix) and cfg(not(unix)) branches and ensure errors are propagated
from the functions used in save_json_credentials.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
This PR hardens the admin plane so API-initiated configuration changes no longer write plaintext credentials into
config.toml, narrows access to the highest-risk admin endpoints, and replaces a few documented-but-unimplemented maintenance paths with real behavior.At a high level, this change does five things:
SecretsStoreand writessecret:references into config instead of plaintext values.api.auth_token.Why?
Before this change, the admin API had a few concrete weaknesses:
update_provider,create_messaging_instance, andcreate_bindingcould persist sensitive credentials directly intoconfig.toml.config/rawandsecrets/exportwere only protected by the optional global API token. Ifapi.auth_tokenwas unset, these very sensitive admin surfaces were effectively open.Originheaders and would happily start on non-loopback binds without auth, which is too easy to misconfigure for a control-plane API.ApiStatewas fixed at startup. Updating config later did not change the middleware behavior.ArcSwaps.This PR fixes those issues without changing the intended local-dev workflow:
How?
1. Shared admin config / secret plumbing
New helper module:
src/api/admin.rsThis module centralizes the repeated admin-plane logic for:
config.tomledits,SecretsStore,secret:references fromSystemSecretsmetadata.This removes the prior pattern where each admin handler open-coded its own read/parse/write/reload sequence and independently decided how to persist credentials.
2. Stop writing plaintext credentials into config
Changed handlers:
src/api/providers.rssrc/api/messaging.rssrc/api/bindings.rsBehavior changes:
SecretsStoreand writesecret:...references into[llm].SecretsStoreand persists only references intoconfig.toml.Important detail:
ollama_base_urlstays plaintext because it is configuration, not a secret.WebhookConfignow participates inSystemSecrets, so webhook auth tokens follow the same secret-store path and auto-categorization as the other admin-managed credentials.3. Harden the HTTP API surface
Changed handlers / server wiring:
src/api/server.rssrc/api/settings.rssrc/api/secrets.rssrc/api/state.rsBehavior changes:
GET /api/config/raw,PUT /api/config/raw, andPOST /api/secrets/exportnow require a configuredapi.auth_token. If the server is running without one, those endpoints return403instead of exposing sensitive content.api.auth_token.ApiState.auth_tokenis now live and reloadable. Config reloads update the middleware token instead of leaving the server on whatever auth state existed at startup.4. Shared OAuth credential persistence
New helper module:
src/oauth_storage.rsChanged modules:
src/auth.rssrc/openai_auth.rsThis extracts the duplicated JSON credential save/load logic used by Anthropic and OpenAI OAuth persistence into one shared helper with tests. That gives us one place to maintain the on-disk policy and removes duplicate serialization / permission code.
5. Hot-reload permissions for named messaging adapters
Changed modules:
src/config/watcher.rssrc/main.rsThis introduces registries of the named adapter permission
ArcSwaps for Discord, Slack, Telegram, and Twitch.Before:
After:
ArcSwapfor each named runtime key on config reload,6. Replace the memory merge stub
Changed module:
src/memory/maintenance.rsmerge_similar_memories()is now a real implementation instead of returningOk(0)unconditionally.Implementation details:
This is intentionally conservative. It does not attempt semantic embedding-based merge yet; it implements a real exact/normalized duplicate merge that is safe and testable with the current maintenance inputs.
Testing
Added targeted regression tests for:
update_providerwritessecret:refs + stores value),Validation run:
cargo fmtcargo fmt --checkValidation caveat
Full Rust build validation is currently blocked in this environment because
lance-encoding's build script requiresprotoc, andprotocis not installed here.The current
cargo checkfailure is:So the PR includes formatting validation and targeted test additions, but I could not complete a full compile/test run from this machine.
Reviewer guide
Suggested review order:
src/api/admin.rssrc/api/providers.rssrc/api/messaging.rssrc/api/bindings.rssrc/api/server.rs+src/api/settings.rs+src/api/secrets.rssrc/config/watcher.rs+src/main.rssrc/oauth_storage.rs,src/auth.rs,src/openai_auth.rssrc/memory/maintenance.rsScreenshots
N/A. This PR is backend / config-plane / maintenance work only and does not change the UI.
Note
This PR consolidates security hardening across the admin API surface. The core change moves sensitive credentials from plaintext
config.tomlwrites into an encryptedSecretsStore, withsecret:references persisted to config instead. Additionally, the HTTP API now gates raw config and secret export endpoints with mandatory authentication, refuses to bind on non-loopback addresses without configured auth tokens, and restricts CORS to localhost. The API auth token is now live-reloadable on config changes, named adapter permissions hot-reload via registries, and the memory maintenance merge function transitions from a stub to a real duplicate-merging implementation.Written by Tembo for commit ac586cdd. This will update automatically on new commits.