feat: Update Pathfinding endpoint to hide nodes/edges based on users ETAC list - BED-6973 #2175
Conversation
WalkthroughAdds ETAC (Environment-Targeted Access Control) support: a helper determines if ETAC filtering applies for the request, GetSearchResult uses it to optionally mask unauthorized graph nodes as hidden placeholders, and UnifiedNode gains a Hidden field for serialization. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as GetSearchResult
participant ETAC as ShouldFilterForETAC
participant DB as Database
participant Filter as filterSearchResultMap
Client->>API: GET /search
API->>ETAC: ShouldFilterForETAC(request, db)
ETAC->>DB: GetFlagByKey("ETAC") / fetch user envs
DB-->>ETAC: flag + user/environments
ETAC-->>API: accessList, shouldFilter
alt shouldFilter == true
API->>Filter: filterSearchResultMap(graph, accessList)
loop per node
Filter->>Filter: inspect node env (domainSid/tenantId)
alt allowed
Filter->>Filter: keep node
else denied
Filter->>Filter: replace with hidden node (Hidden=true, label from kinds)
end
end
Filter-->>API: filtered graph
else
API-->>API: use original graph
end
API-->>Client: JSON search results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/api/src/api/v2/pathfinding_test.go (1)
517-517: Minor: Inconsistent spacing in test name.The test name
"Success -- ETAC enabled,user has all environments"is missing a space after the comma. Consider"Success -- ETAC enabled, user has all environments"for consistency with other test names.- Name: "Success -- ETAC enabled,user has all environments", + Name: "Success -- ETAC enabled, user has all environments",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/api/src/api/v2/etac.go(2 hunks)cmd/api/src/api/v2/pathfinding.go(2 hunks)cmd/api/src/api/v2/pathfinding_test.go(7 hunks)cmd/api/src/model/unified_graph.go(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.
📚 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/pathfinding.gocmd/api/src/api/v2/pathfinding_test.go
📚 Learning: 2025-07-09T00:36:54.112Z
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
Applied to files:
cmd/api/src/api/v2/pathfinding.go
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T17:52:33.291Z
Learning: In BloodHound Go code, prefer using explicit slog type functions like slog.Any(), slog.String(), slog.Int(), etc. over simple key-value pairs for structured logging. This provides better type safety and makes key-value pairs more visually distinct. For error types, use slog.Any("key", err) or slog.String("key", err.Error()).
Applied to files:
cmd/api/src/api/v2/pathfinding.go
📚 Learning: 2025-05-29T18:24:23.227Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:49-51
Timestamp: 2025-05-29T18:24:23.227Z
Learning: In the stbernard package, use slog for all logging prints instead of fmt.Printf/fmt.Fprintf to maintain consistency with the codebase's logging standards.
Applied to files:
cmd/api/src/api/v2/pathfinding.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/pathfinding_test.go
📚 Learning: 2025-05-27T16:58:33.295Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Applied to files:
cmd/api/src/api/v2/pathfinding_test.go
🧬 Code graph analysis (3)
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)
cmd/api/src/api/v2/pathfinding.go (5)
cmd/api/src/api/bloodhoundgraph/conversions.go (1)
NodeSetToBloodHoundGraph(105-119)cmd/api/src/api/v2/etac.go (1)
ShouldFilterForETAC(78-94)cmd/api/src/api/marshalling.go (1)
WriteBasicResponse(90-99)cmd/api/src/api/error.go (1)
BuildErrorResponse(134-145)cmd/api/src/api/bloodhoundgraph/bloodhoundgraph.go (3)
BloodHoundGraphNode(87-98)BloodHoundGraphItem(53-58)BloodHoundGraphNodeLabel(77-85)
cmd/api/src/api/v2/pathfinding_test.go (4)
cmd/api/src/queries/mocks/graph.go (1)
NewMockGraph(53-57)cmd/api/src/database/mocks/db.go (1)
NewMockDatabase(55-59)cmd/api/src/queries/graph.go (1)
SearchTypeFuzzy(64-64)cmd/api/src/model/appcfg/flag.go (3)
FeatureETAC(43-43)FeatureFlag(50-70)FeatureOpenGraphSearch(44-44)
⏰ 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). (3)
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
🔇 Additional comments (7)
cmd/api/src/model/unified_graph.go (1)
60-60: LGTM!The
Hiddenfield is properly typed and tagged for JSON serialization. The defaultfalsevalue is appropriate for the hidden flag semantics.cmd/api/src/api/v2/etac.go (1)
78-94: LGTM!The
ShouldFilterForETACfunction correctly implements the ETAC filtering decision logic:
- Returns error for unknown users (proper auth check)
- Short-circuits when ETAC is disabled or user has
AllEnvironments- Returns the user's environment IDs for filtering in restricted scenarios
Based on learnings, the empty
accessListwhenuser.EnvironmentTargetedAccessControlis empty is intentional security behavior.cmd/api/src/api/v2/pathfinding.go (3)
222-231: Verify behavior for nodes withoutdomainsidortenantid.Nodes that lack both
domainsidandtenantidin theirDatamap will havehasAccess = falseand be hidden. This is secure-by-default, but could inadvertently hide legitimate nodes that aren't domain/tenant-scoped (e.g., cross-tenant resources or global objects).Please confirm this is the intended behavior, or consider whether such nodes should be passed through when ETAC filtering is active.
185-209: LGTM!The ETAC filtering integration is well-structured with proper error handling. The conditional filtering logic correctly applies the filter only when
shouldFilteris true, preserving the original graph otherwise.
243-256: LGTM!The hidden marker node properly masks all original node data and provides clear visual indicators (gray color, faded, descriptive label). Using the first element of
kindsfor the label provides useful context without exposing sensitive information.cmd/api/src/api/v2/pathfinding_test.go (2)
541-589: Good test coverage for ETAC filtering behavior.This test case effectively validates:
- Accessible nodes (matching
domainsidin access list) remain visible- Restricted nodes are replaced with hidden markers
- The hidden marker label correctly includes the node kind ("Computer")
- Original restricted data is not leaked in the response
The test data setup and assertions are comprehensive.
344-356: Test helper functions are properly defined and available.The
setupUser()andsetupUserCtx()functions exist incmd/api/src/api/v2/fileingest_test.goand are accessible topathfinding_test.go. Both functions properly set up the auth context with user information required byShouldFilterForETAC. No issues found.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/api/src/model/unified_graph.go (1)
60-60: Consider adding a comment documenting the Hidden field's purpose.While the field name is self-explanatory, a brief comment explaining that it's used for ETAC-based filtering to mark nodes as hidden placeholders would improve maintainability.
Example documentation:
Properties map[string]any `json:"properties,omitempty"` + // Hidden indicates whether this node should be hidden due to ETAC filtering Hidden bool `json:"hidden,omitempty"`
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/model/unified_graph.go(1 hunks)
🧰 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.
⏰ 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
Description
This change is to implement ETAC changes to the explore page pathfinding endpoint.
Motivation and Context
Resolves BED-6973
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?
Unit tests added for new functionality. All existing tests pass.
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):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.