feat: Update Cypher Query endpoint to hide nodes/edges based on users ETAC list - BED-6714 #2130
Conversation
…rs environment as hidden
WalkthroughAdds ETAC-aware server-side graph filtering to the CypherQuery handler, integrating a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as CypherQuery Handler
participant Flags as FeatureFlagService
participant Filter as filterETACGraph
participant Model as Graph Processor
participant Response
Client->>Handler: POST /cypherquery (with user context)
Handler->>Flags: GetFlagByKey("targeted_access_control")
Flags-->>Handler: flag (enabled/disabled)
alt ETAC disabled or user has all environments
Handler->>Model: use original graph
else ETAC enabled & user limited
Handler->>Filter: filterETACGraph(originalGraph, request)
Filter->>Model: evaluate node/env membership
Filter-->>Handler: filteredGraph (with hidden placeholders)
end
Handler->>Handler: ensure includeProperties = true (default)
alt includeProperties = false
Handler->>Model: strip node properties from filteredGraph
end
Handler->>Model: processCypherProperties(filteredGraph)
Handler->>Response: serialize graph (nodes include `hidden` flag)
Response-->>Client: JSON result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
cmd/api/src/api/v2/cypherquery.go (2)
180-180: Redundant condition check.The check
if !user.AllEnvironmentsat line 180 is redundant. This code path is only reachable when line 174's condition!etacFlag.Enabled || user.AllEnvironmentsis false, meaninguser.AllEnvironmentsis already guaranteed to befalse.Apply this diff to remove the redundant check:
- if !user.AllEnvironments { - for id, node := range graphResponse.Nodes { + for id, node := range graphResponse.Nodes { include := false for _, key := range environmentKeys { if val, ok := node.Properties[key]; ok { @@ -200,8 +199,7 @@ filteredNodes[id] = model.UnifiedNode{ ... } } - } }
192-204: Minor: Double space in hidden node label whenKindis empty.When
node.Kindis an empty string, the label becomes"** Hidden **"with a double space. Consider trimming or providing a fallback.- Label: fmt.Sprintf("** Hidden %s **", node.Kind), + label := "** Hidden Node **" + if node.Kind != "" { + label = fmt.Sprintf("** Hidden %s **", node.Kind) + }cmd/api/src/api/v2/cypherquery_test.go (2)
467-471: Misleading mock data:Hidden: truein mock input is ignored.The
Hidden: trueon node "2" in the mock return is misleading. ThefilterETACGraphfunction determines hidden status based on environment property matching, not the inputHiddenfield. Consider settingHidden: falsein the mock to accurately represent what the database returns, letting the filter set it."2": { Label: "label2", Properties: map[string]any{"domainsid": "value"}, - Hidden: true, + Hidden: false, },
551-583: Consider adding tests forfilterETACGrapherror paths.The test suite doesn't cover error scenarios in
filterETACGraph:
- When
GetFlagByKeyreturns an error (line 172-173 in cypherquery.go)- When user cannot be extracted from auth context (line 169-171)
These paths would return 500 errors and should be verified.
Would you like me to generate test cases for these error scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/api/src/api/v2/cypherquery.go(3 hunks)cmd/api/src/api/v2/cypherquery_test.go(10 hunks)cmd/api/src/database/mocks/db.go(2 hunks)cmd/api/src/model/unified_graph.go(1 hunks)packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx(1 hunks)packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx:108-148
Timestamp: 2025-09-08T19:01:53.112Z
Learning: In BloodHound's CypherSearch component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx), the sharing state reset for sharedIds and isPublic after the two-step permissions update is handled elsewhere in the codebase, so additional state reset callbacks in the updateQueryPermissions function are not needed.
📚 Learning: 2025-08-28T19:26:03.304Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneAnalysisIcon.tsx:26-26
Timestamp: 2025-08-28T19:26:03.304Z
Learning: In packages/javascript/bh-shared-ui/src/hooks/, useZonePathParams is exported through the useZoneParams barrel (useZoneParams/index.ts exports it via wildcard from useZonePathParams.tsx), and usePrivilegeZoneAnalysis is exported through useConfiguration.ts. Both are available via the main hooks barrel import.
Applied to files:
packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsxpackages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx
📚 Learning: 2025-08-25T20:12:35.629Z
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 1803
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/Summary/SummaryCard.tsx:24-24
Timestamp: 2025-08-25T20:12:35.629Z
Learning: The useHighestPrivilegeTagId hook is available through the hooks barrel export in packages/javascript/bh-shared-ui/src/hooks/index.ts via the wildcard export `export * from './useAssetGroupTags'`. The import `import { useHighestPrivilegeTagId } from '../../../hooks'` works correctly and doesn't cause build failures.
Applied to files:
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx
📚 Learning: 2025-07-22T20:30:34.839Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.
Applied to files:
cmd/api/src/api/v2/cypherquery_test.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
cmd/api/src/api/v2/cypherquery_test.gocmd/api/src/database/mocks/db.go
📚 Learning: 2025-11-05T21:13:08.542Z
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Applied to files:
cmd/api/src/api/v2/cypherquery_test.gocmd/api/src/api/v2/cypherquery.go
📚 Learning: 2025-11-25T22:11:53.518Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 2107
File: cmd/api/src/database/graphschema.go:86-100
Timestamp: 2025-11-25T22:11:53.518Z
Learning: In cmd/api/src/database/graphschema.go, the CreateSchemaEdgeKind method intentionally does not use AuditableTransaction or audit logging because it would create too much noise in the audit log, unlike CreateGraphSchemaExtension which does use auditing.
Applied to files:
cmd/api/src/database/mocks/db.go
🧬 Code graph analysis (3)
cmd/api/src/api/v2/cypherquery_test.go (4)
cmd/api/src/model/appcfg/flag.go (2)
FeatureETAC(43-43)FeatureFlag(50-70)cmd/api/src/queries/graph.go (1)
DefaultQueryFitnessLowerBoundExplore(67-67)cmd/api/src/model/unified_graph.go (3)
UnifiedGraph(37-40)UnifiedEdge(64-71)UnifiedNode(51-61)cmd/api/src/api/v2/cypherquery.go (1)
CypherQueryPayload(41-44)
cmd/api/src/api/v2/cypherquery.go (4)
cmd/api/src/auth/model.go (2)
Context(174-178)GetUserFromAuthCtx(184-191)cmd/api/src/model/unified_graph.go (3)
UnifiedGraph(37-40)UnifiedNode(51-61)UnifiedEdge(64-71)cmd/api/src/model/appcfg/flag.go (1)
FeatureETAC(43-43)cmd/api/src/api/v2/etac.go (1)
ExtractEnvironmentIDsFromUser(63-71)
cmd/api/src/database/mocks/db.go (1)
cmd/api/src/model/graphschema.go (4)
SchemaEdgeKind(75-81)SchemaEdgeKind(83-85)SchemaNodeKind(43-53)SchemaNodeKind(56-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: build-ui
- GitHub Check: run-analysis
🔇 Additional comments (7)
packages/javascript/bh-shared-ui/src/hooks/usePZParams/usePZPathParams.tsx (1)
65-65: LGTM!Formatting-only change adding trailing newline.
packages/javascript/bh-shared-ui/src/components/EntityInfo/EntityInfoPanel.tsx (1)
17-17: LGTM!Import placement cleanup. No behavioral changes.
cmd/api/src/model/unified_graph.go (1)
60-60: LGTM!The
Hiddenfield appropriately defaults tofalseand is explicitly set totrueduring ETAC filtering infilterETACGraph. The lack ofomitemptyensures consistent JSON serialization across all nodes.cmd/api/src/api/v2/cypherquery.go (1)
166-228: ETAC filtering logic is well-structured.The filtering correctly:
- Bypasses filtering when ETAC is disabled or user has
AllEnvironmentsaccess- Checks both
domainsidandtenantidproperties for environment matching- Replaces inaccessible nodes with hidden placeholders preserving graph structure
- Hides edges connected to hidden nodes
Based on learnings, the empty access list behavior (filtering all nodes) is intentional security behavior.
cmd/api/src/api/v2/cypherquery_test.go (1)
552-583: Test structure follows best practices.Each subtest correctly creates its own mock controller, enabling safe parallel execution without shared state issues.
cmd/api/src/database/mocks/db.go (2)
528-556: CreateSchemaEdgeKind/CreateSchemaNodeKind mocks look consistent with Database interface patternsThese create-method mocks (and their recorders) follow the same Helper/Call/type-assert pattern as the rest of the file, and their parameter lists line up with the SchemaEdgeKind/SchemaNodeKind fields and other CreateGraphSchema* methods. Given this file is mockgen‑generated, this block looks correct and requires no manual changes.
1878-1906: GetSchemaEdgeKindById/GetSchemaNodeKindByID mocks correctly mirror other gettersThe new getter mocks and their recorders match the established pattern for Get* methods in this file (ctx + ID in, model type + error out). Argument ordering and type assertions look correct and aligned with the underlying model types.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/api/src/api/v2/cypherquery.go (2)
106-114: Improve observability and messaging around ETAC filtering errors.Right now, any error from
filterETACGraphresults in a 500 with the hard-coded message"error", and the underlying failure (e.g., flag lookup, unexpected state) isn’t logged here:filteredResponse, err := s.filterETACGraph(graphResponse, request) if err != nil { api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusInternalServerError, "error", request), response) return }Consider either:
- Logging the actual error (e.g.,
slog.ErrorContext(request.Context(), "ETAC graph filter failed", "err", err)), and/or- Returning a more specific message while still being safe for clients.
This will make ETAC-related failures far easier to debug in production without changing behavior for callers.
167-229: ETAC graph filtering semantics look sound; consider minor cleanups and confirm empty-access behavior.The core logic—building an access list from the user, hiding non-matching nodes with placeholders, and converting edges touching hidden nodes into hidden-edge placeholders—looks correct and matches the ETAC feature goals.
A few focused suggestions:
Redundant
!user.AllEnvironmentscheck. Inside theelseof!etacFlag.Enabled || user.AllEnvironments, the innerif !user.AllEnvironmentsis always true and only adds an extra nesting level:} else { accessList := ExtractEnvironmentIDsFromUser(&user) environmentKeys := []string{"domainsid", "tenantid"}if !user.AllEnvironments {for id, node := range graphResponse.Nodes {
for id, node := range graphResponse.Nodes { ...
}}}
Dropping the inner condition simplifies the flow without changing behavior.Membership check performance.
slices.Contains(accessList, envStr)is O(len(accessList)) per environment-bearing node. IfaccessListor the graph is large, consider precomputing amap[string]struct{}set:envSet := make(map[string]struct{}, len(accessList)) for _, id := range accessList { envSet[id] = struct{}{} } ... if envStr, ok := val.(string); ok { if _, ok := envSet[envStr]; ok { include = true } }This keeps the rest of the logic intact while making filtering scale better.
Empty access list semantics. When ETAC is enabled and the user has no environment assignments,
accessListwill be empty, so all nodes become hidden placeholders and all edges become hidden edges rather than the graph being fully filtered out. In other ETAC code paths (e.g.,GetAssetGroupComboNode), an empty allowed list is intentionally used to filter out all nodes. Please double-check that this differing behavior is deliberate for Cypher queries (showing a “fully hidden” graph vs. “no results”), for consistency of user experience and expectations. Based on learnings, this difference is worth confirming with product/UX.Overall the function is correct; the above are targeted cleanups and a semantics check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/api/v2/cypherquery.go(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx:108-148
Timestamp: 2025-09-08T19:01:53.112Z
Learning: In BloodHound's CypherSearch component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx), the sharing state reset for sharedIds and isPublic after the two-step permissions update is handled elsewhere in the codebase, so additional state reset callbacks in the updateQueryPermissions function are not needed.
📚 Learning: 2025-11-05T21:13:08.542Z
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Applied to files:
cmd/api/src/api/v2/cypherquery.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: build-ui
- GitHub Check: run-analysis
🔇 Additional comments (1)
cmd/api/src/api/v2/cypherquery.go (1)
19-35: Imports for ETAC filtering look correct.Adding
timeandappcfgis consistent with the new ETAC filtering logic and hidden-node/edge timestamps; no issues here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/api/src/api/v2/cypherquery.go (1)
26-26: Strengthen ETAC graph filtering defensiveness and tighten a few implementation detailsThe overall ETAC masking behavior (hidden nodes with
Kind: "HIDDEN"/Hidden: trueand corresponding hidden edges) is aligned with the rest of the ETAC design and the fail-closed approach from the query layer. A couple of non-blocking improvements could make this more robust and efficient:
Defensive handling for edges whose endpoints are missing from
filteredNodesThe edge pass currently assumes every
edge.Source/edge.Targetexists infilteredNodes:for _, edge := range graphResponse.Edges { if filteredNodes[edge.Target].Hidden || filteredNodes[edge.Source].Hidden { // ... } else { filteredEdges = append(filteredEdges, edge) } }If the DB ever returns an edge referencing a node ID that’s not present in
graphResponse.Nodes(or future refactors change the node filtering semantics), the zero-value lookup will treat that endpoint as not hidden, and you’ll emit an edge whose endpoint doesn’t exist infilteredResponse.Nodes.A more defensive pattern would be to treat missing endpoints as hidden (fail-closed) and avoid relying on implicit zero values:
- for _, edge := range graphResponse.Edges {
if filteredNodes[edge.Target].Hidden || filteredNodes[edge.Source].Hidden {
- for _, edge := range graphResponse.Edges {
src, okSrc := filteredNodes[edge.Source]tgt, okTgt := filteredNodes[edge.Target]if !okSrc || !okTgt || src.Hidden || tgt.Hidden { filteredEdges = append(filteredEdges, model.UnifiedEdge{ Source: edge.Source, Target: edge.Target, Label: "** Hidden Edge **", Kind: "HIDDEN", LastSeen: time.Time{}, Properties: nil, }) } else { filteredEdges = append(filteredEdges, edge) }This keeps the semantics fail-closed even if graph invariants are ever violated, which is desirable for ETAC. Based on learnings, this matches the “empty allowed list → no usable data” behavior used elsewhere in ETAC paths.
Micro-optimizations for node filtering
Not required, but easy wins given this is on the hot path for explore queries:
Pre-size the node map to avoid rehashing:
- filteredNodes := make(map[string]model.UnifiedNode)
- filteredNodes := make(map[string]model.UnifiedNode, len(graphResponse.Nodes))
If
accessListcan grow beyond a handful of entries, consider building a set once instead of callingslices.Containsinside the inner loop:accessList := ExtractEnvironmentIDsFromUser(&user)
accessList := ExtractEnvironmentIDsFromUser(&user)accessSet := make(map[string]struct{}, len(accessList))for _, id := range accessList {accessSet[id] = struct{}{}}@@
if val, ok := node.Properties[key]; ok {if envStr, ok := val.(string); ok && slices.Contains(accessList, envStr) {include = true}}
if val, ok := node.Properties[key]; ok {if envStr, ok := val.(string); ok {if _, ok := accessSet[envStr]; ok {include = true}}}
Minor readability nit
Inside the
elsebranch guarded by!etacFlag.Enabled || user.AllEnvironments, the innerif !user.AllEnvironmentsaround the node loop is redundant (that condition is already guaranteed by the enclosingelse). Dropping that extraifwould slightly simplify the control flow without changing behavior.None of the above are blockers; they’re primarily about making the ETAC filter more defensive and a bit more efficient.
Can you confirm that
RawCypherQueryalways returns a graph where every edge’sSourceandTargetappears in theNodesmap? If there are known exceptions, I’d strongly recommend adopting the defensive edge handling in point (1).Also applies to: 32-32, 173-235
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/api/v2/cypherquery.go(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx:108-148
Timestamp: 2025-09-08T19:01:53.112Z
Learning: In BloodHound's CypherSearch component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx), the sharing state reset for sharedIds and isPublic after the two-step permissions update is handled elsewhere in the codebase, so additional state reset callbacks in the updateQueryPermissions function are not needed.
📚 Learning: 2025-11-05T21:13:08.542Z
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Applied to files:
cmd/api/src/api/v2/cypherquery.go
📚 Learning: 2025-08-27T21:15:32.207Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
Applied to files:
cmd/api/src/api/v2/cypherquery.go
📚 Learning: 2025-09-08T19:01:53.112Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx:108-148
Timestamp: 2025-09-08T19:01:53.112Z
Learning: In BloodHound's CypherSearch component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx), the sharing state reset for sharedIds and isPublic after the two-step permissions update is handled elsewhere in the codebase, so additional state reset callbacks in the updateQueryPermissions function are not needed.
Applied to files:
cmd/api/src/api/v2/cypherquery.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
🔇 Additional comments (1)
cmd/api/src/api/v2/cypherquery.go (1)
94-133: CypherQuery ETAC integration and property-stripping behavior look correctThe updated flow makes sense:
- For both mutation and non-mutation queries you always call the DB with
includeProperties=true, which is required so ETAC can inspect node properties (env IDs).- You then run the unified graph through
filterETACGraphand base the404check on the filtered result, so users with constrained/no environments see masked graphs instead of a misleading 404.- When
payload.IncludePropertiesisfalse, you now clear both node and edge properties before returning, which preserves the external API contract even though the DB always returned properties.Given the previous fixes to the missing
returnand the node-map key usage, the control flow and response-shaping here look solid to me. No changes requested.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/api/src/api/v2/cypherquery.go (2)
187-221: Redundant condition:!user.AllEnvironmentsis always true here.The check on line 187 is unnecessary. At this point in the code, we're in the else branch of line 181 where
!etacFlag.Enabled || user.AllEnvironmentsis false, meaning ETAC is enabled ANDuser.AllEnvironmentsis false.Apply this diff to simplify:
} else { accessList := ExtractEnvironmentIDsFromUser(&user) environmentKeys := []string{"domainsid", "tenantid"} - if !user.AllEnvironments { - for id, node := range graphResponse.Nodes { - include := false - for _, key := range environmentKeys { - if val, ok := node.Properties[key]; ok { - if envStr, ok := val.(string); ok && slices.Contains(accessList, envStr) { - include = true - } + for id, node := range graphResponse.Nodes { + include := false + for _, key := range environmentKeys { + if val, ok := node.Properties[key]; ok { + if envStr, ok := val.(string); ok && slices.Contains(accessList, envStr) { + include = true } } - if include { - filteredNodes[id] = node - } else { - var kind string - if len(node.Kinds) > 0 && node.Kinds[0] != "" { - kind = node.Kinds[0] - } else { - kind = "Unknown" // unknown if no kind - } - - label := fmt.Sprintf("** Hidden %s Object **", kind) - filteredNodes[id] = model.UnifiedNode{ - Label: label, - Kind: "HIDDEN", - Kinds: []string{}, - ObjectId: "HIDDEN", - IsTierZero: false, - IsOwnedObject: false, - LastSeen: time.Time{}, - Properties: nil, - Hidden: true, - } + } + if include { + filteredNodes[id] = node + } else { + var kind string + if len(node.Kinds) > 0 && node.Kinds[0] != "" { + kind = node.Kinds[0] + } else { + kind = "Unknown" + } + + label := fmt.Sprintf("** Hidden %s Object **", kind) + filteredNodes[id] = model.UnifiedNode{ + Label: label, + Kind: "HIDDEN", + Kinds: []string{}, + ObjectId: "HIDDEN", + IsTierZero: false, + IsOwnedObject: false, + LastSeen: time.Time{}, + Properties: nil, + Hidden: true, } } }
225-238: Consider defensive check for edges referencing non-existent nodes.If
edge.Sourceoredge.Targetreferences a node ID not present ingraphResponse.Nodes(malformed data), accessingfilteredNodes[edge.Source]returns a zero-value struct withHidden = false, causing the edge to be kept as visible rather than hidden.If you want defensive handling:
for _, edge := range graphResponse.Edges { - if filteredNodes[edge.Target].Hidden || filteredNodes[edge.Source].Hidden { + sourceNode, sourceExists := filteredNodes[edge.Source] + targetNode, targetExists := filteredNodes[edge.Target] + if !sourceExists || !targetExists || sourceNode.Hidden || targetNode.Hidden { filteredEdges = append(filteredEdges, model.UnifiedEdge{This would also hide edges that reference nodes not in the original graph response. However, if you trust data integrity from the database, the current implementation is acceptable.
cmd/api/src/api/v2/cypherquery_test.go (1)
540-555: Edge test data references non-existent node"source".In the mock graph response (lines 528-546), nodes "1" and "2" are defined, but the edge references
Source: "source"which isn't a node in the graph. The filtering logic will look upfilteredNodes["source"]and get a zero-value withHidden = false.This means the edge visibility check
filteredNodes[edge.Source].Hidden || filteredNodes[edge.Target].Hiddenevaluates tofalse || <node 1 hidden state>. Since node "1" becomes hidden, the edge correctly becomes hidden, but for the wrong reason.Consider adding
"source"as a node in the mock to properly test the filtering logic, or acknowledge this is testing the edge case of orphan edges.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/api/src/api/v2/cypherquery.go(3 hunks)cmd/api/src/api/v2/cypherquery_test.go(10 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
📚 Learning: 2025-11-05T21:13:08.542Z
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.
Applied to files:
cmd/api/src/api/v2/cypherquery.gocmd/api/src/api/v2/cypherquery_test.go
📚 Learning: 2025-08-27T21:15:32.207Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
Applied to files:
cmd/api/src/api/v2/cypherquery.go
📚 Learning: 2025-09-08T19:01:53.112Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx:108-148
Timestamp: 2025-09-08T19:01:53.112Z
Learning: In BloodHound's CypherSearch component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/CypherSearch.tsx), the sharing state reset for sharedIds and isPublic after the two-step permissions update is handled elsewhere in the codebase, so additional state reset callbacks in the updateQueryPermissions function are not needed.
Applied to files:
cmd/api/src/api/v2/cypherquery.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
cmd/api/src/api/v2/cypherquery_test.go
📚 Learning: 2025-07-22T20:30:34.839Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.
Applied to files:
cmd/api/src/api/v2/cypherquery_test.go
🧬 Code graph analysis (1)
cmd/api/src/api/v2/cypherquery_test.go (4)
cmd/api/src/model/appcfg/flag.go (2)
FeatureETAC(43-43)FeatureFlag(50-70)cmd/api/src/queries/graph.go (1)
DefaultQueryFitnessLowerBoundExplore(67-67)cmd/api/src/model/unified_graph.go (3)
UnifiedGraph(37-40)UnifiedEdge(64-71)UnifiedNode(51-61)cmd/api/src/api/v2/cypherquery.go (1)
CypherQueryPayload(41-44)
🔇 Additional comments (6)
cmd/api/src/api/v2/cypherquery.go (3)
26-26: LGTM on new imports.The
timeimport is used fortime.Time{}in hidden node/edge creation, andappcfgis used for feature flag lookup. Both are necessary.Also applies to: 32-32
94-110: LGTM on ETAC filtering integration.The approach of always fetching properties for ETAC filtering and stripping them later when
IncludePropertiesis false is correct. The error handling now properly returns after writing the error response.
116-132: LGTM on property stripping.The iteration now correctly uses the map key
idinstead ofnode.ObjectId, and edge properties are properly stripped whenIncludePropertiesis false.cmd/api/src/api/v2/cypherquery_test.go (3)
76-94: LGTM on test request construction with user context.The test correctly creates a user, sets up the user context, and attaches it to the request using
WithContext. This pattern is consistent across the test cases.
570-574: LGTM on test resource wiring.The
DBfield is correctly wired to the mock database, enabling the ETAC feature flag lookup in tests.
351-355: The review comment is incorrect; the struct already hasomitemptytags.The
UnifiedGraphWPropertyKeysstruct definesEdgeKeysas[]stringjson:"edge_keys,omitempty"(not withoutomitemptyas claimed). With theomitemptytag, JSON marshaling will omit the field when the slice is empty, which is exactly whyedge_keysis absent from the expected response bodies in the test cases.The test expectations at lines 353, 412, 487, and 552 are correct—
edge_keysshould be omitted when edges have no properties, and the same applies tonode_keyswhen nodes have no properties.Likely an incorrect or invalid review comment.
Description
This change is to implement ETAC changes to the explore page cypher query endpoint.
Motivation and Context
Resolves BED-6714
Why is this change required? What problem does it solve?
This change is needed for ETAC filtering of nodes and edges in the explore page.
How Has This Been Tested?
Added additional unit tests for new functionality changes. Updated existing unit tests. Screenshots below on how to test locally.
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Screenshots (optional):
admin user - hidden in response only on node property as false
single env(phantom) user - hidden node
single env(phantom) user - no ghost corp
single env(phantom) user - no wraith corp
no envs user - all hidden nodes
no envs user - no ghost corp
no envs user - no phantom corp
no envs user - no wraith corp
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.