chore: Merge stage/v8.4.2 release to main BED-6953 #2191
Conversation
…VE-2025-47914 (#2137) (#2143) Co-authored-by: Katherine Powderly <[email protected]>
WalkthroughThis PR introduces a feature-flag-controlled glyph rendering system for graph nodes and applies dependency updates. The tier_management_engine feature flag gates new glyph rendering logic in the Explore view, enabling tier-zero and owned-object glyph variants. Schema changes add filtered ingest-specific ACL relationships. Go dependencies are updated to newer patch and minor versions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
specter-flq
changed the title
![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 (1)
packages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.tsx (1)
52-72: Use explicit undefined check for autoCertify parameter.Line 64 uses a truthy check (
autoCertify ?) to conditionally include theauto_certifyfield. This would incorrectly exclude the value ifautoCertifyis0, which is likely the value forAssetGroupTagSelectorAutoCertifyDisabled. Use an explicit check forundefinedinstead.🔎 Proposed fix
- ...(autoCertify ? { auto_certify: autoCertify } : {}), + ...(autoCertify !== undefined ? { auto_certify: autoCertify } : {}),
🧹 Nitpick comments (2)
cmd/ui/src/views/Explore/utils.test.ts (2)
156-156: Minor typo in test description.The test description has a typo: "tier_managent_engine" should be "tier_management_engine" (missing 'e' in management).
🔎 Proposed fix
- it('determines glyphs with the tier_managent_engine feature flag disabled', () => { + it('determines glyphs with the tier_management_engine feature flag disabled', () => {
204-204: Minor typo in test description.Same typo as Line 156: "tier_managent_engine" should be "tier_management_engine".
🔎 Proposed fix
- it('determines glyphs with the tier_managent_engine feature flag enabled', () => { + it('determines glyphs with the tier_management_engine feature flag enabled', () => {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (41)
cmd/api/src/analysis/ad/ad_integration_test.go(0 hunks)cmd/api/src/analysis/ad/adcs_integration_test.go(0 hunks)cmd/api/src/analysis/ad/ntlm_integration_test.go(0 hunks)cmd/api/src/analysis/analysis_integration_test.go(0 hunks)cmd/api/src/analysis/hybrid/hybrid_integration_test.go(0 hunks)cmd/api/src/analysis/membership_integration_test.go(0 hunks)cmd/api/src/analysis/post_integration_test.go(0 hunks)cmd/api/src/api/tools/pg_test.go(0 hunks)cmd/api/src/daemons/datapipe/agt_test.go(0 hunks)cmd/api/src/database/analysisrequest_test.go(0 hunks)cmd/api/src/database/audit_test.go(0 hunks)cmd/api/src/database/auth_test.go(0 hunks)cmd/api/src/database/datapipestatus_integration_test.go(0 hunks)cmd/api/src/database/ingest_job_test.go(0 hunks)cmd/api/src/database/migration/agi_integration_test.go(0 hunks)cmd/api/src/database/migration/asset_group_constraints_test.go(0 hunks)cmd/api/src/database/migration/migration_integration_test.go(0 hunks)cmd/api/src/database/oidc_providers_test.go(0 hunks)cmd/api/src/database/parameters_test.go(0 hunks)cmd/api/src/database/saved_queries_permissions_integration_test.go(0 hunks)cmd/api/src/database/sourcekinds_integration_test.go(0 hunks)cmd/api/src/database/sso_providers_test.go(0 hunks)cmd/api/src/model/appcfg/parameter_integration_test.go(0 hunks)cmd/api/src/queries/graph_integration_test.go(0 hunks)cmd/api/src/services/graphify/ingest_integration_test.go(0 hunks)cmd/api/src/services/graphify/ingestrelationships_integration_test.go(0 hunks)cmd/ui/src/views/Explore/GraphView.tsx(3 hunks)cmd/ui/src/views/Explore/utils.test.ts(5 hunks)cmd/ui/src/views/Explore/utils.ts(4 hunks)dockerfiles/bloodhound.Dockerfile(2 hunks)go.mod(8 hunks)go.work.template(1 hunks)packages/cue/bh/ad/ad.cue(2 hunks)packages/cue/bh/bh.cue(2 hunks)packages/go/analysis/ad/ad_integration_test.go(0 hunks)packages/go/analysis/post_integration_test.go(0 hunks)packages/go/graphschema/ad/ad.go(1 hunks)packages/go/schemagen/generator/golang.go(1 hunks)packages/go/schemagen/model/schema.go(1 hunks)packages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.tsx(5 hunks)tools/docker-compose/api.Dockerfile(1 hunks)
💤 Files with no reviewable changes (28)
- cmd/api/src/analysis/membership_integration_test.go
- cmd/api/src/database/sso_providers_test.go
- cmd/api/src/analysis/post_integration_test.go
- cmd/api/src/services/graphify/ingest_integration_test.go
- cmd/api/src/database/saved_queries_permissions_integration_test.go
- packages/go/analysis/ad/ad_integration_test.go
- cmd/api/src/queries/graph_integration_test.go
- cmd/api/src/database/datapipestatus_integration_test.go
- cmd/api/src/database/ingest_job_test.go
- cmd/api/src/analysis/analysis_integration_test.go
- cmd/api/src/database/auth_test.go
- cmd/api/src/database/analysisrequest_test.go
- cmd/api/src/database/sourcekinds_integration_test.go
- cmd/api/src/daemons/datapipe/agt_test.go
- cmd/api/src/database/audit_test.go
- cmd/api/src/database/oidc_providers_test.go
- packages/go/analysis/post_integration_test.go
- cmd/api/src/api/tools/pg_test.go
- cmd/api/src/analysis/ad/ad_integration_test.go
- cmd/api/src/analysis/ad/ntlm_integration_test.go
- cmd/api/src/services/graphify/ingestrelationships_integration_test.go
- cmd/api/src/model/appcfg/parameter_integration_test.go
- cmd/api/src/database/migration/agi_integration_test.go
- cmd/api/src/database/migration/migration_integration_test.go
- cmd/api/src/database/migration/asset_group_constraints_test.go
- cmd/api/src/analysis/hybrid/hybrid_integration_test.go
- cmd/api/src/database/parameters_test.go
- cmd/api/src/analysis/ad/adcs_integration_test.go
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1629
File: go.mod:18-18
Timestamp: 2025-06-30T22:58:54.834Z
Learning: Go 1.24 supports patch versions in the go directive of go.mod files. Syntax like "go 1.24.4" is valid and specifies the minimum Go version required to build the module, including the patch level.
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
Applied to files:
cmd/ui/src/views/Explore/utils.ts
📚 Learning: 2025-08-11T23:51:32.709Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1749
File: packages/javascript/bh-shared-ui/src/hooks/useExploreSelectedItem.tsx:40-46
Timestamp: 2025-08-11T23:51:32.709Z
Learning: The useGraphItem hook in packages/javascript/bh-shared-ui/src/hooks/useGraphItem.tsx already has an enabled field set to !!itemId, which prevents the query from executing when itemId is falsy, providing built-in null-safety.
Applied to files:
cmd/ui/src/views/Explore/GraphView.tsx
📚 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:
cmd/ui/src/views/Explore/GraphView.tsxpackages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.tsx
📚 Learning: 2025-09-08T19:22:49.284Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.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/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.tsx
📚 Learning: 2025-09-08T19:38:54.755Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx:27-40
Timestamp: 2025-09-08T19:38:54.755Z
Learning: In BloodHound's ConfirmUpdateQueryDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/ConfirmUpdateQueryDialog.tsx), the nested Dialog structure with DialogTitle and DialogActions inside DialogContent (rather than as direct siblings of Dialog) is intentional design and should not be changed to follow conventional MUI patterns.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.tsx
📚 Learning: 2025-09-24T16:36:12.870Z
Learnt from: dcairnsspecterops
Repo: SpecterOps/BloodHound PR: 1923
File: packages/javascript/bh-shared-ui/src/components/FileUploadDialog/FileUploadDialog.tsx:55-58
Timestamp: 2025-09-24T16:36:12.870Z
Learning: In BloodHound's FileUploadDialog component (packages/javascript/bh-shared-ui/src/components/FileUploadDialog/FileUploadDialog.tsx), the useOnClickOutside hook with dialogRef on the Dialog root is specifically intended to detect navbar clicks, not backdrop clicks. Backdrop clicks are already handled by MUI Dialog's built-in onClose behavior, while the hook handles navbar interactions to allow closing the dialog when users interact with navigation elements.
Applied to files:
packages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.tsx
📚 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:
packages/go/schemagen/generator/golang.gopackages/cue/bh/ad/ad.cuedockerfiles/bloodhound.Dockerfilego.mod
📚 Learning: 2025-06-30T22:58:54.834Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1629
File: go.mod:18-18
Timestamp: 2025-06-30T22:58:54.834Z
Learning: Go 1.24 supports patch versions in the go directive of go.mod files. Syntax like "go 1.24.4" is valid and specifies the minimum Go version required to build the module, including the patch level.
Applied to files:
go.work.templatego.mod
📚 Learning: 2025-08-26T18:13:36.643Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1820
File: .github/workflows/audit-release.yml:0-0
Timestamp: 2025-08-26T18:13:36.643Z
Learning: The go tool command supports custom/third-party binaries as of Go 1.24, allowing invocation of custom CLI tools like "go tool stbernard" rather than being limited to only built-in Go toolchain binaries.
Applied to files:
go.work.template
📚 Learning: 2025-08-28T16:43:43.961Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: packages/go/openapi/doc/openapi.json:18008-18029
Timestamp: 2025-08-28T16:43:43.961Z
Learning: In SpecterOps/BloodHound, packages/go/openapi/doc/openapi.json is generated from YAML under packages/go/openapi/src/schemas; edits must be made to the YAML and then the spec regenerated.
Applied to files:
go.mod
🧬 Code graph analysis (4)
cmd/ui/src/views/Explore/utils.test.ts (1)
cmd/ui/src/views/Explore/utils.ts (1)
getNodeGlyphs(117-161)
packages/go/schemagen/model/schema.go (1)
packages/go/graphschema/ad/ad.go (1)
IngestACLRelationships(1155-1157)
packages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.tsx (2)
packages/javascript/bh-shared-ui/src/hooks/useGraphItem.tsx (1)
NodeResponse(28-32)packages/javascript/js-client-library/src/types.ts (2)
AssetGroupTagSelectorAutoCertifyType(175-178)AssetGroupTagSelectorAutoCertifySeedsOnly(172-172)
packages/go/schemagen/generator/golang.go (2)
packages/go/graphschema/ad/ad.go (1)
IngestACLRelationships(1155-1157)packages/go/schemagen/csgen/models.go (1)
Symbol(23-23)
🔇 Additional comments (26)
packages/javascript/bh-shared-ui/src/views/Explore/ContextMenu/ContextMenuPrivilegeZonesEnabled.tsx (3)
19-23: LGTM: Imports are correctly sourced.The imports for
AssetGroupTagSelectorAutoCertifyTypeand related constants fromjs-client-libraryare appropriate and align with their usage in the mutation function.
106-122: LGTM: Clean higher-order function pattern.The
makeHandleAddNodefunction provides a clean abstraction that captures theautoCertifycontext and returns a handler with the appropriate configuration. The loading state check and dialog lifecycle management are correctly implemented.
163-163: LGTM: Appropriate auto-certification strategies applied.The usage correctly applies different auto-certification strategies:
- Tier Zero assets use
AssetGroupTagSelectorAutoCertifySeedsOnlyto auto-certify seed nodes- Owned assets omit the parameter, allowing the backend to apply default behavior
Also applies to: 188-188
cmd/ui/src/views/Explore/utils.ts (5)
17-32: LGTM! Imports are well-organized.The new imports for glyph rendering (FontAwesome icons, glyph utilities, and types) are appropriate and properly used throughout the file.
65-65: LGTM! Type definition is appropriate.The
pzFeatureFlagEnabled: boolean | undefinedtype correctly handles cases where the feature flag may not be loaded or available.
102-115: LGTM! GLYPHS constant is well-structured.The glyph definitions provide appropriate light/dark variants for both tier zero and owned object icons, ensuring proper visibility across themes.
117-161: LGTM! Feature flag logic is well-implemented.The
getNodeGlyphsfunction cleanly separates the new feature-flag-driven path from the legacy behavior, with appropriate glyph positioning and styling for both modes.
185-189: LGTM! Refactoring improves code organization.The extraction of glyph logic into
getNodeGlyphsimproves maintainability and testability. The conditional application of glyphs when the array is non-empty is appropriate.cmd/ui/src/views/Explore/GraphView.tsx (3)
39-39: LGTM! Import is correct.The
useFeatureFlaghook import is properly placed with other bh-shared-ui imports.
83-83: LGTM! Feature flag retrieval is correct.The hook correctly fetches the
tier_management_enginefeature flag with appropriate naming.
96-105: LGTM! Graph options integration is correct.The feature flag is properly integrated into
graphOptionswith appropriate optional chaining and dependency tracking. The memo will correctly recompute when the feature flag state changes.cmd/ui/src/views/Explore/utils.test.ts (4)
19-20: LGTM! Test imports are appropriate.The imports for
GlyphLocationandgetNodeGlyphssupport the new test suite.
28-41: LGTM! Test data properly supports both feature flag paths.The updated test nodes correctly include both the legacy flags (
isTierZero,isOwnedObject) and the new kinds-based tags (Tag_Tier_Zero,Tag_Owned), enabling comprehensive testing of both code paths.
87-119: LGTM! Tests updated to include feature flag.The existing
initGraphtests correctly pass the newpzFeatureFlagEnabledoption, maintaining test validity while the new glyph-specific behavior is properly tested in the dedicatedgetNodeGlyphssuite.
123-251: LGTM! Comprehensive test coverage for glyph behavior.The test suite thoroughly validates both feature flag states, verifying glyph positioning, styling, and image selection for both tier zero and owned object nodes. Excellent test coverage!
go.work.template (1)
1-1: LGTM! Go toolchain version bump is consistent with the release.The update from 1.24.9 to 1.24.11 aligns with similar changes across go.mod and Dockerfiles.
tools/docker-compose/api.Dockerfile (1)
64-64: LGTM! Docker base image update aligns with Go toolchain bump.The update to golang:1.24.11-alpine3.22 is consistent with the Go version updates in go.mod and go.work.template.
packages/go/graphschema/ad/ad.go (1)
1155-1157: LGTM! New IngestACLRelationships function provides an ingest-focused subset.The function returns a curated subset of ACL relationships, excluding WriteOwner, Owns, SyncLAPSPassword, and DCSync from the full ACLRelationships set. This is generated code from CUE schemas and follows the established pattern.
go.mod (2)
18-18: LGTM! Go version update is valid.The bump to 1.24.11 is consistent with the toolchain updates across the repository. Based on learnings, Go 1.24 supports patch versions in the go directive.
23-23: The specified RoaringBitmap version v2.14.4 does not appear to exist.The RoaringBitmap/roaring repository shows v2.4.5 as the latest release as of early February. Version numbering jumps from v2.4.x directly to much lower versions in the public releases. The claim about updating from v2.6.0 to v2.14.4 cannot be verified as v2.14.4 does not appear in any official releases, GitHub tags, or pkg.go.dev documentation.
Confirm the correct version number and re-evaluate the dependency update accordingly.
dockerfiles/bloodhound.Dockerfile (1)
75-75: LGTM! Docker base images updated consistently.Both build stages (ldflag-builder and api-builder) now use golang:1.24.11-alpine3.22, aligning with the Go toolchain updates across the repository.
Also applies to: 97-97
packages/go/schemagen/model/schema.go (1)
67-67: LGTM! New field supports IngestACLRelationships schema.The IngestACLRelationships field is properly positioned and typed, consistent with the existing ACLRelationships field. This drives the code generation for the new IngestACLRelationships API.
packages/cue/bh/bh.cue (1)
53-53: LGTM! CUE schema properly defines IngestACLRelationships.The field is correctly typed as
[...types.#Kind]and properly wired toad.IngestACLRelationships, following the same pattern as ACLRelationships.Also applies to: 88-88
packages/go/schemagen/generator/golang.go (1)
397-405: LGTM! Codegen for IngestACLRelationships follows established pattern.The function generation mirrors the ACLRelationships codegen structure and properly iterates over
adSchema.IngestACLRelationshipsto emit relationship symbols. The placement between ACLRelationships and PathfindingRelationships is logical.packages/cue/bh/ad/ad.cue (2)
29-29: LGTM! Clean schema export addition.The new
IngestACLRelationshipsexport follows the established pattern and is properly placed alongside the relatedACLRelationshipsdeclaration.
1842-1843: LGTM! Correct list comprehension logic.The computed definition properly filters
ACLRelationshipsto exclude post-processed relationships, creating a clean separation between ingest-time and post-processing ACL relationships. The CUE list comprehension syntax is correct and the logic aligns with the schema's design.
|
|
||
| require ( | ||
| cuelang.org/go v0.13.2 | ||
| cuelang.org/go v0.15.1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the repo structure and find CUE-related processes
echo "=== Looking for CUE-related build/generation files ==="
fd -t f -e justfile -e Makefile | head -20
echo ""
echo "=== Checking justfile for CUE or schema generation commands ==="
if [ -f justfile ]; then
cat justfile | head -100
fiRepository: SpecterOps/BloodHound
Length of output: 3597
🌐 Web query:
cuelang.org/go v0.15.1 breaking changes release notes
💡 Result:
Summary — cuelang.org/go v0.15.1 (release v0.15.1)
- No new breaking changes introduced in v0.15.1. The release is a bugfix/minor update (evaluator fixes, LSP and cmd/cue fixes, a subsume bugfix, and renaming an internal build tag to avoid conflicts). (github.com)
Context (for breaking changes you may care about)
- Breaking/large changes were introduced earlier in v0.15.0 (language experiments like explicitopen, removal of evalv2, etc.); those are not new to v0.15.1. (github.com)
If you want, I can paste the full changelog entries or link specific commits/tests from the release.
Citations:
🏁 Script executed:
# Check for generated schema files and see if they were modified in this change
echo "=== Looking for CUE-generated schema files ==="
fd -t f 'azure|schema' packages/go/graphschema/ 2>/dev/null | head -20
echo ""
echo "=== Checking if azure.go exists and its size ==="
if [ -f "packages/go/graphschema/azure/azure.go" ]; then
wc -l packages/go/graphschema/azure/azure.go
echo ""
echo "=== First 50 lines of azure.go to verify it's generated ==="
head -50 packages/go/graphschema/azure/azure.go
fi
echo ""
echo "=== Checking for CUE source files ==="
fd -t f -e 'cue' packages/go/graphschema/ 2>/dev/null | head -20Repository: SpecterOps/BloodHound
Length of output: 2887
🏁 Script executed:
# Look for any indication of code generation success
echo "=== Checking for stbernard tool and generation-related files ==="
fd -t f 'stbernard|generate' | grep -E '\.(go|mod|yaml|yml)$' | head -20
echo ""
echo "=== Checking packages/go directory structure ==="
fd -t d . packages/go/ | head -20Repository: SpecterOps/BloodHound
Length of output: 883
Verify compatibility with cuelang.org/go v0.15.0 breaking changes.
The update includes v0.15.0, which introduced breaking changes (language experiments like explicitopen, and removal of evalv2). Confirm these changes don't impact the CUE schema generation pipeline. Note that v0.15.1 itself is a bugfix release with no new breaking changes.
🤖 Prompt for AI Agents
In go.mod around line 21, the repo now references cuelang.org/go v0.15.1 but
v0.15.0 introduced breaking changes (explicitopen, removal of evalv2); verify
our CUE schema generation pipeline isn't affected by searching for any
imports/usages of evalv2 or APIs changed in v0.15.0, run a full build and
unit/integration tests (go test ./...), run go vet and try generating the CUE
schemas end-to-end, and if you find breakages either update the code to use the
new API equivalents (replace evalv2 usage with the new eval API and adapt
explicit-open behavior) or pin go.mod to the last compatible pre-0.15.x version;
after changes, run go mod tidy and commit updated go.sum.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
cmd/api/src/api/v2/saved_queries_permissions_test.go (1)
363-363: Fix typo in test name.The test name contains "priavte" which should be "private".
🔎 Proposed fix
- t.Run("Admin (non-admin owned), shared query set to priavte error", func(t *testing.T) { + t.Run("Admin (non-admin owned), shared query set to private error", func(t *testing.T) {cmd/api/src/queries/graph_internal_test.go (1)
249-251: Test assertions verify input extraction rather than output correctness.The assertions compare
expectedName,expectedObjectId, andexpectedDistinguishedNameagainst string literals, which only verifies thatGet().String()works on the input properties. They should instead verify that theactualoutput contains the expected values, as done inTest_filterNodesToSearchResult_default.🔎 Proposed fix
- require.Equal(t, expectedName, "this is a name") - require.Equal(t, expectedObjectId, "object id") - require.Equal(t, expectedDistinguishedName, "ze most distinguished") + require.Equal(t, expectedName, actual[0].Name) + require.Equal(t, expectedObjectId, actual[0].ObjectID) + require.Equal(t, expectedDistinguishedName, actual[0].DistinguishedName)cmd/api/src/bootstrap/server.go (1)
75-128: Critical bug: password digest computed before password is generated.The
secretDigestis computed on line 80 usingcfg.DefaultAdmin.Password, which may be empty at that point. However,defaultAdminFunction()is called later (lines 114-118) to populate the password. This means the stored digest will be for an empty string, not the generated password.The password generation must happen before the digest is computed:
🔎 Proposed fix
func CreateDefaultAdmin(ctx context.Context, cfg config.Configuration, db database.Database, defaultAdminFunction func() (config.DefaultAdminConfiguration, error)) error { var secretDigester = cfg.Crypto.Argon2.NewDigester() + var needsLog = false + + if cfg.DefaultAdmin.Password == "" { + needsLog = true + if admin, err := defaultAdminFunction(); err != nil { + return fmt.Errorf("error in setup initializing auth secret: %w", err) + } else { + cfg.DefaultAdmin = admin + } + } if roles, err := db.GetAllRoles(ctx, "", model.SQLFilter{}); err != nil { return fmt.Errorf("error while attempting to fetch user roles: %w", err) } else if secretDigest, err := secretDigester.Digest(cfg.DefaultAdmin.Password); err != nil { return fmt.Errorf("error while attempting to digest secret for user: %w", err) } else if adminRole, found := roles.FindByName(auth.RoleAdministrator); !found { return fmt.Errorf("unable to find admin role") } else { // ... existing user lookup/delete logic ... var ( adminUser = model.User{ // ... } authSecret = model.AuthSecret{ Digest: secretDigest.String(), DigestMethod: secretDigester.Method(), } - needsLog = false ) - - if cfg.DefaultAdmin.Password == "" { - needsLog = true - if admin, err := defaultAdminFunction(); err != nil { - return fmt.Errorf("error in setup initalizing auth secret: %w", err) - } else { - cfg.DefaultAdmin = admin - } - }cmd/api/src/api/v2/assetgrouptags.go (1)
1188-1244: Fix compile-time bug in fuzzy history filter loop
assetGroupTagHistoryImplementationcurrently uses:for range len(queryableColumns) { sqlFilter.Params = append(sqlFilter.Params, fuzzyQueryParams) }You can’t
rangeover anintin Go; this will not compile.Change the loop to iterate over the slice itself so the fuzzy params are appended once per queryable column:
Proposed fix
- sqlFilter.SQLString += querySQL - for range len(queryableColumns) { - sqlFilter.Params = append(sqlFilter.Params, fuzzyQueryParams) - } + sqlFilter.SQLString += querySQL + for range queryableColumns { + sqlFilter.Params = append(sqlFilter.Params, fuzzyQueryParams) + }cmd/api/src/api/v2/assetgrouptags_test.go (1)
1056-1089: UpdateAssetGroupTagSelector “DatabaseError” test is missing a PrepareCypherQuery expectationIn the
DatabaseErrorcase forUpdateAssetGroupTagSelector, the request body includes Cypher seeds:Seeds: []model.SelectorSeed{ {Type: model.SelectorTypeCypher, Value: "this should be a string of cypher"}, },The handler’s
validateSelectorSeedswill calls.GraphQuery.PrepareCypherQueryfor Cypher‑typed seeds, but this test’sSetuponly configuresUpdateAssetGroupTagSelector,GetAssetGroupTag, andGetAssetGroupTagSelectorBySelectorIdexpectations. With gomock’s default strictness, the call toPrepareCypherQuerywill be an unexpected method call and cause the test to fail.Add a
PrepareCypherQueryexpectation similar to your other tests:Suggested adjustment
{ Name: "DatabaseError", @@ }, Setup: func() { - mockDB.EXPECT().UpdateAssetGroupTagSelector( + mockGraphDb.EXPECT(). + PrepareCypherQuery(gomock.Any(), int64(queries.DefaultQueryFitnessLowerBoundSelector)). + Return(queries.PreparedQuery{}, nil).Times(1) + + mockDB.EXPECT().UpdateAssetGroupTagSelector( gomock.Any(), uuid.Nil.String(), "[email protected]", @@ ). Return(model.AssetGroupTagSelector{}, errors.New("failure")).Times(1)
🧹 Nitpick comments (16)
cmd/ui/src/views/Administration/Administration.test.tsx (2)
72-78: LGTM! Consider reducing redundant queries.The test correctly validates both the link's presence and its href attribute using the dynamic route-based aria-label pattern (consistent with the AppLink component's fallback behavior per retrieved learnings).
For slightly improved efficiency, you could store the element reference once:
Optional refactor to reduce DOM queries
- expect( - screen.getByRole('link', { name: `Navigate to ${ROUTE_ADMINISTRATION_DATA_QUALITY}` }) - ).toBeInTheDocument(); - expect(screen.getByRole('link', { name: `Navigate to ${ROUTE_ADMINISTRATION_DATA_QUALITY}` })).toHaveAttribute( + const dataQualityLink = screen.getByRole('link', { name: `Navigate to ${ROUTE_ADMINISTRATION_DATA_QUALITY}` }); + expect(dataQualityLink).toBeInTheDocument(); + expect(dataQualityLink).toHaveAttribute( 'href', ROUTE_ADMINISTRATION_DATA_QUALITY );
95-100: LGTM! Async handling is appropriate, but consider reusing the element reference.The use of
findByRole(async) correctly handles the SSO configuration link appearing asynchronously (likely after the features API response). However, line 99 re-queries the DOM for the same element.Optional refactor to reuse element reference
- expect( - await screen.findByRole('link', { name: `Navigate to ${ROUTE_ADMINISTRATION_SSO_CONFIGURATION}` }) - ).toBeInTheDocument(); - expect( - screen.getByRole('link', { name: `Navigate to ${ROUTE_ADMINISTRATION_SSO_CONFIGURATION}` }) - ).toHaveAttribute('href', ROUTE_ADMINISTRATION_SSO_CONFIGURATION); + const ssoLink = await screen.findByRole('link', { name: `Navigate to ${ROUTE_ADMINISTRATION_SSO_CONFIGURATION}` }); + expect(ssoLink).toBeInTheDocument(); + expect(ssoLink).toHaveAttribute('href', ROUTE_ADMINISTRATION_SSO_CONFIGURATION);cmd/ui/src/views/Explore/GraphItemInformationPanel.tsx (1)
27-29: Consider simplifying the type annotation.The type annotation
HTMLProps<HTMLElement>['className']is verbose and could be simplified to juststring. The HTMLProps import would then be unnecessary.🔎 Proposed simplification
-import { HTMLProps } from 'react'; - -const defaultClasses: HTMLProps<HTMLElement>['className'] = 'bottom-0 top-0 py-4 absolute right-4'; +const defaultClasses = 'bottom-0 top-0 py-4 absolute right-4';cmd/api/src/api/v2/graphschema.go (1)
68-68: Minor: Use camelCase for variable names per Go conventions.The variable
schema_name_filteruses snake_case, but Go idiom prefers camelCase for local variables.🔎 Suggested fix
- for i, schema_name_filter := range queryFilters["schema_names"] { - schema_name_filter.SetOperator = model.FilterOr - translatedFilters["schema.name"][i] = schema_name_filter + for i, schemaNameFilter := range queryFilters["schema_names"] { + schemaNameFilter.SetOperator = model.FilterOr + translatedFilters["schema.name"][i] = schemaNameFiltercmd/api/src/database/helper.go (1)
179-186: Add a default case to handle unexpected sort directions.The switch covers
AscendingSortDirection,DescendingSortDirection, andInvalidSortDirection, but lacks adefaultcase. If a newSortDirectionenum value is added in the future, it would silently produce an emptydirString, resulting in malformed SQL (e.g.,"column_name ").🔎 Suggested fix
switch item.Direction { case model.AscendingSortDirection: dirString = "ASC" case model.DescendingSortDirection: dirString = "DESC" case model.InvalidSortDirection: return "", ErrInvalidSortDirection + default: + return "", ErrInvalidSortDirection }cmd/api/src/queries/graph_integration_test.go (2)
157-161: Reflection access may panic if field doesn't exist.Using
reflect.ValueOf(expectedUser).FieldByName(testCase.matchUserField)could return a zeroValueif the field doesn't exist, and calling.String()on it would panic. Consider adding a validity check.🔎 Proposed safer reflection access
if testCase.shouldMatchUser { expectedUser := results[0] value := reflect.ValueOf(expectedUser) - require.Equal(t, value.FieldByName(testCase.matchUserField).String(), testCase.queryString) + field := value.FieldByName(testCase.matchUserField) + require.True(t, field.IsValid(), "field %s not found", testCase.matchUserField) + require.Equal(t, field.String(), testCase.queryString) }
253-261: Map access without nil/existence check.Accessing
actualResult.Properties.Map[testCase.matchUserField]without checking if the key exists could returnnil, which may cause unexpected behavior in the equality assertion.🔎 Proposed safer map access
if testCase.shouldMatchUser { var actualResult *graph.Node for _, node := range results { actualResult = node break } - actualValue := actualResult.Properties.Map[testCase.matchUserField] - require.Equal(t, actualValue, testCase.searchValue) + require.NotNil(t, actualResult, "expected at least one result") + actualValue, ok := actualResult.Properties.Map[testCase.matchUserField] + require.True(t, ok, "field %s not found in properties", testCase.matchUserField) + require.Equal(t, actualValue, testCase.searchValue) }cmd/api/src/api/v2/search.go (1)
47-58: Consider early return pattern for cleaner control flow.The nested if-else structure with the user check could be simplified using early returns, which is more idiomatic Go.
🔎 Proposed refactor for cleaner control flow
- if user, isUser := auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx); !isUser { + user, isUser := auth.GetUserFromAuthCtx(bhCtx.FromRequest(request).AuthCtx) + if !isUser { api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusBadRequest, "no associated user found with request", request), response) return - } else { - // ETAC feature flag - if etacFlag, err := s.DB.GetFlagByKey(request.Context(), appcfg.FeatureETAC); err != nil { - api.HandleDatabaseError(request, response, err) - return - } else if etacFlag.Enabled && !user.AllEnvironments { - etacAllowedList = ExtractEnvironmentIDsFromUser(&user) - } } + + // ETAC feature flag + if etacFlag, err := s.DB.GetFlagByKey(request.Context(), appcfg.FeatureETAC); err != nil { + api.HandleDatabaseError(request, response, err) + return + } else if etacFlag.Enabled && !user.AllEnvironments { + etacAllowedList = ExtractEnvironmentIDsFromUser(&user) + }cmd/api/src/queries/queries_integration_test.go (2)
145-149: Close error is discarded.
GraphDB.Closemay return an error that is currently ignored. Consider logging or checking the error, especially in test teardown where lingering issues could affect subsequent tests.🔎 Proposed fix
func teardownIntegrationTestSuite(t *testing.T, suite *IntegrationTestSuite) { t.Helper() - suite.GraphDB.Close(suite.Context) + if err := suite.GraphDB.Close(suite.Context); err != nil { + t.Logf("warning: failed to close graph database: %v", err) + } }
39-46: Acknowledged code duplication with a TODO note.The NOTE comment indicates this setup logic is duplicated from
graphify_integration_test(). Consider extracting shared test utilities to a common location (e.g.,test/integration/utils) to reduce maintenance burden as more integration tests are added.cmd/api/src/database/graphschema_integration_test.go (2)
132-134: Consider addingt.Parallel()for test isolation.Unlike other test functions in this file,
TestDatabase_GetGraphSchemaExtensionsdoesn't callt.Parallel(). If this is intentional due to shared state concerns, consider adding a comment. Otherwise, addingt.Parallel()would be consistent with other tests.
1283-1305: Inconsistent expected data initialization.The second
SchemaEnvironmentin the expected result has explicitBasicstruct with dates that will be zeroed out during comparison anyway (lines 1319-1322). This creates unnecessary noise and inconsistency with the first item. Consider simplifying:🔎 Suggested simplification
res: []model.SchemaEnvironment{ { Serial: model.Serial{ ID: 1, }, SchemaExtensionId: 1, EnvironmentKindId: 1, SourceKindId: 1, }, { Serial: model.Serial{ ID: 2, - Basic: model.Basic{ - CreatedAt: time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), - UpdatedAt: time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), - DeletedAt: sql.NullTime{Time: time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), Valid: false}, - }, }, SchemaExtensionId: 1, EnvironmentKindId: 2, SourceKindId: 2, }, },cmd/api/src/api/v2/search_test.go (1)
37-217: Clarify ETAC flag handling in “No User” SearchHandler testThe SearchHandler tests comprehensively exercise OpenGraphSearch + ETAC interactions, but the case
Name: "Fail -- ETAC Feature Flag On, No User"never configuresmockDB.GetFlagByKeyforappcfg.FeatureETAC, whereas all other ETAC‑specific cases do.If
SearchHandlerdetermines ETAC enablement viaGetFlagByKey, this test either:
- won’t actually exercise the “ETAC on” branch, or
- will panic on an unexpected mock call.
Consider either:
- adding an explicit
GetFlagByKey(..., appcfg.FeatureETAC)expectation returningEnabled: true, or- renaming the case to avoid implying the feature flag is on if the handler doesn’t consult ETAC in this path.
cmd/api/src/api/v2/assetgrouptags_test.go (1)
3247-3317: Align tag types in “success – query by name type label” SearchAssetGroupTags testIn the “success – query by name type label” test,
myTagsis initialized withType: model.AssetGroupTagTypeTier:myTags := model.AssetGroupTags{{Name: "test label", Type: model.AssetGroupTagTypeTier}} myKinds := graph.Kinds{myTags[0].ToKind()}but the mocked
GetAssetGroupTagsreturn value usesType: model.AssetGroupTagTypeLabel. That mismatch doesn’t currently break the test (sinceToKindlikely derives the graph kind from the name), but it makes the intent harder to follow.For clarity and to avoid future surprises if
ToKindstarts depending on tag type, consider makingmyTags’ type consistent with the mocked DB tag (i.e.,AssetGroupTagTypeLabelin both places).cmd/api/src/database/migration/migrations/v8.5.0.sql (1)
92-102: Consider reordering PRIMARY KEY clause for consistency.The
PRIMARY KEY (id)clause is placed after thecreated_at/updated_atcolumns, which differs from other tables in this migration wherePRIMARY KEYcomes earlier (e.g., lines 39, 86, 97). While syntactically valid, consistent ordering improves readability.That said, the migration is idempotent and correctly structured with appropriate foreign keys, indexes, and constraints.
cmd/api/src/database/graphschema.go (1)
58-58: Consider using a constant or error type instead of string matching.The
DuplicateKeyValueErrorStringconstant is used for detecting PostgreSQL unique constraint violations by checking if the error message contains this substring. While this works, it's fragile against database error message changes.Consider using a more robust approach like checking the PostgreSQL error code (e.g.,
23505for unique_violation) if available through the database driver.
| run: | | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| shopt -s extglob | ||
| # Trim the title in case there is whitespace surrounding the issue | ||
| PR_TITLE="${{ github.event.pull_request.title }}" | ||
| PR_TITLE="${PR_TITLE##*( )}" | ||
| TRIMMED_TITLE="${PR_TITLE%%*( )}" |
There was a problem hiding this comment.
Fix script injection vulnerability.
The PR title is directly interpolated into the bash script on line 85, which creates a script injection risk. User-controlled input should be passed through an environment variable instead.
🔎 Proposed fix
- name: Check Issue Reference Exists
id: "ref-check"
env:
+ PR_TITLE: ${{ github.event.pull_request.title }}
JIRA_BASE_URL: ${{ secrets.JIRA_BASE_URL }}
JIRA_USER_EMAIL: ${{ secrets.JIRA_USER_EMAIL }}
JIRA_API_TOKEN: ${{ secrets.JIRA_API_TOKEN }}
run: |
#!/usr/bin/env bash
set -euo pipefail
shopt -s extglob
# Trim the title in case there is whitespace surrounding the issue
- PR_TITLE="${{ github.event.pull_request.title }}"
+ # PR_TITLE is now set via the environment variable above
PR_TITLE="${PR_TITLE##*( )}"
TRIMMED_TITLE="${PR_TITLE%%*( )}"📝 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.
| run: | | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| shopt -s extglob | |
| # Trim the title in case there is whitespace surrounding the issue | |
| PR_TITLE="${{ github.event.pull_request.title }}" | |
| PR_TITLE="${PR_TITLE##*( )}" | |
| TRIMMED_TITLE="${PR_TITLE%%*( )}" | |
| run: | | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| shopt -s extglob | |
| # Trim the title in case there is whitespace surrounding the issue | |
| # PR_TITLE is now set via the environment variable above | |
| PR_TITLE="${PR_TITLE##*( )}" | |
| TRIMMED_TITLE="${PR_TITLE%%*( )}" |
🧰 Tools
🪛 actionlint (1.7.9)
78-78: "github.event.pull_request.title" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/reference/security/secure-use#good-practices-for-mitigating-script-injection-attacks for more details
(expression)
🤖 Prompt for AI Agents
.github/workflows/reusable.conventional-commits.yml around lines 78-87:
currently the workflow interpolates github.event.pull_request.title directly
into the bash run script which creates a script-injection risk; instead, expose
the PR title via the step's env (e.g., set PR_TITLE: ${{
github.event.pull_request.title }}) and then in the run block reference the
value from the environment ($PR_TITLE) with proper double-quotes and parameter
expansion for trimming (do not inject the raw string into the script text);
ensure all expansions are quoted to prevent word-splitting and command
injection.
| t.Run("Invalid JSON Payload.", func(t *testing.T) { | ||
|
|
||
| // Create the Invalid Request Payload | ||
| payload := []string{ | ||
| `{"Public": false}`, | ||
| `Invalid JSON`, | ||
| } | ||
|
|
||
| // Create the HTTP Request | ||
| request, err := http.NewRequestWithContext(createContextWithOwnerId(userId), "PUT", fmt.Sprintf(endpoint, savedQueryId), must.MarshalJSONReader(payload)) | ||
| require.Nil(t, err) | ||
|
|
||
| // Set the Request Headers | ||
| request.Header.Set(headers.ContentType.String(), mediatypes.ApplicationJson.String()) | ||
|
|
||
| router := mux.NewRouter() | ||
| router.HandleFunc("/api/v2/saved-queries/{saved_query_id}/permissions", resources.ShareSavedQueries).Methods("PUT") | ||
|
|
||
| response := httptest.NewRecorder() | ||
|
|
||
| router.ServeHTTP(response, request) | ||
| require.Equal(t, http.StatusBadRequest, response.Code) | ||
| require.Contains(t, response.Body.String(), "could not decode limited payload request into value") | ||
|
|
||
| }) |
There was a problem hiding this comment.
Fix the invalid JSON test construction.
The payload is constructed as a []string, which will marshal to a valid JSON array ["...", "..."] rather than invalid JSON. To properly test invalid JSON handling, you should pass actual malformed JSON.
🔎 Proposed fix for testing invalid JSON
- // Create the Invalid Request Payload
- payload := []string{
- `{"Public": false}`,
- `Invalid JSON`,
- }
-
- // Create the HTTP Request
- request, err := http.NewRequestWithContext(createContextWithOwnerId(userId), "PUT", fmt.Sprintf(endpoint, savedQueryId), must.MarshalJSONReader(payload))
+ // Create the HTTP Request with invalid JSON body
+ request, err := http.NewRequestWithContext(createContextWithOwnerId(userId), "PUT", fmt.Sprintf(endpoint, savedQueryId), strings.NewReader(`{invalid json`))
require.Nil(t, err)🤖 Prompt for AI Agents
In cmd/api/src/api/v2/saved_queries_permissions_test.go around lines 1161-1185,
the test builds payload as a []string which marshals into a valid JSON array;
replace that with an actual malformed JSON payload (e.g. a []byte or io.Reader
containing invalid JSON like `{"Public": false` or `Invalid JSON`) and pass that
reader into http.NewRequestWithContext so the request body is syntactically
invalid; keep the Content-Type header as application/json and assert the same
BadRequest and error substring.
| func (s *BloodhoundDB) GetGraphSchemaEdgeKindsWithSchemaName(ctx context.Context, edgeKindFilters model.Filters, sort model.Sort, skip, limit int) (model.GraphSchemaEdgeKindsWithNamedSchema, int, error) { | ||
| var ( | ||
| schemaEdgeKinds = model.GraphSchemaEdgeKindsWithNamedSchema{} | ||
| totalRowCount int | ||
| ) | ||
|
|
||
| if filterAndPagination, err := parseFiltersAndPagination(edgeKindFilters, sort, skip, limit); err != nil { | ||
| return schemaEdgeKinds, 0, err | ||
| } else { | ||
| sqlStr := fmt.Sprintf(`SELECT edge.id, edge.name, edge.description, edge.is_traversable, schema.name as schema_name | ||
| FROM %s edge JOIN %s schema ON edge.schema_extension_id = schema.id %s %s %s`, | ||
| model.GraphSchemaEdgeKind{}.TableName(), | ||
| model.GraphSchemaExtension{}.TableName(), | ||
| filterAndPagination.WhereClause, | ||
| filterAndPagination.OrderSql, | ||
| filterAndPagination.SkipLimit) | ||
|
|
||
| if result := s.db.WithContext(ctx).Raw(sqlStr, filterAndPagination.Filter.params...).Scan(&schemaEdgeKinds); result.Error != nil { | ||
| return nil, 0, CheckError(result) | ||
| } else { | ||
| if limit > 0 || skip > 0 { | ||
| countSqlStr := fmt.Sprintf(`SELECT COUNT(*) FROM %s edge JOIN %s schema ON edge.schema_extension_id = schema.id %s`, | ||
| model.GraphSchemaEdgeKind{}.TableName(), | ||
| model.GraphSchemaExtension{}.TableName(), | ||
| filterAndPagination.WhereClause) | ||
| if countResult := s.db.WithContext(ctx).Raw(countSqlStr, filterAndPagination.Filter.params...).Scan(&totalRowCount); countResult.Error != nil { | ||
| return model.GraphSchemaEdgeKindsWithNamedSchema{}, 0, CheckError(countResult) | ||
| } | ||
| } else { | ||
| totalRowCount = len(schemaEdgeKinds) | ||
| } | ||
| } | ||
| return schemaEdgeKinds, totalRowCount, nil | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find parseFiltersAndPagination function
rg -n "parseFiltersAndPagination|buildSQLFilter" --type go cmd/api/src/database/ -A 30 | head -150Repository: SpecterOps/BloodHound
Length of output: 12185
🏁 Script executed:
#!/bin/bash
# Find parseFiltersAndPagination function definition
rg -n "func.*parseFiltersAndPagination" --type go cmd/api/src/database/ -A 50Repository: SpecterOps/BloodHound
Length of output: 2323
🏁 Script executed:
#!/bin/bash
# Get more details about buildSQLFilter to see how it generates column references
rg -n "func buildSQLFilter" --type go cmd/api/src/database/helper.go -A 80 | head -120Repository: SpecterOps/BloodHound
Length of output: 2733
🏁 Script executed:
#!/bin/bash
# Get the complete buildSQLFilter function
rg -n "func buildSQLFilter" --type go cmd/api/src/database/helper.go -A 120 | tail -70Repository: SpecterOps/BloodHound
Length of output: 2443
Filter column references require table alias qualification in JOIN queries.
The GetGraphSchemaEdgeKindsWithSchemaName method uses table aliases (edge, schema) in its JOIN. However, the buildSQLFilter function generates unqualified column identifiers (e.g., id instead of edge.id). This causes SQL ambiguity errors when filtering on columns that exist in both tables. Filters applied to this query must either use table-prefixed column names (e.g., edge.id) or only target columns unique to a single table to avoid ambiguous column reference errors.
| entries := strings.Split(config.Database.Connection, " ") | ||
|
|
||
| environmentMap := make(map[string]string) | ||
| for _, entry := range entries { | ||
| parts := strings.Split(entry, "=") | ||
| environmentMap[parts[0]] = parts[1] | ||
| } |
There was a problem hiding this comment.
Potential panic on malformed connection string entries.
If an entry in the connection string doesn't contain an = character, strings.Split(entry, "=") will return a single-element slice, causing a panic when accessing parts[1] on line 115.
🔎 Proposed defensive fix
for _, entry := range entries {
parts := strings.Split(entry, "=")
+ if len(parts) != 2 {
+ continue
+ }
environmentMap[parts[0]] = parts[1]
}📝 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.
| entries := strings.Split(config.Database.Connection, " ") | |
| environmentMap := make(map[string]string) | |
| for _, entry := range entries { | |
| parts := strings.Split(entry, "=") | |
| environmentMap[parts[0]] = parts[1] | |
| } | |
| entries := strings.Split(config.Database.Connection, " ") | |
| environmentMap := make(map[string]string) | |
| for _, entry := range entries { | |
| parts := strings.Split(entry, "=") | |
| if len(parts) != 2 { | |
| continue | |
| } | |
| environmentMap[parts[0]] = parts[1] | |
| } |
🤖 Prompt for AI Agents
In cmd/api/src/queries/queries_integration_test.go around lines 110 to 116, the
loop that splits connection string entries assumes every entry contains '=' and
directly accesses parts[1], which can panic for malformed entries; update the
loop to defensively handle entries without '=', e.g., skip or log malformed
entries, use strings.SplitN(entry, "=", 2) and check that the resulting slice
has length 2 before assigning environmentMap[key]=value, and trim spaces from
key/value to avoid empty keys.
| {/* | ||
| `inert` is a native property that tells screen readers to | ||
| disregard this non-interactive, presentational button. | ||
| It is unavailable in React 18 (enabled in v19), so this spread | ||
| workaround applies the property without triggering type errors | ||
| */} | ||
| <div ref={(node) => node && node.setAttribute('inert', '')}> | ||
| <Switch checked={darkMode} /> | ||
| </div> |
There was a problem hiding this comment.
Reconsider the accessibility pattern for the Dark Mode switch.
Marking the Switch as inert while it remains visually interactive creates a potential accessibility mismatch:
- Sighted users see a clickable Switch component
- Screen reader users are told the element should be disregarded
- The parent menu item IS interactive (via
functionHandler), but the visual Switch suggests direct interaction
This divergence between visual and assistive technology experiences may confuse users. Consider these alternatives:
- Use aria-hidden instead of
inertto hide the decorative Switch from screen readers while keeping the parent menu item's label and handler accessible - Make the Switch truly non-interactive by removing pointer events with CSS (
pointer-events: none) - Document clearly that the entire menu item (not the Switch) is the interactive element
🔎 Proposed fix using aria-hidden
<>
{'Dark Mode'}
- {/*
- `inert` is a native property that tells screen readers to
- disregard this non-interactive, presentational button.
- It is unavailable in React 18 (enabled in v19), so this spread
- workaround applies the property without triggering type errors
- */}
- <div ref={(node) => node && node.setAttribute('inert', '')}>
+ <div aria-hidden="true" style={{ pointerEvents: 'none' }}>
<Switch checked={darkMode} />
</div>
</>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/ui/src/components/MainNav/MainNavData.tsx around lines 142-150 the code
sets a DOM attribute 'inert' on the container to hide the visual Switch from
assistive tech which creates a mismatch between visual affordance and
screen-reader behavior; remove the inert setAttribute call and instead mark the
Switch as decorative by adding aria-hidden="true" on the Switch element, make
the Switch non-interactive with CSS (pointer-events: none) so users cannot
interact with it directly, and ensure the parent menu item remains the
accessible control (has role="button"/tabindex and an accessible label/handler)
so screen readers and keyboard users can operate dark mode via the parent;
update any related tests or documentation to note that the entire menu item is
the interactive target.
|
|
||
| const tagGlyphs = { Tag_Tier_Zero: 'gem', owned: 'Tag_Owned', ownedGlyph: 'skull' }; | ||
|
|
||
| it('determines glyphs with the tier_managent_engine feature flag disabled', () => { |
There was a problem hiding this comment.
Typo in test description: "tier_managent_engine" should be "tier_management_engine".
The test descriptions contain a typo - "managent" should be "management".
🔎 Proposed fix
- it('determines glyphs with the tier_managent_engine feature flag disabled', () => {
+ it('determines glyphs with the tier_management_engine feature flag disabled', () => {And similarly for line 190:
- it('determines glyphs with the tier_managent_engine feature flag enabled', () => {
+ it('determines glyphs with the tier_management_engine feature flag enabled', () => {Also applies to: 190-190
🤖 Prompt for AI Agents
In cmd/ui/src/views/Explore/utils.test.ts around lines 142 and 190, two test
descriptions contain a typo ("tier_managent_engine") — update both occurrences
to "tier_management_engine" so the test names are spelled correctly; simply
correct the string literals in the it(...) descriptions at those lines.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/api/src/bootstrap/server.go (1)
61-61: Consider consistent parameter naming.The parameter is named
defaultAdminFuncinMigrateDB(line 61) butdefaultAdminFunctioninCreateDefaultAdmin(line 75). While functionally identical, consistent naming improves readability.🔎 Proposed fix for consistency
-func CreateDefaultAdmin(ctx context.Context, cfg config.Configuration, db database.Database, defaultAdminFunction func() (config.DefaultAdminConfiguration, error)) error { +func CreateDefaultAdmin(ctx context.Context, cfg config.Configuration, db database.Database, defaultAdminFunc func() (config.DefaultAdminConfiguration, error)) error { var ( secretDigester = cfg.Crypto.Argon2.NewDigester() needsLog = false ) if cfg.DefaultAdmin.Password == "" { needsLog = true - if admin, err := defaultAdminFunction(); err != nil { + if admin, err := defaultAdminFunc(); err != nil { return fmt.Errorf("error in setup initializing auth secret: %w", err)Also applies to: 75-75
cmd/api/src/api/v2/assetgrouptags.go (1)
110-118: Minor optimization: Remove redundant map initialization.Line 110 pre-allocates
assetGroupTagCountsMap, but line 118 immediately overwrites it with the result fromGetAssetGroupTagSelectorCounts. Consider removing line 110 or changing line 118 to avoid the redundant allocation.🔎 Proposed optimization
- assetGroupTagCountsMap = make(model.AssetGroupTagCountsMap, len(tags)) + var assetGroupTagCountsMap model.AssetGroupTagCountsMap ) if paramIncludeCounts {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/api/src/api/v2/assetgrouptags.gocmd/api/src/api/v2/assetgrouptags_test.gocmd/api/src/bootstrap/server.gocmd/api/src/model/search.go
🧰 Additional context used
🧠 Learnings (8)
📚 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/assetgrouptags.gocmd/api/src/api/v2/assetgrouptags_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/assetgrouptags.gocmd/api/src/api/v2/assetgrouptags_test.go
📚 Learning: 2025-09-08T19:22:49.284Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Applied to files:
cmd/api/src/api/v2/assetgrouptags.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/assetgrouptags_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/assetgrouptags_test.go
📚 Learning: 2025-08-28T16:43:43.961Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: packages/go/openapi/doc/openapi.json:18008-18029
Timestamp: 2025-08-28T16:43:43.961Z
Learning: In SpecterOps/BloodHound, packages/go/openapi/doc/openapi.json is generated from YAML under packages/go/openapi/src/schemas; edits must be made to the YAML and then the spec regenerated.
Applied to files:
cmd/api/src/api/v2/assetgrouptags_test.go
📚 Learning: 2025-06-17T22:37:36.389Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1595
File: cmd/api/src/api/v2/saved_queries_test.go:2594-2594
Timestamp: 2025-06-17T22:37:36.389Z
Learning: In Go table-driven tests, there's a distinction between main test function parallelism and subtest parallelism. Main test functions can safely use t.Parallel() for performance benefits, but individual subtests within table-driven tests may need to run sequentially to avoid race conditions with mocks, deferred functions, or shared resources.
Applied to files:
cmd/api/src/api/v2/assetgrouptags_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/assetgrouptags_test.go
🧬 Code graph analysis (3)
cmd/api/src/model/search.go (2)
packages/go/schemagen/csgen/models.go (1)
Type(84-84)packages/go/graphschema/common/common.go (3)
Name(53-53)ObjectID(52-52)Collected(57-57)
cmd/api/src/api/v2/assetgrouptags.go (3)
cmd/api/src/model/assetgrouptags.go (3)
AssetGroupTagCounts(93-99)AssetGroupTagCountsMap(101-101)AssetGroupSelectorNode(288-300)cmd/api/src/api/filters.go (1)
GetValidFilterPredicatesAsStrings(47-57)packages/go/graphschema/common/common.go (2)
Name(53-53)ObjectID(52-52)
cmd/api/src/api/v2/assetgrouptags_test.go (6)
cmd/api/src/model/filter.go (1)
SQLFilter(98-101)packages/go/graphschema/common/common.go (4)
Name(53-53)Description(55-55)ObjectID(52-52)PrimaryKind(70-70)cmd/api/src/model/assetgrouptags.go (8)
AssetGroupTags(121-121)AssetGroupTag(103-119)AssetGroupTag(123-125)AssetGroupTagCountsMap(101-101)AssetGroupTagSelector(225-241)AssetGroupTagSelector(243-245)SelectorSeed(202-206)SelectorSeed(208-210)cmd/api/src/queries/graph.go (1)
DefaultQueryFitnessLowerBoundSelector(67-67)packages/go/graphschema/ad/ad.go (2)
User(29-29)Entity(28-28)cmd/api/src/database/db.go (1)
ErrNotFound(41-41)
🔇 Additional comments (15)
cmd/api/src/bootstrap/server.go (1)
130-130: Typo fix applied correctly.The typo "initalizing" → "initializing" has been properly fixed as noted in the previous review feedback.
cmd/api/src/model/search.go (1)
43-43: The field follows established codebase patterns and doesn't introduce new inconsistencies. The JSON naming uses snake_case (hygiene_attack_paths), which is the prevalent convention across the model files (e.g.,user_id,exposure_percent,asset_group_tag_id). TheimpactValuefield on line 42 is actually the outlier using camelCase, not the new field introducing inconsistency. Additionally, the int64 JavaScript precision concern is already documented with a comment on line 43, which appropriately warns UI consumers about the potential truncation when values exceed JavaScript'sNumber.MAX_SAFE_INTEGER.Likely an incorrect or invalid review comment.
cmd/api/src/api/v2/assetgrouptags.go (7)
828-834: LGTM: ValidFilters method correctly defines filterable fields.The filter definitions align with the AssetGroupMember struct fields and are used consistently in downstream translation logic.
947-983: Filter translation logic is correct.The function properly translates API filter names to graph properties and operators. The NotEquals handling via
query.Notwrapper is correct.
1056-1059: Verify empty environment filter handling is intentional.The code adds environment filtering without checking if
environmentIdsis empty. Based on learnings, an empty list may be intentional for ETAC filtering (filtering out all nodes when user has no environment access). Confirm this behavior is correct for this endpoint.Based on learnings, empty environment lists may be intentional security behavior.
1194-1202: Asset group tag ID filtering added correctly.The logic correctly filters by tag ID when provided. Note that if a tag ID is specified but doesn't match the requested
tag_type, it will be filtered out in the subsequent loop (line 1208), resulting in an empty response. This may be intentional but could be clarified with documentation.
64-64: Type refactoring to model package is correct.The change to reference
*model.AssetGroupTagCountsaligns with the broader refactoring that centralizes type definitions in the model package. All usages are updated consistently throughout the file.
900-914: No changes needed to filter parsing.The omission of
IsStringDatais intentional. UnlikeGetAssetGroupTagsandGetAssetGroupMembersBySelector, which build SQL filters viaBuildSQLFilter()and needIsStringDatafor proper value conversion, this code passes filters directly tobuildAssetGroupMembersByTagGraphDbFilters, which constructs graph criteria without an intermediate SQL step.IsStringDatais not required for graph query criteria.
1052-1064: SQL filter concatenation is safe and properly parameterized.The
BuildSQLFilter()method uses apgsql.Expressionbuilder andformat.SyntaxNode()to generate SQL, which properly handles parameterization. The subsequent string concatenation at lines 1052-1064 maintains this safety by using "?" placeholders with values passed through theParamsarray, following the standard parameterized query pattern.cmd/api/src/api/v2/assetgrouptags_test.go (6)
85-239: LGTM: Mock expectations correctly updated to use concrete types.The change from
gomock.Any()tomodel.SQLFilter{}provides more precise test expectations. TheAssetGroupTagCountsMapstructure correctly reflects the new count fields (Selectors, CustomSelectors, DefaultSelectors, DisabledSelectors), and theCountNodesByKindexpectations properly useToKind()for each tag.
2340-2448: Excellent test coverage for new filtering functionality.The test cases comprehensively cover filter validation and translation:
- Invalid predicate and column errors are tested
primary_kindfiltering withEqualsoperatornamefiltering withApproximatelyEquals(translated toStringContains)object_idfiltering withNotEquals(wrapped inquery.Not)All expectations correctly match the production code's filter translation logic.
2946-3058: Test coverage for node_primary_kind filtering is comprehensive.The two test cases properly verify:
- Field name translation from API
primary_kindto databasenode_primary_kind- Conditional certification filtering when
RequireCertifyis enabledThe SQL filter expectations match the production code's translation and filtering logic.
3571-3756: Excellent test coverage for new asset_group_tag_id filtering.The two new test cases comprehensively verify:
- Filtering by
asset_group_tag_idwith tier type (lines 3571-3654)- Filtering by
asset_group_tag_idwith label type (lines 3656-3756)Both tests correctly expect the SQL filter
"id = ?"to be passed toGetAssetGroupTagsand verify the corresponding selectors and members are filtered appropriately. The test data and expectations align with the production code changes.
51-51: Import addition is correct.The
graphschema/commonimport is necessary forcommon.Nameandcommon.ObjectIDconstants used in the new filter translation test expectations.
478-482: Mock expectations correctly use explicit fitness value.The change from
gomock.Any()toint64(-3)(which equalsqueries.DefaultQueryFitnessLowerBoundSelector) makes the test more precise and verifies the correct fitness value is passed.
| if cfg.DefaultAdmin.Password == "" { | ||
| needsLog = true | ||
| if admin, err := defaultAdminFunction(); err != nil { | ||
| return fmt.Errorf("error in setup initializing auth secret: %w", err) | ||
| } else { | ||
| cfg.DefaultAdmin = admin | ||
| } | ||
| } |
There was a problem hiding this comment.
Clarify error message context.
The error message at line 84 states "error in setup initializing auth secret", but at this point the code is generating default admin configuration, not initializing an auth secret. The auth secret initialization happens later at line 129.
🔎 Proposed fix for clarity
if cfg.DefaultAdmin.Password == "" {
needsLog = true
if admin, err := defaultAdminFunction(); err != nil {
- return fmt.Errorf("error in setup initializing auth secret: %w", err)
+ return fmt.Errorf("error generating default admin configuration: %w", err)
} else {
cfg.DefaultAdmin = admin
}
}🤖 Prompt for AI Agents
In cmd/api/src/bootstrap/server.go around lines 81 to 88, the error return uses
the misleading message "error in setup initializing auth secret" while this
block is generating a default admin; update the error string to accurately
reflect the operation (e.g., "error creating default admin") and keep the
existing error wrapping (%w) so callers receive the original error details.
elikmiller
force-pushed
the
stage/v8.4.2
branch
from
f4bbe44 to
767cf6e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/ui/src/views/Explore/utils.test.ts (1)
142-142: Fix typo in test descriptions.The test descriptions contain a typo: "tier_managent_engine" should be "tier_management_engine" to match the actual feature flag key.
🔎 Proposed fix
- it('determines glyphs with the tier_managent_engine feature flag disabled', () => { + it('determines glyphs with the tier_management_engine feature flag disabled', () => {And for line 190:
- it('determines glyphs with the tier_managent_engine feature flag enabled', () => { + it('determines glyphs with the tier_management_engine feature flag enabled', () => {Also applies to: 190-190
🧹 Nitpick comments (1)
cmd/ui/src/views/Explore/utils.ts (1)
117-161: LGTM! Glyph generation logic is clean and well-structured.The
getNodeGlyphsfunction properly encapsulates the glyph rendering logic with two distinct paths:
- When
pzFeatureFlagEnabledis true: usestagGlyphsfrom node kinds- When false: uses legacy
isTierZeroandisOwnedObjectboolean flagsBoth paths correctly apply themed colors and glyph locations.
💡 Optional ergonomics improvement
Consider making the
standardGlyphsparameter optional withGLYPHSas the default value to simplify call sites:export const getNodeGlyphs = ( node: GraphNode, options: GraphOptions & { themedOptions: ThemedOptions }, - standardGlyphs: typeof GLYPHS + standardGlyphs: typeof GLYPHS = GLYPHS ) => {This would allow callers to omit the third argument in most cases while still supporting test scenarios that need to inject mock glyphs.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/ui/src/views/Explore/GraphView.tsxcmd/ui/src/views/Explore/utils.test.tscmd/ui/src/views/Explore/utils.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-11T23:51:32.709Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1749
File: packages/javascript/bh-shared-ui/src/hooks/useExploreSelectedItem.tsx:40-46
Timestamp: 2025-08-11T23:51:32.709Z
Learning: The useGraphItem hook in packages/javascript/bh-shared-ui/src/hooks/useGraphItem.tsx already has an enabled field set to !!itemId, which prevents the query from executing when itemId is falsy, providing built-in null-safety.
Applied to files:
cmd/ui/src/views/Explore/GraphView.tsx
📚 Learning: 2025-07-17T16:28:00.770Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 1693
File: cmd/ui/src/views/Explore/GraphView.tsx:155-159
Timestamp: 2025-07-17T16:28:00.770Z
Learning: In the BloodHound codebase GraphView.tsx, the context menu toggle logic that switches between null and new coordinates (closing on different item clicks) is intentional, expected behavior as of the current implementation, though this may change in the future.
Applied to files:
cmd/ui/src/views/Explore/GraphView.tsx
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in packages/javascript/bh-shared-ui/src/components/AppIcon/Icons/utils.tsx already provides accessibility through VisuallyHidden labels, eliminating the need for additional accessible names on wrapper elements.
Applied to files:
cmd/ui/src/views/Explore/utils.ts
📚 Learning: 2025-08-28T18:42:47.612Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/components/Navigation/MainNav.tsx:41-56
Timestamp: 2025-08-28T18:42:47.612Z
Learning: The BaseSVG component in BloodHound's bh-shared-ui package automatically includes VisuallyHidden labels with accessible names using the pattern "app-icon-{name}", making additional aria-label or title attributes unnecessary on parent elements.
Applied to files:
cmd/ui/src/views/Explore/utils.ts
📚 Learning: 2025-09-08T19:22:49.284Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.
Applied to files:
cmd/ui/src/views/Explore/utils.tscmd/ui/src/views/Explore/utils.test.ts
🧬 Code graph analysis (2)
cmd/ui/src/views/Explore/GraphView.tsx (1)
packages/javascript/bh-shared-ui/src/hooks/useFeatureFlags.tsx (1)
useFeatureFlag(59-61)
cmd/ui/src/views/Explore/utils.test.ts (1)
cmd/ui/src/views/Explore/utils.ts (1)
getNodeGlyphs(117-161)
⏰ 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). (5)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-go-unit-tests
- GitHub Check: build-ui
- GitHub Check: run-tests
🔇 Additional comments (7)
cmd/ui/src/views/Explore/utils.ts (4)
17-26: LGTM! Imports are appropriate for the glyph rendering feature.The new imports for FontAwesome icons, glyph utilities from bh-shared-ui, and Glyph types are correctly added to support the feature-flagged glyph rendering system.
Also applies to: 30-31
65-65: LGTM! Feature flag field added to GraphOptions.The
pzFeatureFlagEnabledfield correctly usesboolean | undefinedto handle cases where the feature flag hasn't loaded yet.
102-115: LGTM! Standard glyph constants are well-defined.The GLYPHS constant provides light and dark mode variants for tier-zero and owned-object glyphs using FontAwesome icons with appropriate scaling.
185-189: LGTM! Clean refactor that improves testability.The inline glyph logic has been successfully extracted into the
getNodeGlyphsfunction, making the code more modular and easier to test.cmd/ui/src/views/Explore/GraphView.tsx (1)
38-38: LGTM! Feature flag integration is correctly implemented.The feature flag is properly retrieved using
useFeatureFlag('tier_management_engine'), passed intographOptionswith optional chaining to handle loading states, and included in the dependency array to ensure graph re-rendering when the flag changes.Also applies to: 83-83, 103-103, 105-105
cmd/ui/src/views/Explore/utils.test.ts (2)
18-20: LGTM! Test data properly extended for glyph testing.The test data now includes
kindsarrays withTag_Tier_ZeroandTag_Owned, along with correspondingisTierZeroandisOwnedObjectflags. ThepzFeatureFlagEnabledvariable is correctly passed toinitGraphcalls.Also applies to: 28-42, 76-76, 89-89, 104-104
116-237: LGTM! Comprehensive test coverage for glyph rendering.The test suite thoroughly validates both feature flag paths:
- When disabled: verifies legacy
isTierZero/isOwnedObjectbehavior- When enabled: verifies tag-based glyph selection
Each test correctly checks glyph location, themed colors, and image selection.
Description
PR to merge v8.4.2 back to main
Motivation and Context
Resolves BED-6953
How Has This Been Tested?
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.