feat(extensions): unify auth and configure into single entrypoint#677
feat(extensions): unify auth and configure into single entrypoint#677henrypark133 merged 5 commits intostagingfrom
Conversation
github-actions
Bot
added
scope: extensions
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and consistency of extension management, particularly for chat channels. It addresses issues with lingering secrets after extension removal and unifies the authentication process, making it more reliable and extensible. The changes ensure that authentication flows are standardized and that sensitive data is properly cleaned up, improving the overall security and user experience of extensions. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the extension authentication and removal process, unifying the auth paths for chat and the Extensions tab for greater consistency. However, it introduces a High-severity SSRF vulnerability in the new secret validation logic and misses path traversal validation in the save_setup_secrets method. Additionally, there are suggestions to improve the robustness of the new error handling, enhance performance by using a more efficient data structure, and address code duplication for better maintainability.
There was a problem hiding this comment.
Pull request overview
This PR addresses inconsistent authentication behavior between chat-driven setup and the Extensions UI, and fixes stale secrets persisting across uninstall/reinstall cycles for extensions.
Changes:
- Add secret cleanup during
ExtensionManager::remove()using capabilities-based secret enumeration plus a provider-tag fallback. - Unify chat auth for WASM channels by routing token entry through
save_setup_secrets()(enabling validation, auto-generation, and activation). - Replace Telegram-specific token validation with a generic
validation_endpointdeclared in channel capabilities (Telegram updated accordingly).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/extensions/manager.rs |
Implements secret cleanup on remove, unifies WASM channel auth path via save_setup_secrets, and adds capabilities-driven token validation. |
channels-src/telegram/telegram.capabilities.json |
Adds validation_endpoint for Telegram bot token validation via the Telegram API. |
Comments suppressed due to low confidence (1)
src/extensions/manager.rs:2988
save_setup_secretsreads and parses the channel capabilities file multiple times (allowed-secret collection, validation_endpoint, and auto-generate blocks). This adds redundant I/O/JSON parsing and makes the logic harder to keep consistent. Consider loadingcap_fileonce for the channel path and reusing it for allowed names, validation, and auto-generation.
// Load allowed secret names from the extension's capabilities file
let allowed: std::collections::HashSet<String> = match kind {
ExtensionKind::WasmChannel => {
let cap_path = self
.wasm_channels_dir
.join(format!("{}.capabilities.json", name));
if !cap_path.exists() {
return Err(ExtensionError::Other(format!(
"Capabilities file not found for '{}'",
name
)));
}
let cap_bytes = tokio::fs::read(&cap_path)
.await
.map_err(|e| ExtensionError::Other(e.to_string()))?;
let cap_file =
crate::channels::wasm::ChannelCapabilitiesFile::from_bytes(&cap_bytes)
.map_err(|e| ExtensionError::Other(e.to_string()))?;
cap_file
.setup
.required_secrets
.iter()
.map(|s| s.name.clone())
.collect()
}
ExtensionKind::WasmTool => {
let cap_file = self.load_tool_capabilities(name).await.ok_or_else(|| {
ExtensionError::Other(format!("Capabilities file not found for '{}'", name))
})?;
match cap_file.setup {
Some(s) => s.required_secrets.iter().map(|s| s.name.clone()).collect(),
None => {
return Err(ExtensionError::Other(format!(
"Tool '{}' has no setup schema — no secrets to configure",
name
)));
}
}
}
_ => {
return Err(ExtensionError::Other(
"Setup is only supported for WASM channels and tools".to_string(),
));
}
};
// Validate secrets against the validation_endpoint if declared in capabilities.
// The endpoint URL template uses {secret_name} placeholders that are
// substituted with the provided secret value before making the request.
if kind == ExtensionKind::WasmChannel {
let cap_path = self
.wasm_channels_dir
.join(format!("{}.capabilities.json", name));
if let Ok(cap_bytes) = tokio::fs::read(&cap_path).await
&& let Ok(cap_file) =
crate::channels::wasm::ChannelCapabilitiesFile::from_bytes(&cap_bytes)
&& let Some(ref endpoint_template) = cap_file.setup.validation_endpoint
&& let Some(secret_def) = cap_file
.setup
.required_secrets
.iter()
.find(|s| !s.optional && secrets.contains_key(&s.name))
&& let Some(token_value) = secrets.get(&secret_def.name)
{
let token = token_value.trim();
if !token.is_empty() {
let encoded =
url::form_urlencoded::byte_serialize(token.as_bytes()).collect::<String>();
let url =
endpoint_template.replace(&format!("{{{}}}", secret_def.name), &encoded);
let resp = reqwest::Client::builder()
.timeout(std::time::Duration::from_secs(10))
.build()
.map_err(|e| ExtensionError::Other(e.to_string()))?
.get(&url)
.send()
.await
.map_err(|e| {
ExtensionError::Other(format!("Failed to validate token: {}", e))
})?;
if !resp.status().is_success() {
return Err(ExtensionError::Other(format!(
"Invalid token (API returned {})",
resp.status()
)));
}
}
}
}
// Validate and store each submitted secret
for (secret_name, secret_value) in secrets {
if !allowed.contains(secret_name.as_str()) {
return Err(ExtensionError::Other(format!(
"Unknown secret '{}' for extension '{}'",
secret_name, name
)));
}
if secret_value.trim().is_empty() {
continue;
}
let params =
CreateSecretParams::new(secret_name, secret_value).with_provider(name.to_string());
self.secrets
.create(&self.user_id, params)
.await
.map_err(|e| ExtensionError::AuthFailed(e.to_string()))?;
}
// Auto-generate any missing secrets (channel-only feature)
if kind == ExtensionKind::WasmChannel {
let cap_path = self
.wasm_channels_dir
.join(format!("{}.capabilities.json", name));
if let Ok(cap_bytes) = tokio::fs::read(&cap_path).await
&& let Ok(cap_file) =
crate::channels::wasm::ChannelCapabilitiesFile::from_bytes(&cap_bytes)
{
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let kind = self.determine_installed_kind(name).await?; | ||
|
|
||
| // Clean up secrets before deleting capabilities files | ||
| self.cleanup_extension_secrets(name, kind).await; |
There was a problem hiding this comment.
remove() calls cleanup_extension_secrets() but discards any failures inside that cleanup (most operations are ok()/ignored). If the secrets backend is unavailable, remove will still succeed while leaving stale secrets behind—the exact class of issue this PR is trying to prevent. Consider returning Result<()> from cleanup_extension_secrets and either failing remove() on cleanup errors or at least logging warnings when deletions/listing fail.
| self.cleanup_extension_secrets(name, kind).await; | |
| if let Err(err) = self.cleanup_extension_secrets(name, kind).await { | |
| eprintln!( | |
| "Warning: failed to clean up secrets for extension '{}': {}", | |
| name, err | |
| ); | |
| } |
There was a problem hiding this comment.
N/A in current commit. cleanup_extension_secrets() was removed. remove() no longer touches secrets.
henrypark133
force-pushed
the
unify-extension-lifecycle
branch
from
64a72a0 to
974d916
Compare
github-actions
Bot
added
size: L
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
The architectural direction is sound -- splitting auth() into read-only status check and configure() mutation. Four blocking issues:
-
SSRF vulnerability in
validation_endpoint-- The URL from capabilities.json is used for an outbound HTTP request with zero SSRF protection. A malicious capabilities file could targethttp://169.254.169.254/latest/meta-data/or internal services. The project already hasvalidate_fetch_url()-- use it here. -
String-based error discrimination -- Three call sites distinguish errors via
msg.contains("Invalid token") || msg.contains("API returned"). Use typed error variants (TokenValidationFailed) instead of string matching. -
Missing regression tests -- PR description claims three tests but none appear in the diff.
-
Secret cleanup on
remove()not implemented -- The PR title says "clean up secrets on remove" but theremove()method has no changes in the diff.
Non-blocking: Capabilities file parsed multiple times in configure(), MissingSecret struct defined but never populated, WebSocket error path missing AuthRequired re-emission.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (1)
src/channels/web/ws.rs:259
- On WebSocket auth token submission, validation failures are only returned as
WsServerMessage::Error, so the UI won’t get anauth_requiredSSE event to re-show the auth card. Consider detectingExtensionError::ValidationFailed(_)here and broadcastingSseEvent::AuthRequired(similar to the HTTP chat auth handler) so retries work consistently over WS.
Err(e) => {
let _ = direct_tx
.send(WsServerMessage::Error {
message: format!("Auth failed: {}", e),
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .setup | ||
| .required_secrets | ||
| .iter() | ||
| .find(|s| !s.optional) | ||
| .map(|s| s.name.clone()) |
There was a problem hiding this comment.
For WASM channels, configure_token() selects the first non-optional secret in required_secrets rather than the first missing required secret. This breaks channels with multiple required secrets (e.g. discord/slack): when the UI prompts for the second secret, the submitted value will be stored under the wrong name. Compute the missing required secrets using the secrets store (same logic as auth_wasm_channel_status) and choose the first missing one.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Follow-up review (after 2e9b595)
Good progress. The ValidationFailed error variant (item #2 from my previous review) is properly addressed. The architecture -- splitting auth() into read-only status check and configure() mutation -- is clean and well-structured. Callers are simpler and the code deduplication is significant.
Remaining blocking issues:
1. SSRF in validation_endpoint -- still unresolved
Henry's reply says capabilities files are trusted (registry or local filesystem). That's true today, but:
- The capabilities file is JSON on disk. Any future code that writes/modifies capabilities (registry sync, user config edits, etc.) becomes an SSRF vector.
- The project already has
validate_fetch_url()insrc/tools/builtin/skill_tools.rsthat blocks private IPs, localhost, and cloud metadata endpoints. The cost of calling it here is one function call. Defense in depth is a core project principle per CLAUDE.md. - The
NETWORK_SECURITY.mddoc explicitly lists SSRF protection as a review checklist item.
Minimal fix: call validate_fetch_url() (or extract its core logic into a shared util) on the resolved URL before reqwest::get(). This is a one-liner addition after the endpoint_template.replace() call.
2. Transport errors mapped to ValidationFailed
In configure(), the .map_err on the send().await maps network/timeout/DNS errors to ExtensionError::ValidationFailed. Downstream callers treat ValidationFailed as "bad token, re-prompt the user" -- so a transient network failure will tell the user their token is invalid and ask them to re-enter it. This was flagged by both Copilot reviews.
Fix: Map transport errors to ExtensionError::Other (or a new ExtensionError::NetworkError variant). Only map non-success HTTP status codes to ValidationFailed.
// Transport errors -> infrastructure error
.map_err(|e| ExtensionError::Other(format!("Token validation request failed: {}", e)))?;
// API rejection -> validation error
if !resp.status().is_success() {
return Err(ExtensionError::ValidationFailed(...));
}3. PR description/title mismatch with actual changes
The title says "clean up secrets on remove" and the PR description lists three test functions, but neither the secret cleanup code nor the tests appear in the diff. Henry confirmed these were intentionally dropped. The PR title and description need to be updated to match reality, otherwise reviewers (and future git-log readers) will be misled.
4. configure_token() picks first non-optional secret, not first missing secret
For channels with multiple required secrets (e.g., if discord/slack were added), configure_token() always maps the token to the first non-optional secret in required_secrets, even if that secret already exists. This means re-configuring the second secret is impossible through this path. Copilot flagged this as well.
Fix: Query the secrets store for existing secrets and pick the first missing non-optional secret, matching the logic in auth_wasm_channel_status().
5. WebSocket path missing AuthRequired re-emission on ValidationFailed
In src/channels/web/ws.rs, the error path broadcasts WsServerMessage::Error but does not re-emit AuthRequired for ValidationFailed errors like the HTTP chat handler does. This means WebSocket users won't see the auth card re-appear on a bad token.
Non-blocking observations
- Capabilities file parsed 3x in
configure(): Once for allowed names, once for validation_endpoint, once for auto-generation. Should be loaded once and reused. Not blocking but worth cleaning up. MissingSecretstruct defined but never populated: AllConfigureResultreturns havemissing_secrets: vec![]. Either populate it (useful for multi-secret flows) or remove it to avoid dead code. If keeping it, populate it so callers can drive multi-step auth UX.auth()doc comment still says "no side effects": The MCP auth path can initiate OAuth listeners and persist env-var tokens. The doc should reflect actual behavior.Box::pinfor async recursion: The comment on line ~2744 is helpful. Consider whether the recursion depth is bounded (it appears to be, sinceauth()no longer callsconfigure()).
henrypark133
changed the title
Addressed in a54b760All 5 blocking items resolved: 1. SSRF protection — 2. Transport errors vs ValidationFailed — Transport/network errors now map to 3. PR title/description — Updated to match actual changes. Removed "clean up secrets on remove" from title. 4. configure_token() picks first missing secret — Now queries the secrets store and picks the first non-optional secret that doesn't already exist. Falls back to first non-optional if all exist (overwrite case). Matches 5. WebSocket AuthRequired re-emission — WS error path now checks for Non-blocking items noted for follow-up:
|
henrypark133
force-pushed
the
unify-extension-lifecycle
branch
from
a54b760 to
525533f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
src/channels/web/server.rs:1853
extensions_setup_submit_handleralways broadcastsAuthCompleted { success: true }even whenconfigure()returnsactivated: false(activation failed). Since the response already surfacesresult.activated, consider aligning the SSE event’ssuccessflag (and/or using a different event) so the UI doesn’t treat a failed activation as a completed successful auth.
match ext_mgr.configure(&name, &req.secrets).await {
Ok(result) => {
// Broadcast auth_completed so the chat UI can dismiss any in-progress
// auth card or setup modal that was triggered by tool_auth/tool_activate.
state.sse.broadcast(SseEvent::AuthCompleted {
extension_name: name.clone(),
success: true,
message: result.message.clone(),
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| match ext_mgr | ||
| .configure_token(&pending.extension_name, token) | ||
| .await | ||
| { | ||
| Ok(result) => { | ||
| tracing::info!( | ||
| "Extension '{}' authenticated via auth mode", | ||
| pending.extension_name | ||
| "Extension '{}' configured via auth mode: {}", | ||
| pending.extension_name, | ||
| result.message | ||
| ); | ||
|
|
||
| // Auto-activate so tools are available immediately after auth | ||
| match ext_mgr.activate(&pending.extension_name).await { | ||
| Ok(activate_result) => { | ||
| let tool_count = activate_result.tools_loaded.len(); | ||
| let tool_list = if activate_result.tools_loaded.is_empty() { | ||
| String::new() | ||
| } else { | ||
| format!("\n\nTools: {}", activate_result.tools_loaded.join(", ")) | ||
| }; | ||
| let msg = format!( | ||
| "{} authenticated and activated ({} tools loaded).{}", | ||
| pending.extension_name, tool_count, tool_list | ||
| ); | ||
| let _ = self | ||
| .channels | ||
| .send_status( | ||
| &message.channel, | ||
| StatusUpdate::AuthCompleted { | ||
| extension_name: pending.extension_name.clone(), | ||
| success: true, | ||
| message: msg.clone(), | ||
| }, | ||
| &message.metadata, | ||
| ) | ||
| .await; | ||
| Ok(Some(msg)) | ||
| } | ||
| Err(e) => { | ||
| tracing::warn!( | ||
| "Extension '{}' authenticated but activation failed: {}", | ||
| pending.extension_name, | ||
| e | ||
| ); | ||
| let msg = format!( | ||
| "{} authenticated successfully, but activation failed: {}. \ | ||
| Try activating manually.", | ||
| pending.extension_name, e | ||
| ); | ||
| let _ = self | ||
| .channels | ||
| .send_status( | ||
| &message.channel, | ||
| StatusUpdate::AuthCompleted { | ||
| extension_name: pending.extension_name.clone(), | ||
| success: true, | ||
| message: msg.clone(), | ||
| }, | ||
| &message.metadata, | ||
| ) | ||
| .await; | ||
| Ok(Some(msg)) | ||
| } | ||
| } | ||
| } | ||
| Ok(result) => { | ||
| // Invalid token, re-enter auth mode | ||
| { | ||
| let mut sess = session.lock().await; | ||
| if let Some(thread) = sess.threads.get_mut(&thread_id) { | ||
| thread.enter_auth_mode(pending.extension_name.clone()); | ||
| } | ||
| } | ||
| let msg = result | ||
| .instructions() | ||
| .map(String::from) | ||
| .unwrap_or_else(|| "Invalid token. Please try again.".to_string()); | ||
| // Re-emit AuthRequired so web UI re-shows the card | ||
| let _ = self | ||
| .channels | ||
| .send_status( | ||
| &message.channel, | ||
| StatusUpdate::AuthRequired { | ||
| StatusUpdate::AuthCompleted { | ||
| extension_name: pending.extension_name.clone(), | ||
| instructions: Some(msg.clone()), | ||
| auth_url: result.auth_url().map(String::from), | ||
| setup_url: result.setup_url().map(String::from), | ||
| success: true, | ||
| message: result.message.clone(), | ||
| }, | ||
| &message.metadata, | ||
| ) | ||
| .await; | ||
| Ok(Some(msg)) | ||
| Ok(Some(result.message)) |
There was a problem hiding this comment.
process_auth_token() reports AuthCompleted { success: true } for any Ok(ConfigureResult), but configure_token() can return activated: false when activation fails. This can incorrectly mark auth as successful and exit auth mode even though the extension still isn’t usable. Consider setting success based on result.activated and deciding whether to keep/re-enter auth mode when activated is false.
| if !self | ||
| .secrets | ||
| .exists(&self.user_id, &s.name) | ||
| .await | ||
| .unwrap_or(false) | ||
| { |
There was a problem hiding this comment.
configure_token() uses .exists(...).await.unwrap_or(false) when selecting which secret to populate. If the secrets backend errors, this silently treats it as “missing” and may pick the wrong secret (and then fail later in a less clear way). Consider propagating SecretsStore::exists errors (or returning an ExtensionError) instead of swallowing them.
| return Ok(ConfigureResult { | ||
| message, | ||
| activated: true, | ||
| auth_url, | ||
| missing_secrets: vec![], | ||
| }); |
There was a problem hiding this comment.
ConfigureResult includes missing_secrets, but all current return paths set it to vec![] regardless of whether activation failed. This makes the field/documentation misleading and prevents callers from guiding users through multi-step setup. Consider either populating missing_secrets (e.g., by reusing the same missing-secret detection used in auth_wasm_channel_status) or removing the field until it’s supported.
| match ext_mgr.configure_token(&extension_name, &token).await { | ||
| Ok(result) => { | ||
| crate::channels::web::server::clear_auth_mode(state).await; | ||
| state | ||
| .sse | ||
| .broadcast(crate::channels::web::types::SseEvent::AuthCompleted { | ||
| extension_name, | ||
| success: true, | ||
| message: msg, | ||
| }); | ||
| } | ||
| Ok(result) => { | ||
| state | ||
| .sse | ||
| .broadcast(crate::channels::web::types::SseEvent::AuthRequired { | ||
| extension_name, | ||
| instructions: result.instructions().map(String::from), | ||
| auth_url: result.auth_url().map(String::from), | ||
| setup_url: result.setup_url().map(String::from), | ||
| message: result.message, | ||
| }); |
There was a problem hiding this comment.
configure_token() can return Ok(ConfigureResult { activated: false, .. }) when secrets were stored but hot-activation failed. This handler always broadcasts AuthCompleted { success: true } and clears auth mode on any Ok, which can incorrectly report success and prevent retry. Consider setting success based on result.activated (or broadcasting AuthRequired/keeping auth mode when activated is false).
| match ext_mgr | ||
| .configure_token(&req.extension_name, &req.token) | ||
| .await | ||
| .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; | ||
|
|
||
| if result.is_authenticated() { | ||
| // Auto-activate so tools are available immediately | ||
| let msg = match ext_mgr.activate(&req.extension_name).await { | ||
| Ok(r) => format!( | ||
| "{} authenticated ({} tools loaded)", | ||
| req.extension_name, | ||
| r.tools_loaded.len() | ||
| ), | ||
| Err(e) => format!( | ||
| "{} authenticated but activation failed: {}", | ||
| req.extension_name, e | ||
| ), | ||
| }; | ||
|
|
||
| // Clear auth mode on the active thread | ||
| clear_auth_mode(&state).await; | ||
| { | ||
| Ok(result) => { | ||
| // Clear auth mode on the active thread | ||
| clear_auth_mode(&state).await; | ||
|
|
||
| state.sse.broadcast(SseEvent::AuthCompleted { | ||
| extension_name: req.extension_name, | ||
| success: true, | ||
| message: msg.clone(), | ||
| }); | ||
| state.sse.broadcast(SseEvent::AuthCompleted { | ||
| extension_name: req.extension_name.clone(), | ||
| success: true, | ||
| message: result.message.clone(), | ||
| }); | ||
|
|
||
| Ok(Json(ActionResponse::ok(msg))) | ||
| } else { | ||
| // Re-emit auth_required for retry | ||
| state.sse.broadcast(SseEvent::AuthRequired { | ||
| extension_name: req.extension_name.clone(), | ||
| instructions: result.instructions().map(String::from), | ||
| auth_url: result.auth_url().map(String::from), | ||
| setup_url: result.setup_url().map(String::from), | ||
| }); | ||
| Ok(Json(ActionResponse::fail( | ||
| result | ||
| .instructions() | ||
| .map(String::from) | ||
| .unwrap_or_else(|| "Invalid token".to_string()), | ||
| ))) | ||
| Ok(Json(ActionResponse::ok(result.message))) | ||
| } |
There was a problem hiding this comment.
configure_token() may return Ok even if activation failed (result.activated == false). This handler always clears auth mode and broadcasts AuthCompleted { success: true } on any Ok, which can misreport authentication success and hide activation/setup failures. Consider using result.activated to set success and only clearing auth mode when fully activated (or re-emitting AuthRequired when not activated).
| match ext_mgr | ||
| .configure_token(&req.extension_name, &req.token) | ||
| .await | ||
| .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; | ||
|
|
||
| if result.is_authenticated() { | ||
| // Auto-activate so tools are available immediately | ||
| let msg = match ext_mgr.activate(&req.extension_name).await { | ||
| Ok(r) => format!( | ||
| "{} authenticated ({} tools loaded)", | ||
| req.extension_name, | ||
| r.tools_loaded.len() | ||
| ), | ||
| Err(e) => format!( | ||
| "{} authenticated but activation failed: {}", | ||
| req.extension_name, e | ||
| ), | ||
| }; | ||
|
|
||
| // Clear auth mode on the active thread | ||
| clear_auth_mode(&state).await; | ||
| { | ||
| Ok(result) => { | ||
| clear_auth_mode(&state).await; | ||
|
|
||
| state.sse.broadcast(SseEvent::AuthCompleted { | ||
| extension_name: req.extension_name, | ||
| success: true, | ||
| message: msg.clone(), | ||
| }); | ||
| state.sse.broadcast(SseEvent::AuthCompleted { | ||
| extension_name: req.extension_name.clone(), | ||
| success: true, | ||
| message: result.message.clone(), | ||
| }); | ||
|
|
||
| Ok(Json(ActionResponse::ok(msg))) | ||
| } else { | ||
| // Re-emit auth_required for retry | ||
| state.sse.broadcast(SseEvent::AuthRequired { | ||
| extension_name: req.extension_name.clone(), | ||
| instructions: result.instructions().map(String::from), | ||
| auth_url: result.auth_url().map(String::from), | ||
| setup_url: result.setup_url().map(String::from), | ||
| }); | ||
| Ok(Json(ActionResponse::fail( | ||
| result | ||
| .instructions() | ||
| .map(String::from) | ||
| .unwrap_or_else(|| "Invalid token".to_string()), | ||
| ))) | ||
| Ok(Json(ActionResponse::ok(result.message))) |
There was a problem hiding this comment.
This endpoint treats any Ok(ConfigureResult) as a successful auth completion (success: true), but configure_token() can succeed at storing secrets while returning activated: false (activation failed). Consider setting success based on result.activated and avoiding clear_auth_mode() when activation didn’t complete, so the UI can retry or show next steps.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -3155,13 +3080,20 @@ impl ExtensionManager { | |||
|
|
|||
| /// Save setup secrets for an extension, validating names against the capabilities schema. | |||
There was a problem hiding this comment.
The configure() doc comment still starts with “Save setup secrets…”, which is now misleading after the rename/unification. Update/remove that first sentence so the comment consistently describes configure() as the single configuration entrypoint.
| /// Save setup secrets for an extension, validating names against the capabilities schema. | |
| /// Configure an extension, validating provided secret names against the capabilities schema. |
| if !self | ||
| .secrets | ||
| .exists(&self.user_id, &s.name) | ||
| .await | ||
| .unwrap_or(false) | ||
| { |
There was a problem hiding this comment.
secrets.exists(...).await.unwrap_or(false) silently treats secrets-store errors as “secret missing”. If the store is unavailable, configure_token() can pick the wrong target secret (or overwrite unexpectedly) instead of surfacing an infrastructure error. Prefer propagating the SecretError (map to ExtensionError) rather than defaulting to false.
| /// Convenience wrapper: configure a single token for an extension. | ||
| /// | ||
| /// Determines the primary secret name from the extension's capabilities, | ||
| /// then delegates to [`configure()`]. Use this when the caller only has | ||
| /// a bare token value (e.g., from the chat auth card or WebSocket auth). | ||
| pub async fn configure_token( | ||
| &self, | ||
| name: &str, | ||
| token: &str, | ||
| ) -> Result<ConfigureResult, ExtensionError> { |
There was a problem hiding this comment.
New configure() / configure_token() behavior (validation_endpoint handling, multi-secret selection, and ValidationFailed error) isn’t covered by tests in this file. Since src/extensions/manager.rs already has unit tests, please add coverage for at least: (1) multi-secret channel prompting progression (e.g., Discord), and (2) ValidationFailed vs transport errors classification.
| match ext_mgr.configure_token(&extension_name, &token).await { | ||
| Ok(result) => { | ||
| crate::channels::web::server::clear_auth_mode(state).await; | ||
| state | ||
| .sse | ||
| .broadcast(crate::channels::web::types::SseEvent::AuthCompleted { | ||
| extension_name, | ||
| success: true, | ||
| message: msg, | ||
| }); | ||
| } | ||
| Ok(result) => { | ||
| state | ||
| .sse | ||
| .broadcast(crate::channels::web::types::SseEvent::AuthRequired { | ||
| extension_name, | ||
| instructions: result.instructions().map(String::from), | ||
| auth_url: result.auth_url().map(String::from), | ||
| setup_url: result.setup_url().map(String::from), | ||
| message: result.message, | ||
| }); | ||
| } |
There was a problem hiding this comment.
configure_token() can return Ok(ConfigureResult) even when further user action is required (e.g., multi-secret channels like Discord/Slack still missing another secret, or tools returning an OAuth auth_url). This handler always clears auth mode and emits AuthCompleted on any Ok, which can prematurely dismiss the auth UI and block completion. Consider branching on result.activated / result.auth_url (and/or missing_secrets) and emitting AuthRequired when additional steps are needed.
| match ext_mgr | ||
| .configure_token(&pending.extension_name, token) | ||
| .await | ||
| { | ||
| Ok(result) => { | ||
| tracing::info!( | ||
| "Extension '{}' authenticated via auth mode", | ||
| pending.extension_name | ||
| "Extension '{}' configured via auth mode: {}", | ||
| pending.extension_name, | ||
| result.message | ||
| ); | ||
|
|
||
| // Auto-activate so tools are available immediately after auth | ||
| match ext_mgr.activate(&pending.extension_name).await { | ||
| Ok(activate_result) => { | ||
| let tool_count = activate_result.tools_loaded.len(); | ||
| let tool_list = if activate_result.tools_loaded.is_empty() { | ||
| String::new() | ||
| } else { | ||
| format!("\n\nTools: {}", activate_result.tools_loaded.join(", ")) | ||
| }; | ||
| let msg = format!( | ||
| "{} authenticated and activated ({} tools loaded).{}", | ||
| pending.extension_name, tool_count, tool_list | ||
| ); | ||
| let _ = self | ||
| .channels | ||
| .send_status( | ||
| &message.channel, | ||
| StatusUpdate::AuthCompleted { | ||
| extension_name: pending.extension_name.clone(), | ||
| success: true, | ||
| message: msg.clone(), | ||
| }, | ||
| &message.metadata, | ||
| ) | ||
| .await; | ||
| Ok(Some(msg)) | ||
| } | ||
| Err(e) => { | ||
| tracing::warn!( | ||
| "Extension '{}' authenticated but activation failed: {}", | ||
| pending.extension_name, | ||
| e | ||
| ); | ||
| let msg = format!( | ||
| "{} authenticated successfully, but activation failed: {}. \ | ||
| Try activating manually.", | ||
| pending.extension_name, e | ||
| ); | ||
| let _ = self | ||
| .channels | ||
| .send_status( | ||
| &message.channel, | ||
| StatusUpdate::AuthCompleted { | ||
| extension_name: pending.extension_name.clone(), | ||
| success: true, | ||
| message: msg.clone(), | ||
| }, | ||
| &message.metadata, | ||
| ) | ||
| .await; | ||
| Ok(Some(msg)) | ||
| } | ||
| } | ||
| } | ||
| Ok(result) => { | ||
| // Invalid token, re-enter auth mode | ||
| { | ||
| let mut sess = session.lock().await; | ||
| if let Some(thread) = sess.threads.get_mut(&thread_id) { | ||
| thread.enter_auth_mode(pending.extension_name.clone()); | ||
| } | ||
| } | ||
| let msg = result | ||
| .instructions() | ||
| .map(String::from) | ||
| .unwrap_or_else(|| "Invalid token. Please try again.".to_string()); | ||
| // Re-emit AuthRequired so web UI re-shows the card | ||
| let _ = self | ||
| .channels | ||
| .send_status( | ||
| &message.channel, | ||
| StatusUpdate::AuthRequired { | ||
| StatusUpdate::AuthCompleted { | ||
| extension_name: pending.extension_name.clone(), | ||
| instructions: Some(msg.clone()), | ||
| auth_url: result.auth_url().map(String::from), | ||
| setup_url: result.setup_url().map(String::from), | ||
| success: true, | ||
| message: result.message.clone(), | ||
| }, | ||
| &message.metadata, | ||
| ) | ||
| .await; | ||
| Ok(Some(msg)) | ||
| Ok(Some(result.message)) |
There was a problem hiding this comment.
After configure_token() returns Ok, this code always emits StatusUpdate::AuthCompleted { success: true } and leaves auth mode cleared. For multi-secret extensions or OAuth tools, configuration may be partial (activated == false or auth_url present), and the agent should stay in auth mode / send AuthRequired for the next step instead of reporting completion. Consider branching on the ConfigureResult to either keep prompting (re-enter auth mode) or only send AuthCompleted once activation/auth is truly complete.
| ), | ||
| activated: true, | ||
| auth_url: None, | ||
| missing_secrets: vec![], |
There was a problem hiding this comment.
ConfigureResult.missing_secrets is always returned as an empty vec, even though the repo has multi-secret channel setup schemas (e.g., Discord/Slack). That makes it hard for callers to know whether they should re-prompt for another required secret after a configure_token() call. Consider populating missing_secrets (and/or returning a distinct result) by re-checking the capabilities’ required secrets against the secrets store after writes.
| missing_secrets: vec![], | |
| // Surface any secrets that are still missing after activation. | |
| missing_secrets: result.missing_secrets, |
| match ext_mgr | ||
| .configure_token(&req.extension_name, &req.token) | ||
| .await | ||
| .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; | ||
|
|
||
| if result.is_authenticated() { | ||
| // Auto-activate so tools are available immediately | ||
| let msg = match ext_mgr.activate(&req.extension_name).await { | ||
| Ok(r) => format!( | ||
| "{} authenticated ({} tools loaded)", | ||
| req.extension_name, | ||
| r.tools_loaded.len() | ||
| ), | ||
| Err(e) => format!( | ||
| "{} authenticated but activation failed: {}", | ||
| req.extension_name, e | ||
| ), | ||
| }; | ||
|
|
||
| // Clear auth mode on the active thread | ||
| clear_auth_mode(&state).await; | ||
| { | ||
| Ok(result) => { | ||
| clear_auth_mode(&state).await; | ||
|
|
||
| state.sse.broadcast(SseEvent::AuthCompleted { | ||
| extension_name: req.extension_name, | ||
| success: true, | ||
| message: msg.clone(), | ||
| }); | ||
| state.sse.broadcast(SseEvent::AuthCompleted { | ||
| extension_name: req.extension_name.clone(), | ||
| success: true, | ||
| message: result.message.clone(), | ||
| }); | ||
|
|
||
| Ok(Json(ActionResponse::ok(msg))) | ||
| } else { | ||
| // Re-emit auth_required for retry | ||
| state.sse.broadcast(SseEvent::AuthRequired { | ||
| extension_name: req.extension_name.clone(), | ||
| instructions: result.instructions().map(String::from), | ||
| auth_url: result.auth_url().map(String::from), | ||
| setup_url: result.setup_url().map(String::from), | ||
| }); | ||
| Ok(Json(ActionResponse::fail( | ||
| result | ||
| .instructions() | ||
| .map(String::from) | ||
| .unwrap_or_else(|| "Invalid token".to_string()), | ||
| ))) | ||
| Ok(Json(ActionResponse::ok(result.message))) | ||
| } |
There was a problem hiding this comment.
This handler treats any Ok(ConfigureResult) as full success and emits AuthCompleted, but configure_token() can succeed at storing a secret while still requiring more steps (OAuth via auth_url, or another required secret for channels like Discord/Slack). Consider checking result.activated / result.auth_url (and/or missing secrets) and re-emitting AuthRequired when the user still needs to act, rather than always completing the flow.
| @@ -1631,7 +1617,7 @@ async fn extensions_activate_handler( | |||
| // already has a token but missing the documents scope). | |||
| // Initial OAuth setup is triggered via save_setup_secrets. | |||
There was a problem hiding this comment.
This comment still refers to save_setup_secrets, but the setup entrypoint was renamed to configure(). Please update the comment to match the new API to avoid confusion when tracing OAuth/setup flows.
| // Initial OAuth setup is triggered via save_setup_secrets. | |
| // Initial OAuth setup is triggered via configure(). |
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Re-review: All 5 blocking issues addressed
Verified each issue from the previous two reviews:
-
SSRF in validation_endpoint -- Fixed.
validate_fetch_url()is called on the resolved URL before the HTTP request. Token is URL-encoded before template substitution. Correct. -
Transport errors mapped to ValidationFailed -- Fixed. The
.send().awaiterror now maps toExtensionError::Other("Token validation request failed: ..."). Only non-success HTTP status codes produceExtensionError::ValidationFailed. Callers can now correctly distinguish "bad token" from "network failure". -
PR title/description mismatch -- Fixed. Title and description now accurately describe the actual changes.
-
configure_token() first-missing secret selection -- Fixed. The code queries
self.secrets.exists()in a loop over non-optional secrets and picks the first missing one. Falls back to first non-optional if all exist (for overwrite scenarios). Correct for multi-secret channels. -
WebSocket AuthRequired re-emission -- Fixed. The WS error path in
ws.rsnow checksmatches!(e, ExtensionError::ValidationFailed(_))and broadcastsAuthRequired, matching the HTTP handler behavior.
Remaining non-blocking observations (carry forward, not blocking merge)
auth()doc says "no side effects" butauth_mcp()still runs OAuth flows (opens browser, starts callback listener). The doc comment should say "read-only for WASM extensions; may initiate OAuth for MCP servers".missing_secretsalwaysvec![]-- TheMissingSecrettype is defined but never populated. Either populate it to drive multi-step auth UX or remove the dead type. Low priority.- Capabilities file parsed multiple times in
configure()-- Once for allowed names, once for validation_endpoint. Could be loaded once. Minor performance nit. validation_endpointonly checked forWasmChannel-- If aWasmToolcapabilities file declares avalidation_endpoint, it won't be validated. Fine for now since no tools use it, but worth documenting the restriction.
The architecture is clean. Splitting auth() into a read-only status check and configure() as the mutation entrypoint eliminates the code duplication between chat and gateway paths. Callers are significantly simpler. Approving.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Re-review: All 5 blocking issues addressed
Verified each issue from the previous two reviews:
-
SSRF in validation_endpoint -- Fixed. validate_fetch_url() is called on the resolved URL before the HTTP request. Token is URL-encoded before template substitution. Correct.
-
Transport errors mapped to ValidationFailed -- Fixed. The .send().await error now maps to ExtensionError::Other. Only non-success HTTP status codes produce ExtensionError::ValidationFailed. Callers can now correctly distinguish bad token from network failure.
-
PR title/description mismatch -- Fixed. Title and description now accurately describe the actual changes.
-
configure_token() first-missing secret selection -- Fixed. The code queries self.secrets.exists() in a loop over non-optional secrets and picks the first missing one. Falls back to first non-optional if all exist (for overwrite scenarios). Correct for multi-secret channels.
-
WebSocket AuthRequired re-emission -- Fixed. The WS error path in ws.rs now checks matches!(e, ExtensionError::ValidationFailed(_)) and broadcasts AuthRequired, matching the HTTP handler behavior.
Remaining non-blocking observations (carry forward, not blocking merge)
- auth() doc says no side effects but auth_mcp() still runs OAuth flows (opens browser, starts callback listener). The doc comment should say read-only for WASM extensions; may initiate OAuth for MCP servers.
- missing_secrets always vec![]. The MissingSecret type is defined but never populated. Either populate it to drive multi-step auth UX or remove the dead type. Low priority.
- Capabilities file parsed multiple times in configure(). Once for allowed names, once for validation_endpoint. Could be loaded once. Minor performance nit.
- validation_endpoint only checked for WasmChannel. If a WasmTool capabilities file declares a validation_endpoint, it will not be validated. Fine for now since no tools use it, but worth documenting the restriction.
The architecture is clean. Splitting auth() into a read-only status check and configure() as the mutation entrypoint eliminates the code duplication between chat and gateway paths. Callers are significantly simpler.
Refactors the extension lifecycle to eliminate the divergence between chat and gateway paths that caused Telegram setup via chat to fail (missing webhook secret auto-generation, no token validation). Key changes: - Rename save_setup_secrets() → configure(): single entrypoint for providing secrets to any extension (WasmChannel, WasmTool, MCP). Validates, stores, auto-generates, and activates. - Add configure_token(): convenience wrapper for single-token callers (chat auth card, WebSocket, agent auth mode). - Refactor auth() to pure status check: remove token parameter, delete token-storing branches from auth_mcp/auth_wasm_tool, rename auth_wasm_channel → auth_wasm_channel_status. - Add ConfigureResult/MissingSecret types for structured responses. - Replace hardcoded Telegram token validation with generic validation_endpoint from capabilities.json. - Update all callers (9 files) to use the new interface. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace brittle msg.contains("Invalid token") checks with a proper
ExtensionError::ValidationFailed variant. configure() now returns
this variant for token validation failures, and callers match on it
directly instead of parsing error message strings.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…election, WS auth 1. SSRF: call validate_fetch_url() before validation_endpoint HTTP request 2. Transport errors map to ExtensionError::Other (not ValidationFailed) 3. configure_token() picks first *missing* secret, not first non-optional 4. WebSocket error path re-emits AuthRequired on ValidationFailed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
henrypark133
force-pushed
the
unify-extension-lifecycle
branch
from
42f4b61 to
ce106f4
Compare
- test_configure_token_picks_first_missing_secret: verifies multi-secret channels can be configured one secret at a time (commit ce106f4) - test_auth_is_read_only_for_wasm_channel: verifies auth() has no side effects and doesn't store secrets (commit 47f8eb6) - test_validation_failed_is_distinct_error_variant: verifies the typed error variant can be pattern-matched (commit a318161) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| match ext_mgr | ||
| .configure_token(&req.extension_name, &req.token) | ||
| .await | ||
| .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; | ||
|
|
||
| if result.is_authenticated() { | ||
| // Auto-activate so tools are available immediately | ||
| let msg = match ext_mgr.activate(&req.extension_name).await { | ||
| Ok(r) => format!( | ||
| "{} authenticated ({} tools loaded)", | ||
| req.extension_name, | ||
| r.tools_loaded.len() | ||
| ), | ||
| Err(e) => format!( | ||
| "{} authenticated but activation failed: {}", | ||
| req.extension_name, e | ||
| ), | ||
| }; | ||
|
|
||
| // Clear auth mode on the active thread | ||
| clear_auth_mode(&state).await; | ||
| { | ||
| Ok(result) => { | ||
| // Clear auth mode on the active thread | ||
| clear_auth_mode(&state).await; | ||
|
|
||
| state.sse.broadcast(SseEvent::AuthCompleted { | ||
| extension_name: req.extension_name, | ||
| success: true, | ||
| message: msg.clone(), | ||
| }); | ||
| state.sse.broadcast(SseEvent::AuthCompleted { | ||
| extension_name: req.extension_name.clone(), | ||
| success: true, | ||
| message: result.message.clone(), | ||
| }); | ||
|
|
||
| Ok(Json(ActionResponse::ok(msg))) | ||
| } else { | ||
| // Re-emit auth_required for retry | ||
| state.sse.broadcast(SseEvent::AuthRequired { | ||
| extension_name: req.extension_name.clone(), | ||
| instructions: result.instructions().map(String::from), | ||
| auth_url: result.auth_url().map(String::from), | ||
| setup_url: result.setup_url().map(String::from), | ||
| }); | ||
| Ok(Json(ActionResponse::fail( | ||
| result | ||
| .instructions() | ||
| .map(String::from) | ||
| .unwrap_or_else(|| "Invalid token".to_string()), | ||
| ))) | ||
| Ok(Json(ActionResponse::ok(result.message))) | ||
| } | ||
| Err(e) => { | ||
| let msg = e.to_string(); | ||
| // Re-emit auth_required for retry on validation errors | ||
| if matches!(e, crate::extensions::ExtensionError::ValidationFailed(_)) { | ||
| state.sse.broadcast(SseEvent::AuthRequired { | ||
| extension_name: req.extension_name.clone(), | ||
| instructions: Some(msg.clone()), | ||
| auth_url: None, | ||
| setup_url: None, | ||
| }); | ||
| } | ||
| Ok(Json(ActionResponse::fail(msg))) | ||
| } |
There was a problem hiding this comment.
This handler clears auth mode and broadcasts AuthCompleted { success: true } for any Ok from configure_token(), even when result.activated is false. That can dismiss the auth UI even though activation/auth didn’t complete (and prevents retry for channels that don’t have a validation_endpoint). Gate success/clearing auth mode on result.activated (or re-check ext_mgr.auth() and re-emit AuthRequired when still awaiting token/OAuth).
| match ext_mgr | ||
| .configure_token(&req.extension_name, &req.token) | ||
| .await | ||
| .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; | ||
|
|
||
| if result.is_authenticated() { | ||
| // Auto-activate so tools are available immediately | ||
| let msg = match ext_mgr.activate(&req.extension_name).await { | ||
| Ok(r) => format!( | ||
| "{} authenticated ({} tools loaded)", | ||
| req.extension_name, | ||
| r.tools_loaded.len() | ||
| ), | ||
| Err(e) => format!( | ||
| "{} authenticated but activation failed: {}", | ||
| req.extension_name, e | ||
| ), | ||
| }; | ||
|
|
||
| // Clear auth mode on the active thread | ||
| clear_auth_mode(&state).await; | ||
| { | ||
| Ok(result) => { | ||
| clear_auth_mode(&state).await; | ||
|
|
||
| state.sse.broadcast(SseEvent::AuthCompleted { | ||
| extension_name: req.extension_name, | ||
| success: true, | ||
| message: msg.clone(), | ||
| }); | ||
| state.sse.broadcast(SseEvent::AuthCompleted { | ||
| extension_name: req.extension_name.clone(), | ||
| success: true, | ||
| message: result.message.clone(), | ||
| }); | ||
|
|
||
| Ok(Json(ActionResponse::ok(msg))) | ||
| } else { | ||
| // Re-emit auth_required for retry | ||
| state.sse.broadcast(SseEvent::AuthRequired { | ||
| extension_name: req.extension_name.clone(), | ||
| instructions: result.instructions().map(String::from), | ||
| auth_url: result.auth_url().map(String::from), | ||
| setup_url: result.setup_url().map(String::from), | ||
| }); | ||
| Ok(Json(ActionResponse::fail( | ||
| result | ||
| .instructions() | ||
| .map(String::from) | ||
| .unwrap_or_else(|| "Invalid token".to_string()), | ||
| ))) | ||
| Ok(Json(ActionResponse::ok(result.message))) | ||
| } | ||
| Err(e) => { | ||
| let msg = e.to_string(); | ||
| if matches!(e, crate::extensions::ExtensionError::ValidationFailed(_)) { | ||
| state.sse.broadcast(SseEvent::AuthRequired { | ||
| extension_name: req.extension_name.clone(), | ||
| instructions: Some(msg.clone()), | ||
| auth_url: None, | ||
| setup_url: None, | ||
| }); | ||
| } | ||
| Ok(Json(ActionResponse::fail(msg))) | ||
| } |
There was a problem hiding this comment.
configure_token() returning Ok is currently treated as auth completion and clears auth mode, but ConfigureResult can represent partial/incomplete outcomes (activated == false, or OAuth still required). This can dismiss the auth card prematurely and prevents retries. Consider using result.activated and/or calling ext_mgr.auth() after configuration to decide whether to emit AuthCompleted vs AuthRequired and whether to clear auth mode.
| /// Check auth status for an installed extension (read-only, no side effects). | ||
| /// | ||
| /// To provide secrets, use [`configure()`] instead. |
There was a problem hiding this comment.
The auth() method is documented as “read-only, no side effects”, but it can start OAuth flows / allocate a pending auth listener (e.g., MCP/WASM OAuth). That’s a side effect and could mislead callers about when it’s safe to call. Please clarify the docs to reflect the actual behavior (e.g., “does not persist secrets, but may start OAuth/pending auth flows”).
| /// Check auth status for an installed extension (read-only, no side effects). | |
| /// | |
| /// To provide secrets, use [`configure()`] instead. | |
| /// Check auth status for an installed extension. | |
| /// | |
| /// This method does not persist secrets, but it may initiate or manage | |
| /// OAuth / pending-auth flows (for example, MCP or WASM OAuth) and perform | |
| /// in-memory cleanup of expired auth state. Callers should not assume it is | |
| /// side-effect free. | |
| /// | |
| /// To provide or update secrets, use [`configure()`] instead. |
| Ok(result) => { | ||
| tracing::info!( | ||
| "Extension '{}' authenticated via auth mode", | ||
| pending.extension_name | ||
| "Extension '{}' configured via auth mode: {}", | ||
| pending.extension_name, | ||
| result.message | ||
| ); | ||
|
|
||
| // Auto-activate so tools are available immediately after auth | ||
| match ext_mgr.activate(&pending.extension_name).await { | ||
| Ok(activate_result) => { | ||
| let tool_count = activate_result.tools_loaded.len(); | ||
| let tool_list = if activate_result.tools_loaded.is_empty() { | ||
| String::new() | ||
| } else { | ||
| format!("\n\nTools: {}", activate_result.tools_loaded.join(", ")) | ||
| }; | ||
| let msg = format!( | ||
| "{} authenticated and activated ({} tools loaded).{}", | ||
| pending.extension_name, tool_count, tool_list | ||
| ); | ||
| let _ = self | ||
| .channels | ||
| .send_status( | ||
| &message.channel, | ||
| StatusUpdate::AuthCompleted { | ||
| extension_name: pending.extension_name.clone(), | ||
| success: true, | ||
| message: msg.clone(), | ||
| }, | ||
| &message.metadata, | ||
| ) | ||
| .await; | ||
| Ok(Some(msg)) | ||
| } | ||
| Err(e) => { | ||
| tracing::warn!( | ||
| "Extension '{}' authenticated but activation failed: {}", | ||
| pending.extension_name, | ||
| e | ||
| ); | ||
| let msg = format!( | ||
| "{} authenticated successfully, but activation failed: {}. \ | ||
| Try activating manually.", | ||
| pending.extension_name, e | ||
| ); | ||
| let _ = self | ||
| .channels | ||
| .send_status( | ||
| &message.channel, | ||
| StatusUpdate::AuthCompleted { | ||
| extension_name: pending.extension_name.clone(), | ||
| success: true, | ||
| message: msg.clone(), | ||
| }, | ||
| &message.metadata, | ||
| ) | ||
| .await; | ||
| Ok(Some(msg)) | ||
| } | ||
| } | ||
| } | ||
| Ok(result) => { | ||
| // Invalid token, re-enter auth mode | ||
| { | ||
| let mut sess = session.lock().await; | ||
| if let Some(thread) = sess.threads.get_mut(&thread_id) { | ||
| thread.enter_auth_mode(pending.extension_name.clone()); | ||
| } | ||
| } | ||
| let msg = result | ||
| .instructions() | ||
| .map(String::from) | ||
| .unwrap_or_else(|| "Invalid token. Please try again.".to_string()); | ||
| // Re-emit AuthRequired so web UI re-shows the card | ||
| let _ = self | ||
| .channels | ||
| .send_status( | ||
| &message.channel, | ||
| StatusUpdate::AuthRequired { | ||
| StatusUpdate::AuthCompleted { | ||
| extension_name: pending.extension_name.clone(), | ||
| instructions: Some(msg.clone()), | ||
| auth_url: result.auth_url().map(String::from), | ||
| setup_url: result.setup_url().map(String::from), | ||
| success: true, | ||
| message: result.message.clone(), | ||
| }, | ||
| &message.metadata, | ||
| ) | ||
| .await; | ||
| Ok(Some(msg)) | ||
| Ok(Some(result.message)) | ||
| } |
There was a problem hiding this comment.
On Ok(result) this reports AuthCompleted { success: true } unconditionally, but configure_token() can return activated == false (activation failed / more config required) or require further auth steps. Consider using result.activated (and/or re-checking ext_mgr.auth()) to decide between AuthCompleted vs re-entering auth mode and sending AuthRequired.
| }; | ||
|
|
||
| let mut secrets = std::collections::HashMap::new(); | ||
| secrets.insert(secret_name, token.to_string()); |
There was a problem hiding this comment.
configure_token() forwards token verbatim into the secrets map. Since web/WS callers may include leading/trailing whitespace, consider trimming/normalizing token here before persisting so users don’t accidentally store an invalid credential.
| secrets.insert(secret_name, token.to_string()); | |
| let normalized_token = token.trim().to_string(); | |
| secrets.insert(secret_name, normalized_token); |
| /// For Slack: initiates OAuth flow (redirect-based). | ||
| /// For Telegram: accepts a bot token, registers it with channel-relay, | ||
| /// and stores the returned stream token. |
There was a problem hiding this comment.
The doc comment says channel-relay auth supports Telegram token registration, but the implementation here only initiates Slack OAuth (Slack callback path and RelayProvider::Slack). Since this function signature changed in this PR, please update the docs (and/or implement the Telegram branch) to avoid misleading behavior expectations.
| /// For Slack: initiates OAuth flow (redirect-based). | |
| /// For Telegram: accepts a bot token, registers it with channel-relay, | |
| /// and stores the returned stream token. | |
| /// Currently initiates a redirect-based OAuth flow via the relay service | |
| /// (Slack-backed), storing a CSRF nonce for the OAuth state parameter and | |
| /// returning a URL for the user to authorize. |
| } | ||
| Ok(Json(ActionResponse::fail(msg))) |
There was a problem hiding this comment.
This handler used to surface extension-manager failures as HTTP 5xx, but now all errors from configure_token() return 200 OK with ActionResponse::fail. That makes infra/config errors indistinguishable from user input errors for clients. Consider mapping non-validation failures to an appropriate HTTP error status (and keep 200 only for expected “invalid token” style cases).
| } | |
| Ok(Json(ActionResponse::fail(msg))) | |
| // Validation errors are expected user input problems: return 200 with a failure payload. | |
| Ok(Json(ActionResponse::fail(msg))) | |
| } else { | |
| // Non-validation errors indicate infra/configuration problems: surface as HTTP 5xx. | |
| Err((StatusCode::INTERNAL_SERVER_ERROR, msg)) | |
| } |
| } | ||
| Ok(Json(ActionResponse::fail(msg))) |
There was a problem hiding this comment.
Errors from configure_token() are always returned as 200 OK with ActionResponse::fail, whereas this handler previously returned HTTP errors for execution failures. Consider preserving an HTTP error status for infrastructure/config failures so clients can distinguish “invalid token” from server-side issues.
| } | |
| Ok(Json(ActionResponse::fail(msg))) | |
| Ok(Json(ActionResponse::fail(msg))) | |
| } else { | |
| Err((StatusCode::INTERNAL_SERVER_ERROR, msg)) | |
| } |
…onsolidation - Fix configure() fallthrough bug: dispatch activation by ExtensionKind instead of unconditionally calling activate_wasm_channel() for all non-WasmTool types (MCP servers and channel relays now use their correct activation methods) - Remove dead MissingSecret struct and missing_secrets field (never populated, flagged by reviewer) - Consolidate capabilities file parsing in configure(): parse once and reuse for allowed names, validation_endpoint, and auto-generation - Fix auth() doc comment: note MCP OAuth side effects - Fix stale save_setup_secrets reference in server.rs comment - Add regression test for activation dispatch bug Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Re-review (round 3): APPROVE
All 5 blocking issues from my previous two reviews are verified fixed:
- SSRF in validation_endpoint --
validate_fetch_url()called before HTTP request - String matching for errors --
ExtensionError::ValidationFailedvariant withmatches!()in all callers - Transport errors mistyped --
reqwestfailures now map toExtensionError::Other - configure_token() wrong secret -- iterates to find first missing non-optional secret
- Activation dispatch bug -- dispatches by
ExtensionKind(latest commit c175396)
Architecture is clean: auth() is read-only, configure() is the single mutation entrypoint, dead code removed, capabilities parsed once. 4 regression tests cover the critical invariants.
…arai#677) * feat(extensions): unify auth and configure into single entrypoint Refactors the extension lifecycle to eliminate the divergence between chat and gateway paths that caused Telegram setup via chat to fail (missing webhook secret auto-generation, no token validation). Key changes: - Rename save_setup_secrets() → configure(): single entrypoint for providing secrets to any extension (WasmChannel, WasmTool, MCP). Validates, stores, auto-generates, and activates. - Add configure_token(): convenience wrapper for single-token callers (chat auth card, WebSocket, agent auth mode). - Refactor auth() to pure status check: remove token parameter, delete token-storing branches from auth_mcp/auth_wasm_tool, rename auth_wasm_channel → auth_wasm_channel_status. - Add ConfigureResult/MissingSecret types for structured responses. - Replace hardcoded Telegram token validation with generic validation_endpoint from capabilities.json. - Update all callers (9 files) to use the new interface. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: use ValidationFailed error variant instead of string matching Replace brittle msg.contains("Invalid token") checks with a proper ExtensionError::ValidationFailed variant. configure() now returns this variant for token validation failures, and callers match on it directly instead of parsing error message strings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review — SSRF protection, error typing, missing-secret selection, WS auth 1. SSRF: call validate_fetch_url() before validation_endpoint HTTP request 2. Transport errors map to ExtensionError::Other (not ValidationFailed) 3. configure_token() picks first *missing* secret, not first non-optional 4. WebSocket error path re-emits AuthRequired on ValidationFailed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: add regression tests for extension lifecycle refactoring - test_configure_token_picks_first_missing_secret: verifies multi-secret channels can be configured one secret at a time (commit ce106f4) - test_auth_is_read_only_for_wasm_channel: verifies auth() has no side effects and doesn't store secrets (commit 47f8eb6) - test_validation_failed_is_distinct_error_variant: verifies the typed error variant can be pattern-matched (commit a318161) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address review comments — activation dispatch, dead code, caps consolidation - Fix configure() fallthrough bug: dispatch activation by ExtensionKind instead of unconditionally calling activate_wasm_channel() for all non-WasmTool types (MCP servers and channel relays now use their correct activation methods) - Remove dead MissingSecret struct and missing_secrets field (never populated, flagged by reviewer) - Consolidate capabilities file parsing in configure(): parse once and reuse for allowed names, validation_endpoint, and auto-generation - Fix auth() doc comment: note MCP OAuth side effects - Fix stale save_setup_secrets reference in server.rs comment - Add regression test for activation dispatch bug Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…arai#677) * feat(extensions): unify auth and configure into single entrypoint Refactors the extension lifecycle to eliminate the divergence between chat and gateway paths that caused Telegram setup via chat to fail (missing webhook secret auto-generation, no token validation). Key changes: - Rename save_setup_secrets() → configure(): single entrypoint for providing secrets to any extension (WasmChannel, WasmTool, MCP). Validates, stores, auto-generates, and activates. - Add configure_token(): convenience wrapper for single-token callers (chat auth card, WebSocket, agent auth mode). - Refactor auth() to pure status check: remove token parameter, delete token-storing branches from auth_mcp/auth_wasm_tool, rename auth_wasm_channel → auth_wasm_channel_status. - Add ConfigureResult/MissingSecret types for structured responses. - Replace hardcoded Telegram token validation with generic validation_endpoint from capabilities.json. - Update all callers (9 files) to use the new interface. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: use ValidationFailed error variant instead of string matching Replace brittle msg.contains("Invalid token") checks with a proper ExtensionError::ValidationFailed variant. configure() now returns this variant for token validation failures, and callers match on it directly instead of parsing error message strings. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review — SSRF protection, error typing, missing-secret selection, WS auth 1. SSRF: call validate_fetch_url() before validation_endpoint HTTP request 2. Transport errors map to ExtensionError::Other (not ValidationFailed) 3. configure_token() picks first *missing* secret, not first non-optional 4. WebSocket error path re-emits AuthRequired on ValidationFailed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: add regression tests for extension lifecycle refactoring - test_configure_token_picks_first_missing_secret: verifies multi-secret channels can be configured one secret at a time (commit ce106f4) - test_auth_is_read_only_for_wasm_channel: verifies auth() has no side effects and doesn't store secrets (commit 47f8eb6) - test_validation_failed_is_distinct_error_variant: verifies the typed error variant can be pattern-matched (commit a318161) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address review comments — activation dispatch, dead code, caps consolidation - Fix configure() fallthrough bug: dispatch activation by ExtensionKind instead of unconditionally calling activate_wasm_channel() for all non-WasmTool types (MCP servers and channel relays now use their correct activation methods) - Remove dead MissingSecret struct and missing_secrets field (never populated, flagged by reviewer) - Consolidate capabilities file parsing in configure(): parse once and reuse for allowed names, validation_endpoint, and auto-generation - Fix auth() doc comment: note MCP OAuth side effects - Fix stale save_setup_secrets reference in server.rs comment - Add regression test for activation dispatch bug Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Refactors the extension lifecycle to eliminate divergence between chat and gateway auth paths.
configure()entrypoint: Renamesave_setup_secrets()→configure(). All secret storage flows through one method that validates, stores, auto-generates webhook secrets, and activates. Handles WasmChannel, WasmTool, and McpServer.configure_token()convenience wrapper: For single-token callers (chat auth card, WebSocket, agent auth mode). Picks the first missing non-optional secret.auth()is now read-only: Removedtokenparameter. Pure status check — no secret storage, no side effects. Callers useconfigure()to provide secrets.validation_endpoint: Replaced hardcoded Telegram token validation with capabilities-driven URL template. Any channel can declare validation in its capabilities.json.ExtensionError::ValidationFailed: Typed error variant for token validation failures. Callers match on variant instead of string contents.validate_fetch_url()called before validation_endpoint HTTP requests.AuthRequiredonValidationFailed(matching HTTP handler).Problem
Chat auth path (
tool_auth→auth_wasm_channel()) stored tokens directly without validation, auto-generation of webhook secrets, or full hot activation. The Extensions tab used a separate method (save_setup_secrets()) that did all of these. Result: Telegram setup via chat didn't work properly.Test plan
cargo clippy --all --all-features— zero warningscargo test extensions::manager::tests— 12 tests passcargo test— full suite passes🤖 Generated with Claude Code