fix: proxy host filtering and credential resolution for sandboxed#215
fix: proxy host filtering and credential resolution for sandboxed#215
Conversation
profiles Sandbox profiles had no network_profile set, so the proxy started with an empty host allowlist and blocked all traffic. Credential keys were hardcoded per-provider instead of defaulting to the service name. - Set network_profile on all sandbox profiles in policy.json - Consolidate duplicate network profiles into single "developer" profile - Default credential_key to service name (keychain account matches credential) - Remove hardcoded credential_key from all built-in credential definitions - Expand --proxy-allow group names to their constituent hosts - Downgrade missing Proxy-Authorization to debug (Node.js undici omits it) - Make CONNECT auth non-fatal since host filtering is the real security Signed-off-by: Luke Hinds <lukehinds@gmail.com>
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 refines the network proxy and credential management within sandboxed environments. It addresses issues where sandbox profiles lacked proper network configurations, leading to blocked traffic, and streamlines credential resolution by defaulting keys to service names. Additionally, it enhances the flexibility of proxy allowlists and makes CONNECT proxy authentication more robust by tolerating missing authorization headers from certain clients, ultimately improving the reliability and usability of network interactions for sandboxed operations. 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 enhances proxy functionality for sandboxed profiles by ensuring network profiles are set, consolidating duplicated network profiles, improving credential resolution through defaulting credential_key to the service name, and expanding --proxy-allow group names. However, a critical security concern has been identified: making CONNECT authentication non-fatal introduces a high-severity authentication bypass. This change weakens the proxy's security model by allowing unauthorized local processes to utilize the proxy tunnel without proper session token validation. Additionally, a suggestion has been made to reduce code duplication.
| /// Expand `--proxy-allow` entries: if an entry matches a group name in the | ||
| /// network policy, expand it to the group's hosts and suffixes. Otherwise | ||
| /// treat it as a literal hostname. | ||
| pub fn expand_proxy_allow(policy: &NetworkPolicy, entries: &[String]) -> Vec<String> { | ||
| let mut result = Vec::new(); | ||
| for entry in entries { | ||
| if let Some(group) = policy.groups.get(entry.as_str()) { | ||
| result.extend(group.hosts.clone()); | ||
| for suffix in &group.suffixes { | ||
| let wildcard = if suffix.starts_with('.') { | ||
| format!("*{}", suffix) | ||
| } else { | ||
| format!("*.{}", suffix) | ||
| }; | ||
| result.push(wildcard); | ||
| } | ||
| } else { | ||
| result.push(entry.clone()); | ||
| } | ||
| } | ||
| result | ||
| } |
There was a problem hiding this comment.
The logic for converting a suffix to a wildcard pattern is also present in the build_proxy_config function (lines 245-251). To improve maintainability and reduce duplication, I've extracted it into a private helper function suffix_to_wildcard.
This change makes expand_proxy_allow cleaner. You could consider updating build_proxy_config to use this new helper function in a follow-up change to fully eliminate the code duplication.
/// Expand `--proxy-allow` entries: if an entry matches a group name in the
/// network policy, expand it to the group's hosts and suffixes. Otherwise
/// treat it as a literal hostname.
pub fn expand_proxy_allow(policy: &NetworkPolicy, entries: &[String]) -> Vec<String> {
let mut result = Vec::new();
for entry in entries {
if let Some(group) = policy.groups.get(entry.as_str()) {
result.extend(group.hosts.clone());
for suffix in &group.suffixes {
result.push(suffix_to_wildcard(suffix));
}
} else {
result.push(entry.clone());
}
}
result
}
/// Converts a suffix string to a wildcard pattern.
fn suffix_to_wildcard(suffix: &str) -> String {
if suffix.starts_with('.') {
format!("*{}
1 participant
profiles
Sandbox profiles had no network_profile set, so the proxy started with an empty host allowlist and blocked all traffic. Credential keys were hardcoded per-provider instead of defaulting to the service name.