Add restricted proxy mode with directory allowlist #280
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![chatgpt-codex-connector[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| Log { .. } | Sync => GuardEvaluation::allow(), | ||
| } |
There was a problem hiding this comment.
When an allowed directory is configured the proxy is supposed to block all reads and writes outside that subtree, but ProxyAccess::evaluate still unconditionally allows ProxyRequest::Sync. A Sync job invokes the backend’s global synchronization routine (ProxyServer::handle ➜ db.sync_job()), which updates the entire notebook regardless of directory boundaries. A restricted client can therefore trigger writes that touch directories outside the allowlisted subtree, contradicting the README’s promise that “reads and writes outside the subtree are rejected”. Consider rejecting Sync (or scoping it) whenever an allowlist is active.
Useful? React with 👍 / 👎.
Glues Coverage Report
|
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
README.md (1)
56-56: Clarify how to obtain and add a quick example.Suggest adding 1–2 sentences on how users can get a directory ID (e.g., via RootId and navigations, or from the TUI/devtools) and a concrete startup example to reduce ambiguity.
server/tests/proxy_access.rs (1)
48-181: Add a positive-read and a “no allowlist” control test.
- Include one success path that reads note content inside the allowed subtree to validate allowed reads (not only writes/mutations).
- Add a small test with allowed_directory=None to ensure the guard is bypassed and existing behavior remains unchanged.
Example additions (new tests):
#[tokio::test] async fn proxy_allows_reads_within_allowed_subtree() { // Arrange similar to main test but: create note under `allowed_dir`, // then assert FetchNoteContent returns Ok. } #[tokio::test] async fn proxy_without_allowlist_is_unrestricted() { let db = Db::memory().await.unwrap(); let proxy = glues_core::backend::proxy::ProxyServer::new(Box::new(db)); let state = ServerState::new(proxy, None).await.unwrap(); // Assert that e.g., RootId equals the real root and outside ops are permitted. }server/src/args.rs (1)
12-13: Avoid accepting an empty auth token.If
GLUES_SERVER_TOKENor--auth-token ""is provided, the server will enforce a blank Bearer token. Reject empty values at parse-time.Apply this diff:
-use { - clap::{Args, Parser, Subcommand}, +use { + clap::{Args, Parser, Subcommand, builder::NonEmptyStringValueParser}, glues_core::types::DirectoryId, std::net::SocketAddr, }; @@ - #[arg(long, env = "GLUES_SERVER_TOKEN")] + #[arg(long, env = "GLUES_SERVER_TOKEN", value_parser = NonEmptyStringValueParser::new())] pub auth_token: Option<String>,Optionally, also derive Debug for easier diagnostics:
-#[derive(Clone, Args)] +#[derive(Clone, Debug, Args)] pub struct ServerArgs { ... } @@ -#[derive(Subcommand, Clone)] +#[derive(Subcommand, Clone, Debug)] pub enum StorageCommand { ... }server/src/state.rs (2)
63-76: Global request serialization may throttle throughput.
self.server: AsyncMutex<ProxyServer>serializes all requests, including reads. If feasible, consider:
- Making
ProxyServerinternally synchronized sohandletakes&self.- Narrowing the lock to only operations that require atomicity (e.g., the collect-removal + remove sequence), letting read ops proceed concurrently.
47-56: Add a debug log on access denial.Helps operators understand why a request was blocked.
- GuardEvaluation::Deny { message } => { - return ProxyResponse::Err(message); - } + GuardEvaluation::Deny { message } => { + tracing::debug!(%message, "proxy access denied"); + return ProxyResponse::Err(message); + }server/src/lib.rs (1)
55-60: Optional: restrict CORS origins via env for deployments.Current
Anyis convenient but permissive. Consider allowing a comma‑separatedGLUES_CORS_ORIGINSto tighten origins in production.Sketch:
let cors = if let Ok(list) = std::env::var("GLUES_CORS_ORIGINS") { let origins = list.split(',').map(|s| s.trim().parse().unwrap()).collect::<Vec<_>>(); CorsLayer::new().allow_origin(origins) .allow_methods(Any).allow_headers(Any) } else { CorsLayer::new().allow_origin(Any) .allow_methods(Any).allow_headers(Any) };server/src/proxy_access.rs (6)
223-248: Make removal traversal cycle-safe and de-duplicate entriesIf directory relations are ever corrupted (cycles, shared subtrees), this loop can revisit nodes indefinitely and duplicate IDs. Add visited sets to prevent infinite traversal and duplicate removals.
Please confirm the DB guarantees a strict tree; if not, apply the patch below.
pub(crate) async fn collect_removal_plan( server: &mut ProxyServer, directory_id: DirectoryId, ) -> Result<RemovalPlan> { - let mut directories = Vec::new(); - let mut notes = Vec::new(); - let mut queue = VecDeque::new(); + let mut directories = Vec::new(); + let mut notes = Vec::new(); + let mut queue = VecDeque::new(); + let mut seen_dirs: HashSet<DirectoryId> = HashSet::new(); + let mut seen_notes: HashSet<NoteId> = HashSet::new(); - queue.push_back(directory_id.clone()); + seen_dirs.insert(directory_id.clone()); + queue.push_back(directory_id.clone()); while let Some(dir_id) = queue.pop_front() { directories.push(dir_id.clone()); let children = server.db.fetch_directories(dir_id.clone()).await?; for child in children { - queue.push_back(child.id.clone()); + if seen_dirs.insert(child.id.clone()) { + queue.push_back(child.id.clone()); + } } let dir_notes = server.db.fetch_notes(dir_id).await?; for note in dir_notes { - notes.push(note.id); + if seen_notes.insert(note.id.clone()) { + notes.push(note.id); + } } } Ok(RemovalPlan { directories, notes }) }
149-159: Avoid relying on Display for IDs in error messagesFormatting with
{id}requires Display for DirectoryId/NoteId; using{:?}(Debug) is safer across types and avoids build breaks if Display isn’t implemented.If Display is guaranteed, feel free to ignore; otherwise apply:
pub(crate) fn deny_directory(id: &DirectoryId) -> GuardEvaluation { GuardEvaluation::Deny { - message: format!("proxy: directory {id} is outside the allowed subtree"), + message: format!("proxy: directory {id:?} is outside the allowed subtree"), } } pub(crate) fn deny_note(id: &NoteId) -> GuardEvaluation { GuardEvaluation::Deny { - message: format!("proxy: note {id} is outside the allowed subtree"), + message: format!("proxy: note {id:?} is outside the allowed subtree"), } }
179-190: Bridge PendingMutation to PostPlan for directory removalGuardEvaluation can carry a pending CollectRemoval, but PostPlan::from_request doesn’t cover RemoveDirectory. Add a helper to convert the pending to a concrete RemovalPlan so callers don’t have to stitch this together.
I can wire state.rs to use this if helpful.
impl PostPlan { pub(crate) fn from_request(request: &ProxyRequest) -> Self { match request { ProxyRequest::AddDirectory { .. } => PostPlan::AddDirectory, ProxyRequest::AddNote { .. } => PostPlan::AddNote, ProxyRequest::RemoveNote { note_id } => PostPlan::RemoveNote { note_id: note_id.clone(), }, _ => PostPlan::None, } } } + +impl PostPlan { + pub(crate) async fn from_guard( + server: &mut ProxyServer, + eval: &GuardEvaluation, + ) -> Result<Self> { + match eval { + GuardEvaluation::Allow { + pending: Some(PendingMutation::CollectRemoval { directory_id }), + } => { + let plan = collect_removal_plan(server, directory_id.clone()).await?; + Ok(PostPlan::RemoveDirectory(plan)) + } + _ => Ok(PostPlan::None), + } + } +}
197-198: Minor: drop thelet _ =no-op bindingYou can just await the call for existence validation; binding to
_is unnecessary.- let _ = server.db.fetch_directory(root.clone()).await?; + server.db.fetch_directory(root.clone()).await?;
208-212: Reduce unnecessary cloning when enqueuing child directoriesMove the ID once and clone only for the set insert, avoiding the second clone.
- for child in children { - if directories.insert(child.id.clone()) { - queue.push_back(child.id.clone()); - } - } + for child in children { + let id = child.id; + if directories.insert(id.clone()) { + queue.push_back(id); + } + }
274-284: Keep allowed_root invariant after removals (debug check)As a guardrail, assert the allowed_root remains tracked even after a removal plan is applied. This catches accidental root removal plans early.
PostPlan::RemoveDirectory(RemovalPlan { directories, notes }) => { if matches!(response, ProxyResponse::Ok(ResultPayload::Unit)) { let mut guard = access.write().await; for directory_id in directories { guard.directories.remove(&directory_id); } for note_id in notes { guard.notes.remove(¬e_id); } + debug_assert!( + guard.directories.contains(&guard.allowed_root), + "allowed_root must remain in the allowed directories set" + ); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)server/src/args.rs(1 hunks)server/src/lib.rs(5 hunks)server/src/proxy_access.rs(1 hunks)server/src/state.rs(1 hunks)server/tests/proxy_access.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runcargo clippy --all-targets -- -D warningsand ensure it passes
Runcargo fmtacross the workspace before committing
Files:
server/src/state.rsserver/src/proxy_access.rsserver/src/args.rsserver/tests/proxy_access.rsserver/src/lib.rs
🧬 Code graph analysis (4)
server/src/state.rs (1)
server/src/proxy_access.rs (5)
apply_post_plan(249-286)collect_removal_plan(223-248)load_proxy_access(192-221)new(25-35)from_request(180-189)
server/src/proxy_access.rs (1)
server/src/state.rs (1)
new(23-32)
server/tests/proxy_access.rs (2)
server/src/proxy_access.rs (1)
new(25-35)server/src/state.rs (1)
new(23-32)
server/src/lib.rs (2)
server/src/args.rs (1)
parse_args(47-49)server/src/state.rs (1)
new(23-32)
🔇 Additional comments (5)
server/tests/proxy_access.rs (1)
12-46: Helper assertions read well and keep the test focused.The expect_* utilities are concise and make failures actionable. LGTM.
server/src/lib.rs (3)
46-49: Nice state wiring and router setup.Backend construction, state init with
allowed_directory, and axum wiring look clean. LGTM.Also applies to: 75-83
107-113: Handler is straightforward and correct for the established JSON envelope.Fine to always return 200 with a typed
ProxyResponse. LGTM.
1-128: Manual verification required: cargo toolchain not accessible in sandbox environment.The sandbox environment lacks necessary system resources (/proc filesystem) to execute cargo commands. Cargo fmt and clippy verification must be performed locally on your machine before merge.
Please run these commands in your local repository and confirm they pass:
cargo fmt --all -- --check cargo clippy --all-targets --all-features -- -D warningsEnsure both commands complete without errors or warnings before merging.
server/src/proxy_access.rs (1)
37-118: ACL evaluation logic is clear and consistentGuards comprehensively cover fetch/mutate operations, root protection looks correct, and note/directory checks are symmetric. Nice.
| let ServerArgs { | ||
| listen, | ||
| auth_token, | ||
| allowed_directory, | ||
| storage, | ||
| } = args; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Treat empty auth tokens as “no auth” to avoid enforcing Bearer (blank).
Filter out empty/whitespace-only tokens at startup.
Apply this diff:
let ServerArgs {
listen,
- auth_token,
+ auth_token,
allowed_directory,
storage,
} = args;
- let backend = build_backend(storage).await?;
+ // Treat empty or whitespace-only tokens as None.
+ let auth_token = auth_token.and_then(|t| {
+ let trimmed = t.trim().to_owned();
+ if trimmed.is_empty() { None } else { Some(trimmed) }
+ });
+
+ let backend = build_backend(storage).await?;No behavior change needed below; the existing if let Some(token) branch will now skip for empty inputs.
Also applies to: 61-69
🤖 Prompt for AI Agents
In server/src/lib.rs around lines 39-44 (and similarly 61-69), the extracted
auth_token should treat empty or whitespace-only strings as None so the existing
if let Some(token) branch won't enforce a blank "Bearer " header; modify the
initialization to trim the auth_token and set it to None when it's empty or only
whitespace (e.g., map Some(s) to s.trim().is_empty() ? None :
Some(s.trim().to_string())), ensuring downstream logic remains unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
56-56: Well-documented feature addition. Line 56 clearly explains the new--allowed-directoryflag, including how to obtain a directory ID and the expected behavior (virtual root, access rejection, root ID exposure). Placement within the Proxy section is logical and accessible to users.Optional enhancement: For developers troubleshooting access issues, consider mentioning that out-of-subtree requests return a 403 error. This is a nice-to-have for future documentation polish and could be added to help users understand expected error behavior.
Summary
--allowed-directory) so the proxy server can run in a restricted mode.Usage Scenario
glues server file --path /path/to/notebook). Use an API call such asRootIdorFetchDirectoriesto capture thedirectory_idyou want to expose, then stop the server.outside the allowed subtree.Summary by CodeRabbit
New Features
Documentation
Tests