Conversation
…_CAPABILITY The perspectives() resolver previously identified "admin" requests by checking whether the token held ALL_CAPABILITY wildcards and had no user email. This meant any regular app token granted ALL_CAPABILITY through the normal auth flow would also receive a full view of every perspective on the node — including ones owned by other users — without being able to do anything meaningful in them. Replace that heuristic with an explicit is_admin_credential flag on RequestContext, set to true only when the incoming token is the launcher's admin_credential (or an empty token in legacy single-user mode). JWT app tokens never have this flag, regardless of which capabilities they hold. Agents that are not the launcher now only see perspectives they own or have explicitly joined via a neighbourhood.
Contributor
📝 WalkthroughWalkthroughThis PR introduces admin credential token verification across the executor. It adds a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant GraphQLHandler as GraphQL Handler
participant AdminCheck as Admin Credential Check
participant RequestCtx as RequestContext
participant Resolver as Query Resolver
Client->>GraphQLHandler: HTTP Request + auth_header
GraphQLHandler->>AdminCheck: is_admin_credential_token(auth_header, admin_credential)
AdminCheck-->>GraphQLHandler: bool (true/false)
GraphQLHandler->>RequestCtx: Create RequestContext with is_admin_credential flag
GraphQLHandler->>Resolver: Execute query with RequestContext
alt is_admin_credential = true
Resolver->>Resolver: Return all perspectives (full overview)
else is_admin_credential = false
Resolver->>Resolver: Filter perspectives by ownership
end
Resolver-->>Client: Response
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust-executor/src/graphql/query_resolvers.rs (1)
440-464:⚠️ Potential issue | 🟠 MajorThe single-perspective resolver doesn't honor
is_admin_credential.The
perspectives()list resolver (line 566) grants the launcher admin full visibility, but theperspective(uuid)resolver (line 440) still filters viacan_access_perspectivewithout checkingis_admin_credential. This means an admin can discover a perspective UUID in the list but receiveNonewhen querying it directly.Apply the same admin bypass to the single-perspective resolver:
Suggested fix
if let Some(p) = get_perspective(&uuid) { let handle = p.persisted.lock().await.clone(); + // Admin (launcher) can access any perspective + if context.is_admin_credential { + return Ok(Some(handle)); + } + // Check if user has access to this perspective let user_email = user_email_from_token(context.auth_token.clone()); if can_access_perspective(&user_email, &handle) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-executor/src/graphql/query_resolvers.rs` around lines 440 - 464, The perspective resolver currently always enforces can_access_perspective, causing admin users to be blocked; update the async fn perspective to extract the user_email via user_email_from_token(context.auth_token.clone()) and check is_admin_credential(&user_email) before calling can_access_perspective (i.e., if is_admin_credential returns true, return Ok(Some(handle)) without further checks). Ensure you keep the existing capability check (check_capability) and the get_perspective/handle logic but short-circuit permission evaluation for admins so admin users see the perspective the same way perspectives() does.
🧹 Nitpick comments (1)
rust-executor/src/agent/capabilities/mod.rs (1)
37-45: Consider deduplicating withcapabilities_from_tokenand adding unit tests.The admin-credential matching logic here (lines 40-44) is identical to the first block of
capabilities_from_token(lines 264-276). You could havecapabilities_from_tokencallis_admin_credential_tokeninternally to keep the admin-check logic in one place:♻️ Suggested deduplication in capabilities_from_token
pub fn capabilities_from_token( token: String, admin_credential: Option<String>, ) -> Result<Vec<Capability>, String> { - match admin_credential { - Some(admin_credential) => { - // Use constant-time comparison to prevent timing attacks - if constant_time_eq(&token, &admin_credential) { - return Ok(vec![ALL_CAPABILITY.clone()]); - } - } - None => { - if token.is_empty() { - return Ok(vec![ALL_CAPABILITY.clone()]); - } - } + if is_admin_credential_token(&token, &admin_credential) { + return Ok(vec![ALL_CAPABILITY.clone()]); }Also, since this is a security-critical function, consider adding unit tests:
🧪 Suggested tests
#[test] fn admin_credential_token_matches() { let cred = Some("secret123".to_string()); assert!(is_admin_credential_token("secret123", &cred)); assert!(!is_admin_credential_token("wrong", &cred)); assert!(!is_admin_credential_token("", &cred)); } #[test] fn admin_credential_none_legacy_mode() { assert!(is_admin_credential_token("", &None)); assert!(!is_admin_credential_token("some-token", &None)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust-executor/src/agent/capabilities/mod.rs` around lines 37 - 45, The admin-check logic in is_admin_credential_token is duplicated in capabilities_from_token; refactor capabilities_from_token to call is_admin_credential_token(token, admin_credential) for the admin branch to centralize matching logic, remove the duplicate block, and keep behavior identical (constant-time compare when Some, empty token allowed when None). After refactoring, add unit tests for is_admin_credential_token (e.g., matching, non-matching, empty token with Some, and legacy None behavior) to cover security cases and prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@rust-executor/src/graphql/query_resolvers.rs`:
- Around line 440-464: The perspective resolver currently always enforces
can_access_perspective, causing admin users to be blocked; update the async fn
perspective to extract the user_email via
user_email_from_token(context.auth_token.clone()) and check
is_admin_credential(&user_email) before calling can_access_perspective (i.e., if
is_admin_credential returns true, return Ok(Some(handle)) without further
checks). Ensure you keep the existing capability check (check_capability) and
the get_perspective/handle logic but short-circuit permission evaluation for
admins so admin users see the perspective the same way perspectives() does.
---
Nitpick comments:
In `@rust-executor/src/agent/capabilities/mod.rs`:
- Around line 37-45: The admin-check logic in is_admin_credential_token is
duplicated in capabilities_from_token; refactor capabilities_from_token to call
is_admin_credential_token(token, admin_credential) for the admin branch to
centralize matching logic, remove the duplicate block, and keep behavior
identical (constant-time compare when Some, empty token allowed when None).
After refactoring, add unit tests for is_admin_credential_token (e.g., matching,
non-matching, empty token with Some, and legacy None behavior) to cover security
cases and prevent regressions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The perspectives() resolver previously identified "admin" requests by checking whether the token held ALL_CAPABILITY wildcards and had no user email. This meant any regular app token granted ALL_CAPABILITY through the normal auth flow would also receive a full view of every perspective on the node — including ones owned by other users — without being able to do anything meaningful in them.
Replace that heuristic with an explicit is_admin_credential flag on RequestContext, set to true only when the incoming token is the launcher's admin_credential (or an empty token in legacy single-user mode). JWT app tokens never have this flag, regardless of which capabilities they hold. Agents that are not the launcher now only see perspectives they own or have explicitly joined via a neighbourhood.
Summary by CodeRabbit