fix: PATCH AssetTagSelector shouldn't ignore explicit empty seeds array in payload BED-7144 #2252
+19
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughThe UpdateAssetGroupTagSelector function now checks if Seeds is non-nil instead of checking if its length exceeds zero, causing empty seed slices to trigger validation logic rather than being skipped. A corresponding test case was added to verify this behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/api/src/api/v2/assetgrouptags_test.go`:
- Around line 946-963: The test fails early because UpdateAssetGroupTagSelector
parses api.URIPathVariableAssetGroupTagID using strconv.Atoi and "non-numeric"
causes a 404 before seed validation; update the test case
"ExplicitlySuppliedEmptySeedArray" to supply valid numeric URL vars (e.g.,
numeric strings for api.URIPathVariableAssetGroupTagID and
api.URIPathVariableAssetGroupTagSelectorID), set up any required mocks for
loading the asset group tag/selector so execution reaches validateSelectorSeeds,
and change the expected status to http.StatusBadRequest with the body assertion
still checking for "seeds are required".
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/api/src/api/v2/assetgrouptags.gocmd/api/src/api/v2/assetgrouptags_test.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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
🧬 Code graph analysis (1)
cmd/api/src/api/v2/assetgrouptags_test.go (1)
cmd/api/src/model/assetgrouptags.go (4)
AssetGroupTagSelector(225-241)AssetGroupTagSelector(243-245)SelectorSeed(202-206)SelectorSeed(208-210)
⏰ 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: build-ui
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: run-go-unit-tests
🔇 Additional comments (1)
cmd/api/src/api/v2/assetgrouptags.go (1)
303-313: LGTM! Correctly distinguishes nil vs empty slice.The change from
len(selUpdateReq.Seeds) > 0toselUpdateReq.Seeds != nilproperly handles the semantic difference between:
nilSeeds (field omitted from payload → preserve existing seeds)- Empty slice (explicit
"seeds": []in payload → validate and reject)This ensures the API correctly validates explicitly provided empty arrays instead of silently ignoring them.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines
+946
to
+963
| { | ||
| Name: "ExplicitlySuppliedEmptySeedArray", | ||
| Input: func(input *apitest.Input) { | ||
| apitest.SetContext(input, userCtx) | ||
| apitest.SetURLVar(input, api.URIPathVariableAssetGroupTagID, "non-numeric") | ||
| apitest.SetURLVar(input, api.URIPathVariableAssetGroupTagSelectorID, "1") | ||
| apitest.BodyStruct(input, model.AssetGroupTagSelector{ | ||
| Name: "TestSelector", | ||
| Description: "Test selector description", | ||
| Seeds: []model.SelectorSeed{}, | ||
| AutoCertify: model.SelectorAutoCertifyMethodDisabled, | ||
| }) | ||
| }, | ||
| Test: func(output apitest.Output) { | ||
| apitest.StatusCode(output, http.StatusNotFound) | ||
| apitest.BodyContains(output, "seeds are required") | ||
| }, | ||
| }, |
Contributor
There was a problem hiding this comment.
Test will fail at ID parsing, never reaching seeds validation.
The test sets api.URIPathVariableAssetGroupTagID to "non-numeric", which causes strconv.Atoi to fail at line 250-251 of UpdateAssetGroupTagSelector. This returns a 404 with "ID malformed" before the seeds validation logic is ever reached.
To properly test the empty seeds array scenario, use valid numeric IDs and set up the required mocks:
🐛 Proposed fix
{
Name: "ExplicitlySuppliedEmptySeedArray",
Input: func(input *apitest.Input) {
apitest.SetContext(input, userCtx)
- apitest.SetURLVar(input, api.URIPathVariableAssetGroupTagID, "non-numeric")
+ apitest.SetURLVar(input, api.URIPathVariableAssetGroupTagID, "1")
apitest.SetURLVar(input, api.URIPathVariableAssetGroupTagSelectorID, "1")
apitest.BodyStruct(input, model.AssetGroupTagSelector{
Name: "TestSelector",
Description: "Test selector description",
Seeds: []model.SelectorSeed{},
AutoCertify: model.SelectorAutoCertifyMethodDisabled,
})
},
+ Setup: func() {
+ mockDB.EXPECT().GetAssetGroupTag(gomock.Any(), 1).
+ Return(model.AssetGroupTag{ID: 1}, nil).Times(1)
+ mockDB.EXPECT().GetAssetGroupTagSelectorBySelectorId(gomock.Any(), 1).
+ Return(model.AssetGroupTagSelector{AssetGroupTagId: 1}, nil).Times(1)
+ },
Test: func(output apitest.Output) {
- apitest.StatusCode(output, http.StatusNotFound)
+ apitest.StatusCode(output, http.StatusBadRequest)
apitest.BodyContains(output, "seeds are required")
},
},Note: The expected status code should be http.StatusBadRequest (400) since validateSelectorSeeds errors result in a BadRequest response (see line 305 in the production code).
📝 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.
Suggested change
| { | |
| Name: "ExplicitlySuppliedEmptySeedArray", | |
| Input: func(input *apitest.Input) { | |
| apitest.SetContext(input, userCtx) | |
| apitest.SetURLVar(input, api.URIPathVariableAssetGroupTagID, "non-numeric") | |
| apitest.SetURLVar(input, api.URIPathVariableAssetGroupTagSelectorID, "1") | |
| apitest.BodyStruct(input, model.AssetGroupTagSelector{ | |
| Name: "TestSelector", | |
| Description: "Test selector description", | |
| Seeds: []model.SelectorSeed{}, | |
| AutoCertify: model.SelectorAutoCertifyMethodDisabled, | |
| }) | |
| }, | |
| Test: func(output apitest.Output) { | |
| apitest.StatusCode(output, http.StatusNotFound) | |
| apitest.BodyContains(output, "seeds are required") | |
| }, | |
| }, | |
| { | |
| Name: "ExplicitlySuppliedEmptySeedArray", | |
| Input: func(input *apitest.Input) { | |
| apitest.SetContext(input, userCtx) | |
| apitest.SetURLVar(input, api.URIPathVariableAssetGroupTagID, "1") | |
| apitest.SetURLVar(input, api.URIPathVariableAssetGroupTagSelectorID, "1") | |
| apitest.BodyStruct(input, model.AssetGroupTagSelector{ | |
| Name: "TestSelector", | |
| Description: "Test selector description", | |
| Seeds: []model.SelectorSeed{}, | |
| AutoCertify: model.SelectorAutoCertifyMethodDisabled, | |
| }) | |
| }, | |
| Setup: func() { | |
| mockDB.EXPECT().GetAssetGroupTag(gomock.Any(), 1). | |
| Return(model.AssetGroupTag{ID: 1}, nil).Times(1) | |
| mockDB.EXPECT().GetAssetGroupTagSelectorBySelectorId(gomock.Any(), 1). | |
| Return(model.AssetGroupTagSelector{AssetGroupTagId: 1}, nil).Times(1) | |
| }, | |
| Test: func(output apitest.Output) { | |
| apitest.StatusCode(output, http.StatusBadRequest) | |
| apitest.BodyContains(output, "seeds are required") | |
| }, | |
| }, |
🤖 Prompt for AI Agents
In `@cmd/api/src/api/v2/assetgrouptags_test.go` around lines 946 - 963, The test
fails early because UpdateAssetGroupTagSelector parses
api.URIPathVariableAssetGroupTagID using strconv.Atoi and "non-numeric" causes a
404 before seed validation; update the test case
"ExplicitlySuppliedEmptySeedArray" to supply valid numeric URL vars (e.g.,
numeric strings for api.URIPathVariableAssetGroupTagID and
api.URIPathVariableAssetGroupTagSelectorID), set up any required mocks for
loading the asset group tag/selector so execution reaches validateSelectorSeeds,
and change the expected status to http.StatusBadRequest with the body assertion
still checking for "seeds are required".
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Describe your changes in detail
Motivation and Context
Resolves BED-7144
Why is this change required? What problem does it solve?
How Has This Been Tested?
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
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.