fix(tunnel): preserve resolved cloudflare account id across feature start#62
fix(tunnel): preserve resolved cloudflare account id across feature start#62
Conversation
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 improves the robustness and user experience of tunnel management and configuration within the system. It addresses issues related to configuration file persistence across feature worktrees, enhances the handling of tunnel provider states, and introduces automated Cloudflare account ID resolution during initialization, reducing manual intervention and potential configuration errors. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 several improvements to tunnel configuration handling, including robust fallback logic for unsupported tunnel providers in feature.rs, automatic resolution of Cloudflare account IDs during init --update in init.rs, and correct exclusion of BranchBox configuration files from git stash during feature creation. However, a critical security vulnerability was identified in the new read_env_file_value function in core/src/workflows/init.rs. This function follows symlinks and absolute paths, which could allow a malicious repository to read sensitive files or cause a Denial of Service (DoS). It is recommended to implement path validation and symlink checks before reading files from user-controlled paths. Additionally, I've provided a couple of suggestions in the specific comments to improve code clarity and efficiency.
| return Ok(None); | ||
| } | ||
|
|
||
| let content = fs::read_to_string(path)?; |
There was a problem hiding this comment.
The read_env_file_value function reads a file from a path specified in the repository's configuration (.branchbox/config.json) without validating that the path is safe. An attacker can point the api_token_path to an absolute location (e.g., /etc/shadow) or a symlink to a sensitive file by committing a malicious configuration file to the repository. While the tool only extracts values for specific keys (like CLOUDFLARE_API_TOKEN), this can lead to partial information leakage if the file contains keys the tool is looking for. Furthermore, pointing the path to a special device like /dev/zero or a named pipe (FIFO) can cause the tool to hang or consume excessive memory, leading to a Denial of Service (DoS).
Remediation: Validate that the path is within the workspace and is not a symlink or a special device before attempting to read it. Use fs::symlink_metadata to check the file type and ensure it is a regular file.
| let provider_name = stored_provider | ||
| .as_deref() | ||
| .and_then(Self::normalize_tunnel_provider_name) | ||
| .map(str::to_string) | ||
| .unwrap_or_else(|| configured_provider.clone()); | ||
|
|
||
| if let Some(stored) = stored_provider.as_ref() { | ||
| if Self::normalize_tunnel_provider_name(stored).is_none() { | ||
| warnings.push(format!( | ||
| "Stored tunnel provider '{}' is unsupported; using configured provider '{}'", | ||
| stored, configured_provider | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic to determine the provider_name and issue a warning for unsupported providers can be simplified for better readability and to avoid calling normalize_tunnel_provider_name twice. You can combine the provider resolution and warning logic into a single block.
let provider_name = if let Some(stored) = &stored_provider {
if let Some(normalized) = Self::normalize_tunnel_provider_name(stored) {
normalized.to_string()
} else {
warnings.push(format!(
"Stored tunnel provider '{}' is unsupported; using configured provider '{}'",
stored, &configured_provider
));
configured_provider.clone()
}
} else {
configured_provider.clone()
};| for line in content.lines() { | ||
| if let Some(value) = line.strip_prefix(&format!("{key}=")) { | ||
| let normalized = value.trim().trim_matches(['"', '\'']).trim().to_string(); | ||
| if !normalized.is_empty() { | ||
| return Ok(Some(normalized)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The format!("{key}=") is being called inside the loop on every iteration. This can be slightly inefficient for large files. You can create the prefix string once before the loop.
| for line in content.lines() { | |
| if let Some(value) = line.strip_prefix(&format!("{key}=")) { | |
| let normalized = value.trim().trim_matches(['"', '\'']).trim().to_string(); | |
| if !normalized.is_empty() { | |
| return Ok(Some(normalized)); | |
| } | |
| } | |
| } | |
| let prefix = format!("{key}="); | |
| for line in content.lines() { | |
| if let Some(value) = line.strip_prefix(&prefix) { | |
| let normalized = value.trim().trim_matches(['"', '\'']).trim().to_string(); | |
| if !normalized.is_empty() { | |
| return Ok(Some(normalized)); | |
| } | |
| } | |
| } |
1 participant
Summary
.branchbox/config.jsonand.branchbox/registry.jsonout of the feature-start stash path soinit --updateconfig repairs are not moved into feature worktreestunnel openignore stale unsupported stored providers (like legacymanual) and fall back to configured providerinit --updateto resolve${CLOUDFLARE_ACCOUNT_ID}from env, secure file, or Cloudflare API account lookup when unambiguousValidation
cargo fmt --allSDKROOT="$(xcrun --sdk macosx --show-sdk-path)" cargo test -p worktree-core tunnel_open_falls_back_to_configured_provider_when_stored_provider_is_unsupportedSDKROOT="$(xcrun --sdk macosx --show-sdk-path)" cargo test -p worktree-core test_execute_update_mode_resolves_cloudflare_account_id_from_api_tokenSDKROOT="$(xcrun --sdk macosx --show-sdk-path)" cargo test -p worktree-core feature_start_keeps_branchbox_config_changes_in_main_worktreeSDKROOT="$(xcrun --sdk macosx --show-sdk-path)" cargo build -p branchbox-cli