feat: Update Cypher Query endpoint to hide nodes/edges based on users ETAC list - BED-6714 #2130
Conversation
…rs environment as hidden
WalkthroughThe PR introduces ETAC (Environment-based Targeted Access Control) filtering into the CypherQuery flow. It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Request
participant Handler as CypherQuery Handler
participant DB as Database
participant ETAC as ETAC Check
participant Filter as filterETACGraph
participant Model as Response Model
Client->>Handler: HTTP Request (with user context)
Handler->>Handler: Extract user from auth context
alt User extraction fails
Handler->>Client: 500 Internal Server Error
else User extracted successfully
Handler->>DB: GetFlagByKey(ETAC feature flag)
DB->>ETAC: Feature flag status
ETAC->>Handler: Should filter? (bool)
Handler->>Filter: filterETACGraph(ctx, graph, user)
alt ETAC filtering enabled
Filter->>Model: Apply filtering (mark hidden nodes/edges)
Model->>Filter: Filtered graph
else No filtering required
Filter->>Handler: Original graph
end
Handler->>Handler: Process properties on filtered graph
alt IncludeProperties is false
Handler->>Model: Strip properties from filtered graph
end
Handler->>Client: JSON response with filtered/processed graph
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/api/src/api/v2/cypherquery.go (1)
174-176: Include the error value in the log message.The error is propagated correctly but the log message doesn't include the actual error, making debugging harder.
if err != nil { - slog.Error("Unable to check ETAC filtering") + slog.Error("Unable to check ETAC filtering", "error", err) return model.UnifiedGraph{}, err }
📜 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/etac.go(2 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.
📚 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/etac.gocmd/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
🔇 Additional comments (6)
cmd/api/src/api/v2/etac.go (1)
78-94: LGTM! Clean ETAC filtering decision logic.The function correctly handles:
- User extraction from auth context with proper error handling
- Feature flag check with error propagation
- Short-circuit for disabled ETAC or admin users with
AllEnvironmentsBased on learnings, the empty
accessListwhenuser.EnvironmentTargetedAccessControlis empty is intentional for security—users with no environment assignments correctly see no results.cmd/api/src/api/v2/cypherquery.go (5)
93-98: Good approach: Always fetch properties for ETAC filtering.The comments clearly explain why
includePropertiesis alwaystrueat the database level—ETAC filtering needs access todomainsid/tenantidproperties. Properties are correctly stripped at response-level whenpayload.IncludePropertiesisfalse.
105-131: LGTM! Error handling and property stripping are correctly implemented.Previous review issues have been addressed:
- Return statement after error response (line 108-109)
- Using map
idkey instead ofnode.ObjectId(line 117)- Stripping both node and edge properties (lines 117-125)
199-221: Clean implementation of hidden node placeholders.The hidden node structure appropriately:
- Preserves the map key (original node ID) for graph traversal
- Uses descriptive label with the original kind for user context
- Sets the
Hiddenflag for downstream filtering- Clears sensitive data (properties, ObjectId, IsOwnedObject, IsTierZero)
186-197: The property key casing in the code is correct. BloodHound consistently uses lowercase property keys:"domainsid"and uses similar lowercase naming patterns across its collectors. While Neo4j property keys are case-sensitive, the code correctly matches the actual BloodHound data model where these properties are stored in lowercase. This is not a correctness issue.Likely an incorrect or invalid review comment.
227-240: No action needed. The edge case described cannot occur with the current code flow because Neo4j maintains referential integrity—all relationships returned from Cypher queries include their source and target nodes in the result set. TheAddPathSet()method populates the graph with nodes and edges from the same paths, ensuring all edge references exist infilteredNodes.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/api/src/api/v2/cypherquery.go (3)
108-111: Provide a more descriptive error message.The generic
"error"message provides no context for debugging. Consider including what operation failed.if err != nil { - api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusInternalServerError, "error", request), response) + api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusInternalServerError, "error filtering graph for access control", request), response) return }
180-183: Include the error in the log message for debugging.The error value is discarded in the log, making troubleshooting difficult.
if err != nil { - slog.Error("Unable to check ETAC filtering") + slog.Error("Unable to check ETAC filtering", "error", err) return model.UnifiedGraph{}, err }
193-205: Consider extracting environment keys as package-level constants.Hardcoding
"domainsid"and"tenantid"inline may make future updates error-prone if these keys are used elsewhere.+const ( + envKeyDomainSID = "domainsid" + envKeyTenantID = "tenantid" +) + func (s Resources) filterETACGraph(...) { ... - environmentKeys := []string{"domainsid", "tenantid"} + environmentKeys := []string{envKeyDomainSID, envKeyTenantID}
📜 Review details
Configuration used: Organization 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/etac.go(2 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.
📚 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/etac.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
🧬 Code graph analysis (2)
cmd/api/src/api/v2/cypherquery.go (3)
cmd/api/src/api/error.go (1)
BuildErrorResponse(134-145)cmd/api/src/model/unified_graph.go (3)
UnifiedGraph(37-40)UnifiedNode(51-61)UnifiedEdge(64-71)cmd/api/src/api/v2/etac.go (1)
ShouldFilterForETAC(81-97)
cmd/api/src/api/v2/etac.go (3)
cmd/api/src/auth/model.go (1)
GetUserFromAuthCtx(184-191)cmd/api/src/ctx/ctx.go (1)
FromRequest(70-72)cmd/api/src/model/appcfg/flag.go (1)
FeatureETAC(43-43)
⏰ 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 (4)
cmd/api/src/api/v2/etac.go (1)
78-96: LGTM! Clear and secure ETAC filtering determination.The function correctly implements the security model:
- Returns error for unknown users (line 84)
- Propagates DB errors (line 89)
- Bypasses filtering when ETAC is disabled or user has full access (line 92-93)
- Returns environment list for restricted users
Based on learnings, the behavior where an empty
accessList(when user has no environments) causes all nodes to be filtered out is the correct security behavior.cmd/api/src/api/v2/cypherquery.go (3)
117-131: LGTM!Property stripping correctly handles both nodes and edges using the proper map key and slice index for in-place modification.
237-253: LGTM!Edge filtering correctly marks edges as hidden when connected to hidden nodes. The use of
Kind: "HIDDEN"and a descriptive label provides a consistent marker for client-side filtering, matching the hidden node pattern.
207-231: LGTM!Hidden node placeholder correctly:
- Extracts the node kind for informative labeling
- Sanitizes all identifying information (ObjectId, Properties, LastSeen)
- Sets
Hidden: truefor explicit client-side detection
stephanieslamb
added
enhancement
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/api/src/api/v2/cypherquery_test.go(10 hunks)cmd/api/src/model/unified_graph.go(1 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.
📚 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
📚 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.go
🧬 Code graph analysis (1)
cmd/api/src/api/v2/cypherquery_test.go (3)
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/api/v2/cypherquery.go (1)
CypherQueryPayload(40-43)
⏰ 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: run-analysis
- GitHub Check: build-ui
🔇 Additional comments (7)
cmd/api/src/model/unified_graph.go (1)
60-60: LGTM! Clean addition of ETAC visibility metadata.The
Hiddenfield withomitemptytag is well-designed: it maintains backward compatibility (omitted when false), supports the ETAC filtering feature, and correctly defaults to false for nodes created viaFromDAWGSNode. The filtering logic that sets this field to true for inaccessible nodes is handled elsewhere in the request flow.cmd/api/src/api/v2/cypherquery_test.go (6)
39-39: LGTM! Import required for ETAC feature flag testing.The
appcfgpackage import is necessary for accessingappcfg.FeatureETACin feature flag mocks throughout the updated tests.
76-95: LGTM! User context setup enables ETAC-aware testing.Adding user context with
AllEnvironments: trueto existing tests ensures the handler can access user permissions for ETAC filtering. This is necessary infrastructure for ETAC support without changing the expected behavior of these tests (since users with all environments access should see unfiltered results).Also applies to: 263-282, 309-327
115-116: LGTM! Feature flag mocking maintains existing test semantics.Mocking the ETAC feature flag as disabled in existing tests ensures they continue to verify the original (non-filtered) behavior while supporting the new handler logic that checks the flag.
Also applies to: 289-290, 348-349
357-415: LGTM! Test verifies ETAC bypass for users with all environments access.This test correctly validates that when ETAC is enabled but a user has
AllEnvironments: true, no filtering occurs and the response remains identical to the non-ETAC case. This is the expected security behavior for privileged users.
417-490: LGTM! Test validates selective ETAC filtering and placeholder rendering.This test comprehensively verifies the ETAC filtering behavior for users with limited environment access:
- Accessible nodes (matching user's ETAC list) are rendered with full details
- Inaccessible nodes are marked with
hidden:trueand replaced with placeholder values (kind: "HIDDEN", label: "** Hidden kinds Object **", etc.)- Edges connected to hidden nodes are also marked as hidden
- Graph structure is preserved while sensitive details are obscured
This is the correct security and UX behavior.
570-574: LGTM! Proper database wiring for ETAC feature flag checks.Setting the
DBfield in theResourcesstruct enables the handler to callGetFlagByKeyfor ETAC feature detection. TheAuthorizerinitialization with the mock database is also necessary for permission checks. This is the correct test setup pattern.
| name: "Success: ETAC enabled, user has no access, hidden graph - 200", | ||
| buildRequest: func() *http.Request { | ||
| payload := &v2.CypherQueryPayload{ | ||
| Query: "query", | ||
| IncludeProperties: true, | ||
| } | ||
| jsonPayload, err := json.Marshal(payload) | ||
| if err != nil { | ||
| t.Fatalf("error occurred while marshaling payload necessary for test: %v", err) | ||
| } | ||
| user := model.User{ | ||
| AllEnvironments: false, | ||
| } | ||
| userCtx := setupUserCtx(user) | ||
|
|
||
| req := &http.Request{ | ||
| URL: &url.URL{ | ||
| Path: "/api/v2/graphs/cypher", | ||
| }, | ||
| Body: io.NopCloser(bytes.NewReader(jsonPayload)), | ||
| Header: http.Header{ | ||
| headers.ContentType.String(): []string{ | ||
| "application/json", | ||
| }, | ||
| }, | ||
| Method: http.MethodPost, | ||
| } | ||
| req = req.WithContext(userCtx) | ||
| return req | ||
| }, | ||
| setupMocks: func(t *testing.T, mocks *mock) { | ||
| t.Helper() | ||
| mocks.mockGraphQuery.EXPECT().PrepareCypherQuery("query", int64(queries.DefaultQueryFitnessLowerBoundExplore)).Return(queries.PreparedQuery{ | ||
| HasMutation: false, | ||
| }, nil) | ||
| mocks.mockGraphQuery.EXPECT().RawCypherQuery(gomock.Any(), gomock.Any(), gomock.Any()).Return(model.UnifiedGraph{ | ||
| Nodes: map[string]model.UnifiedNode{ | ||
| "1": { | ||
| Label: "label", | ||
| Properties: map[string]any{"domainsid": "testenv"}, | ||
| Kinds: []string{"kinds"}, | ||
| }, | ||
| "2": { | ||
| Label: "label2", | ||
| Properties: map[string]any{"domainsid": "value"}, | ||
| Kinds: []string{"kinds"}, | ||
| }, | ||
| }, | ||
| Edges: []model.UnifiedEdge{ | ||
| { | ||
| Source: "source", | ||
| Target: "1", | ||
| }, | ||
| }, | ||
| }, nil) | ||
| mocks.mockDatabase.EXPECT().GetFlagByKey(gomock.Any(), appcfg.FeatureETAC). | ||
| Return(appcfg.FeatureFlag{Enabled: true}, nil) | ||
| }, | ||
| expected: expected{ | ||
| responseCode: http.StatusOK, | ||
| responseBody: `{"data":{"node_keys": ["key"], "nodes":{"1":{"label":"label","properties": {"key": "value"},"kind":"","objectId":"","kinds":null, "isTierZero":false,"isOwnedObject":false,"lastSeen":"0001-01-01T00:00:00Z"}},"edges":[{"source":"source","target":"","label":"","kind":"","lastSeen":"0001-01-01T00:00:00Z"}]}}`, | ||
| responseBody: `{"data":{"nodes":{"1":{"hidden":true,"isOwnedObject":false,"isTierZero":false,"kind":"HIDDEN","kinds":[],"label":"** Hidden kinds Object **","lastSeen":"0001-01-01T00:00:00Z","objectId":"HIDDEN"},"2":{"hidden":true,"isOwnedObject":false,"isTierZero":false,"kind":"HIDDEN","kinds":[],"label":"** Hidden kinds Object **","lastSeen":"0001-01-01T00:00:00Z","objectId":"HIDDEN"}},"edges":[{"source":"source","target":"1","label":"** Hidden Edge **","kind":"HIDDEN","lastSeen":"0001-01-01T00:00:00Z"}]}}`, | ||
| responseHeader: http.Header{"Content-Type": []string{"application/json"}}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where response structure is built, particularly for hidden graphs
rg -n "node_keys|edge_keys" cmd/api/src/api/v2/cypherquery_test.go | head -20Repository: SpecterOps/BloodHound
Length of output: 2192
🏁 Script executed:
#!/bin/bash
# Search for response building logic in the handler
fd -e go -path "*/api/v2/*" ! -path "*/test.go" | xargs rg -l "CypherQuery" | head -5Repository: SpecterOps/BloodHound
Length of output: 626
🏁 Script executed:
#!/bin/bash
# Extract all test case names to understand the test scenarios
rg -n "name:.*ETAC" cmd/api/src/api/v2/cypherquery_test.go | head -10Repository: SpecterOps/BloodHound
Length of output: 272
🏁 Script executed:
#!/bin/bash
# Find the handler that processes cypher queries
fd -type f -name "*.go" cmd/api/src/api/v2/ ! -name "*test*" | xargs rg -l "CypherQuery" | head -5Repository: SpecterOps/BloodHound
Length of output: 623
🏁 Script executed:
#!/bin/bash
# Search for the response building logic for cypher queries
rg -n "\"data\"" cmd/api/src/api/v2/*.go | grep -i cypher | head -10Repository: SpecterOps/BloodHound
Length of output: 2912
🏁 Script executed:
#!/bin/bash
# Look for where node_keys and edge_keys are populated
rg -n "node_keys|edge_keys" cmd/api/src/ --type go | grep -v test | head -20Repository: SpecterOps/BloodHound
Length of output: 264
🏁 Script executed:
#!/bin/bash
# Find the cypher query handler function
rg -n "func.*CypherQuery" cmd/api/src/api/v2/*.go | grep -v testRepository: SpecterOps/BloodHound
Length of output: 286
🏁 Script executed:
#!/bin/bash
# Look for where NodeKeys are populated in the response building
rg -n "NodeKeys\s*=" cmd/api/src -A 3 -B 3 | head -40Repository: SpecterOps/BloodHound
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check the full handler implementation for the cypher query endpoint
rg -n "RawCypherQuery\|PostCypherQuery" cmd/api/src/api/v2/*.go | grep -v testRepository: SpecterOps/BloodHound
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Get the CypherQuery handler function implementation
sed -n '75,300p' cmd/api/src/api/v2/cypherquery.goRepository: SpecterOps/BloodHound
Length of output: 6365
The response is missing node_keys and edge_keys fields—this indicates a bug in the ETAC filtering logic.
When ETAC filtering is applied in filterETACGraph, the new UnifiedGraph response object never gets NodeKeys and EdgeKeys populated from the original query result. Since these fields have omitempty tags and are nil, they're excluded from the JSON response. The BloodHound API specification shows these fields as part of the standard response structure, and all other test cases (lines 353, 412, 487) include at least node_keys. The filtered response should preserve or regenerate these field keys. This is not a test oversight but a code issue that needs correction in the filterETACGraph function to properly copy NodeKeys and EdgeKeys from the original response.
🤖 Prompt for AI Agents
In cmd/api/src/api/v2/cypherquery_test.go around lines 492-555, the
ETAC-filtered response is missing node_keys and edge_keys because
filterETACGraph builds a new UnifiedGraph but does not copy the original's
NodeKeys and EdgeKeys; update filterETACGraph to copy (or regenerate) the
original graph's NodeKeys and EdgeKeys into the filtered UnifiedGraph before
returning (ensuring they are non-nil and preserve order/semantics) so the JSON
response includes these fields.
mvlipka
left a comment
mvlipka
left a comment
There was a problem hiding this comment.
This looks great! Just one comment. Will pull down for testing
cmd/api/src/api/v2/etac.go
Outdated
| // ShouldFilterForETAC is a helper function that determines whether ETAC(Environment-based Access Control) | ||
| // filtering should be applied for the current user. It checks if the ETAC feature flag is enabled and | ||
| // returns the users level of access(AllEnvironments or a list of environments). | ||
| func ShouldFilterForETAC(request *http.Request, db database.Database) (accessList []string, shouldFilter bool, err error) { |
There was a problem hiding this comment.
Not sure the intent of this function is evident as the verbiage, to me, indicates that a boolean would be returned. Without reading the returns or documentation, I wouldn't know that this could be used to pull a user's environment list
The name may need some rework, but I think I would remove the Extract functionality from here and keep it as two explicit calls
Aside from that: you're pre-declaring variable names in the return but aren't using them in scope.
Named returns, generally, should be used scarcely when writing a function with a small amount of LOC
There was a problem hiding this comment.
updated this function. let me know if this is more what you are thinking!
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/api/src/api/v2/cypherquery_test.go (1)
583-586: Verify: response missingnode_keysandedge_keysfields.The expected response at line 585 is missing
node_keysandedge_keysfields, while other successful test cases (lines 120, 386, 445, 520) include at leastnode_keys. This was flagged in a prior review as a potential bug in thefilterETACGraphfunction rather than a test issue.If the handler has been fixed to include these keys for filtered graphs, this expected response should be updated accordingly. If not, this test correctly documents the current (potentially buggy) behavior.
#!/bin/bash # Check if node_keys/edge_keys are populated in the filtered graph response path rg -n "NodeKeys|EdgeKeys" cmd/api/src/api/v2/cypherquery.go -A 3 -B 3
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/api/src/api/v2/cypherquery.go(4 hunks)cmd/api/src/api/v2/cypherquery_test.go(17 hunks)cmd/api/src/api/v2/etac.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/api/src/api/v2/etac.go
- cmd/api/src/api/v2/cypherquery.go
🧰 Additional context used
🧠 Learnings (5)
📓 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: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: cmd/api/src/api/v2/auth/auth.go:559-573
Timestamp: 2025-09-02T16:46:30.895Z
Learning: In the BloodHound codebase, when updating user ETAC (Environment Access Control) lists in the UpdateUser function, the approach is to let GORM handle the creation/persistence of the environment access records through model associations rather than using explicit database helper methods like UpdateEnvironmentListForUser.
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-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
📚 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.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_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)UnifiedNode(51-61)UnifiedEdge(64-71)cmd/api/src/api/v2/cypherquery.go (1)
CypherQueryPayload(40-43)
⏰ 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: build-ui
- GitHub Check: run-tests
- GitHub Check: run-analysis
🔇 Additional comments (4)
cmd/api/src/api/v2/cypherquery_test.go (4)
35-42: LGTM!The
appcfgimport is correctly added to support ETAC feature flag lookups in the new test cases.
124-284: LGTM!The early error path test cases correctly omit the
GetFlagByKeymock since these scenarios fail before reaching the ETAC filtering logic. The user context setup is consistently applied.
449-523: LGTM!This test case thoroughly verifies ETAC filtering behavior with a user who has access to specific environments. The mock data correctly sets up:
- Accessible nodes matching the user's environment ("testenv")
- A hidden node with a different domain SID
- Edges that should be hidden when connected to hidden nodes
The expected response properly reflects hidden placeholders with
kind: "HIDDEN"labels.
603-607: LGTM!The
DBfield is correctly wired into theResourcesstruct to enable ETAC feature flag lookups via the mock database. TheAuthorizerinitialization with the same mock maintains consistency. Based on learnings, subtest parallelization is acceptable here since each creates its own mock controller.
| mocks.mockGraphQuery.EXPECT().RawCypherQuery(gomock.Any(), gomock.Any(), gomock.Any()).Return(model.UnifiedGraph{ | ||
| Nodes: map[string]model.UnifiedNode{ | ||
| "1": { | ||
| Label: "label", | ||
| Properties: map[string]any{"domainsid": "testenv"}, | ||
| Kinds: []string{"kinds"}, | ||
| }, | ||
| "2": { | ||
| Label: "label2", | ||
| Properties: map[string]any{"domainsid": "value"}, | ||
| Kinds: []string{"kinds"}, | ||
| }, | ||
| }, | ||
| Edges: []model.UnifiedEdge{ | ||
| { | ||
| Source: "source", | ||
| Target: "1", | ||
| }, | ||
| }, | ||
| }, nil) |
There was a problem hiding this comment.
Test data inconsistency: edge references non-existent node.
The edge at lines 573-577 has Source: "source", but there's no node with ID "source" in the Nodes map (only "1" and "2" exist). This differs from the similar test case at lines 488-514 which correctly includes a "source" node.
This inconsistency could mask potential edge-case bugs in the filtering logic when edges reference nodes outside the graph.
🔎 Suggested fix
Either add the missing "source" node to align with realistic graph data:
mocks.mockGraphQuery.EXPECT().RawCypherQuery(gomock.Any(), gomock.Any(), gomock.Any()).Return(model.UnifiedGraph{
Nodes: map[string]model.UnifiedNode{
"1": {
Label: "label",
Properties: map[string]any{"domainsid": "testenv"},
Kinds: []string{"kinds"},
},
"2": {
Label: "label2",
Properties: map[string]any{"domainsid": "value"},
Kinds: []string{"kinds"},
},
+ "source": {
+ Label: "labelSource",
+ Properties: map[string]any{"domainsid": "otherenv"},
+ Kinds: []string{"kinds"},
+ },
},Or update the edge to reference existing nodes:
Edges: []model.UnifiedEdge{
{
- Source: "source",
+ Source: "2",
Target: "1",
},
},🤖 Prompt for AI Agents
In cmd/api/src/api/v2/cypherquery_test.go around lines 560-579 the test graph
contains an edge with Source: "source" but the Nodes map only contains "1" and
"2", causing an inconsistent test fixture; fix by either adding a "source" node
entry to the Nodes map (matching the shape used in the similar test at lines
488-514) with appropriate Label/Properties/Kinds, or change the edge Source to
reference an existing node ID ("1" or "2") so the edge points to a valid node.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/api/src/api/v2/cypherquery.go (1)
202-202: Consider documenting the environment key list.The hardcoded list
["domainsid", "tenantid"]determines which node properties are checked for environment access. If additional environment identifier properties exist (or may be added in the future), this could silently fail to filter correctly.Consider:
- Adding a comment explaining why only these two keys are checked
- Or centralizing this list as a package-level constant if used elsewhere
- Or verifying against a schema/configuration to ensure completeness
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/api/v2/cypherquery.go
🧰 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: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: cmd/api/src/api/v2/auth/auth.go:559-573
Timestamp: 2025-09-02T16:46:30.895Z
Learning: In the BloodHound codebase, when updating user ETAC (Environment Access Control) lists in the UpdateUser function, the approach is to let GORM handle the creation/persistence of the environment access records through model associations rather than using explicit database helper methods like UpdateEnvironmentListForUser.
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 (6)
cmd/api/src/api/v2/cypherquery.go (6)
100-106: LGTM: Properties always fetched for ETAC filtering.The hardcoded
trueforincludePropertiesensures ETAC filtering logic can access node properties for environment checks. The originalpayload.IncludePropertiesflag is correctly honored later during response construction.
113-122: LGTM: ETAC filtering integration.The filtering is applied correctly with proper error handling and appropriate 404 response for non-mutation queries that return no accessible resources.
124-138: LGTM: Complete property stripping.Both node and edge properties are correctly removed from the filtered response when
payload.IncludePropertiesis false, maintaining the original API contract.
229-240: Verify: All hidden nodes shareObjectId="HIDDEN".All hidden placeholder nodes are assigned the same
ObjectId="HIDDEN". While this appears intentional for ETAC display purposes, please confirm that downstream code (graph rendering, caching, etc.) doesn't assume unique ObjectIds within a response.Based on learnings, the empty
accessListbehavior (when a user has no environment assignments) is correct and will properly filter all nodes.
205-214: LGTM: Environment-based node filtering logic.The filtering correctly checks node properties against the user's allowed environment list. The type assertion and contains check properly handle missing properties and non-string values.
247-261: LGTM: Edge filtering preserves graph consistency.Edges are correctly marked as hidden when either endpoint is inaccessible, maintaining graph structure while respecting ETAC boundaries.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @cmd/api/src/queries/graph.go:
- Line 448: The stray debug print in PrepareCypherQuery (fmt.Printf("multi
query: %v", queryModel.SingleQuery.MultiPartQuery.Parts)) should be removed;
replace any required debugging with the structured logger used in the codebase
(e.g., use slog.DebugContext with a context and slog.Any("parts", ...) if you
change PrepareCypherQuery to accept a context) or simply delete the print to
avoid unstructured stdout, potential sensitive output, and test pipeline
failures.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/api/src/config/config.gocmd/api/src/config/default.gocmd/api/src/queries/graph.gopackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.test.tsx
✅ Files skipped from review due to trivial changes (3)
- cmd/api/src/config/default.go
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.test.tsx
- cmd/api/src/config/config.go
🧰 Additional context used
🧠 Learnings (1)
📓 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: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: cmd/api/src/api/v2/auth/auth.go:559-573
Timestamp: 2025-09-02T16:46:30.895Z
Learning: In the BloodHound codebase, when updating user ETAC (Environment Access Control) lists in the UpdateUser function, the approach is to let GORM handle the creation/persistence of the environment access records through model associations rather than using explicit database helper methods like UpdateEnvironmentListForUser.
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.
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.
🪛 GitHub Actions: Run Go Unit Tests
cmd/api/src/queries/graph.go
[error] 1-1: Test run failed with exit code 1
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cmd/api/src/api/v2/cypherquery.go:
- Around line 114-119: The error response for the ETAC filtering call is too
generic; when s.filterETACGraph(request.Context(), graphResponse, user) returns
an error, include the actual error text and context in the response and logs by
passing err.Error() (or a wrapped message) into api.BuildErrorResponse and/or
logging it before calling api.WriteErrorResponse; update the handling around
s.filterETACGraph to construct a descriptive message like "ETAC filter failed:
<err>" and use that with api.BuildErrorResponse and any available logger so both
the response and logs contain the specific failure details.
🧹 Nitpick comments (1)
cmd/api/src/api/v2/cypherquery.go (1)
203-203: Consider extracting environment keys to a constant.The hardcoded
environmentKeysslice could become a package-level constant for better maintainability. If new environment types are added in the future, having these keys in one centralized location would make updates easier.♻️ Suggested refactor
At the package level:
var ( errUnauthorizedGraphMutation = errors.New("unauthorized graph mutation") // ETACEnvironmentKeys are the property keys used to determine node environment ownership ETACEnvironmentKeys = []string{"domainsid", "tenantid"} )Then in the function:
- environmentKeys := []string{"domainsid", "tenantid"} - // filter nodes based on environment access for id, node := range graphResponse.Nodes { include := false - for _, key := range environmentKeys { + for _, key := range ETACEnvironmentKeys {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/api/v2/cypherquery.go
🧰 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.
📚 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: run-tests
- GitHub Check: build-ui
🔇 Additional comments (11)
cmd/api/src/api/v2/cypherquery.go (11)
84-89: Good security practice: early user extraction with fail-safe behavior.Retrieving the user early and failing with a 500 error if extraction fails is appropriate for this security-critical ETAC filtering path.
102-106: LGTM: Properties fetched internally for ETAC filtering.Defaulting
includePropertiestotrueensures ETAC filtering has access to environment properties (domainsid,tenantid) for access control decisions. The user's preference is correctly honored later by stripping properties whenpayload.IncludePropertiesis false.
120-123: LGTM: Zero-result check operates on filtered graph.Checking the filtered response for empty results is correct. Users whose queries return results that are all filtered out by ETAC should receive a 404, not see hidden placeholders for everything.
125-139: LGTM: Property stripping operates on filtered graph.Correctly strips properties from the filtered response when
IncludePropertiesis false, preventing information leakage and honoring user preferences.
141-141: LGTM: Property processing uses filtered graph.Correctly processes properties from the filtered response, ensuring property keys reflect only the accessible data.
186-196: LGTM: Early exit when ETAC filtering not needed.Efficiently checks if filtering is required and returns the original graph when filtering is disabled, avoiding unnecessary processing.
198-198: LGTM: Empty access list behavior is intentional.Based on learnings, when
ExtractEnvironmentIDsFromUserreturns an empty list (for users with no environment assignments), hiding all nodes is the correct security behavior—users with no environment assignments should see no results.
206-242: LGTM: Node filtering logic correctly implements ETAC.The logic correctly:
- Checks each node for environment property matches against the user's access list
- Preserves nodes the user can access
- Replaces inaccessible nodes with hidden placeholders that have all sensitive data (properties, ObjectId) removed
Note: Nodes without
domainsidortenantidproperties will be hidden. This appears intentional for security—if a node lacks environment identifiers, users without full access shouldn't see it.
223-227: Good defensive programming: safe kind extraction.Safely handles empty or missing
Kindsby defaulting to "Unknown", preventing potential panics or display issues.
230-240: LGTM: Hidden node structure prevents information leakage.The hidden placeholder correctly:
- Sets all sensitive fields (ObjectId, Properties) to safe values (nil or "HIDDEN")
- Preserves only the kind for UX clarity ("** Hidden {Kind} Object **")
- Marks the node with
Hidden: truefor downstream processing
248-262: LGTM: Edge filtering correctly handles hidden nodes.Edges connected to hidden nodes are properly replaced with hidden placeholders, preventing information leakage about relationship existence between inaccessible objects. Properties are correctly set to nil.
| // etac filtering | ||
| filteredResponse, err := s.filterETACGraph(request.Context(), graphResponse, user) | ||
| if err != nil { | ||
| api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusInternalServerError, "error", request), response) | ||
| return | ||
| } |
There was a problem hiding this comment.
Improve error message for ETAC filtering failure.
The error message on Line 117 is too generic. Consider providing more context for debugging and user support.
💬 Suggested improvement
filteredResponse, err := s.filterETACGraph(request.Context(), graphResponse, user)
if err != nil {
- api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusInternalServerError, "error", request), response)
+ api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusInternalServerError, "failed to apply access control filtering", request), response)
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // etac filtering | |
| filteredResponse, err := s.filterETACGraph(request.Context(), graphResponse, user) | |
| if err != nil { | |
| api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusInternalServerError, "error", request), response) | |
| return | |
| } | |
| // etac filtering | |
| filteredResponse, err := s.filterETACGraph(request.Context(), graphResponse, user) | |
| if err != nil { | |
| api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusInternalServerError, "failed to apply access control filtering", request), response) | |
| return | |
| } |
🤖 Prompt for AI Agents
In @cmd/api/src/api/v2/cypherquery.go around lines 114 - 119, The error response
for the ETAC filtering call is too generic; when
s.filterETACGraph(request.Context(), graphResponse, user) returns an error,
include the actual error text and context in the response and logs by passing
err.Error() (or a wrapped message) into api.BuildErrorResponse and/or logging it
before calling api.WriteErrorResponse; update the handling around
s.filterETACGraph to construct a descriptive message like "ETAC filter failed:
<err>" and use that with api.BuildErrorResponse and any available logger so both
the response and logs contain the specific failure details.
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
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.