chore: merge v8.5.1 release stage branch into main BED-7201#2292
chore: merge v8.5.1 release stage branch into main BED-7201#2292specter-flq merged 5 commits intomainfrom
Conversation
WalkthroughAdds FontAwesome-based custom node icon formatting, expands the server Content-Security-Policy format and its use, inserts an OpenGraph pathfinding feature flag and enables opengraph_search, refines CanRDP error handling to ignore only NotFound, and bumps a single Go dependency. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/api/src/analysis/ad/adcs_integration_test.go (1)
41-53: HandleBuildCacheerrors inFetchADCSPrereqs.
BuildCachereturns an error in the main code path; ignoring it here can mask failures and make tests pass incorrectly.🛠️ Suggested fix
- cache.BuildCache(context.Background(), db, enterpriseCertAuthorities, certTemplates) - return localGroupData, cache.GetEnterpriseCertAuthorities(), cache.GetCertTemplates(), cache.GetDomains(), cache, nil + if err := cache.BuildCache(context.Background(), db, enterpriseCertAuthorities, certTemplates); err != nil { + return nil, nil, nil, nil, ad2.ADCSCache{}, err + } + return localGroupData, cache.GetEnterpriseCertAuthorities(), cache.GetCertTemplates(), cache.GetDomains(), cache, nilpackages/go/analysis/ad/post.go (1)
208-224: UseNodePropertyin FetchAdminGroups node filter.
StartProperty/EndPropertyare relationship-scoped and will not match in a node query. Switch toNodePropertyto match the pattern used elsewhere in the codebase (seepackages/go/analysis/ad/ad.gofor a similar check). This also allows removing the unnecessaryOr()wrapper.🔧 Proposed fix
- return tx.Nodes().Filter(query.And( - query.Or( - query.StringEndsWith(query.StartProperty(common.ObjectID.String()), wellknown.AdministratorsSIDSuffix.String()), - query.StringEndsWith(query.EndProperty(common.ObjectID.String()), wellknown.AdministratorsSIDSuffix.String()), - ), - )).FetchIDs(func(cursor graph.Cursor[graph.ID]) error { + return tx.Nodes().Filter( + query.StringEndsWith(query.NodeProperty(common.ObjectID.String()), wellknown.AdministratorsSIDSuffix.String()), + ).FetchIDs(func(cursor graph.Cursor[graph.ID]) error {
🤖 Fix all issues with AI agents
In `@packages/go/analysis/ad/local_groups.go`:
- Around line 66-85: The BatchOperation error is being logged and cancelled but
not propagated, causing callers to see success despite failures; update the
BatchOperation call in PostLocalGroups (and the similar block in PostCanRDP) to
return the error to the caller instead of swallowing it: when
graphDB.BatchOperation(...) returns err, remove the unconditional nil return
path, keep the slog.Error(...) and done() handling, then propagate that err up
by returning it from the enclosing function (and do the same for the other
occurrence around lines 221-240). Ensure the enclosing function signatures
already allow returning an error or update them so the error can be returned.
🧹 Nitpick comments (4)
cmd/api/src/analysis/membership_integration_test.go (1)
78-80: Replace deprecated integration test harness.Staticcheck flags
integration.NewGraphTestContextas deprecated; please migrate to the newer integration utils to avoid future breakage.cmd/api/src/analysis/ad/ad_integration_test.go (1)
1144-1146: Consider updating error message to reflect new operation.The error message
"error expanding groups in integration test"is slightly misleading since the operation is nowFetchLocalGroupData, not group expansion. This is a minor cosmetic issue.- if localGroupData, err := adAnalysis.FetchLocalGroupData(testContext.Context(), db); err != nil { - t.Fatalf("error expanding groups in integration test; %v", err) + if localGroupData, err := adAnalysis.FetchLocalGroupData(testContext.Context(), db); err != nil { + t.Fatalf("error fetching local group data in integration test; %v", err)packages/go/analysis/ad/ad.go (1)
475-483: Add a defensive guard for nilLocalGroupData.
This avoids a panic if a caller passes a nil or partially initialized struct.🛡️ Suggested guard
func CalculateCrossProductNodeSets(localGroupData *LocalGroupData, nodeSlices ...[]*graph.Node) cardinality.Duplex[uint64] { + if localGroupData == nil || localGroupData.GroupMembershipCache == nil { + slog.Error("Cross products require LocalGroupData with GroupMembershipCache") + return cardinality.NewBitmap64() + } if len(nodeSlices) < 2 { slog.Error("Cross products require at least 2 nodesets") return cardinality.NewBitmap64() }cmd/api/src/analysis/ad/ntlm_integration_test.go (1)
154-157: RenamegrouplocalGroupDatatolocalGroupDatafor clarity.
The current name reads like a typo and makes the flow harder to scan.
5 participants
Description
Backmerge v8.5.1 release stage branch into main
Motivation and Context
Resolves BED-7201
How Has This Been Tested?
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Security
✏️ Tip: You can customize this high-level summary in your review settings.