feat: Enforce toUpper for objectids - BED-8944#202
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughCustom ChangesJSON identifier normalization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Model
participant NormalizationHelpers
participant JSONEncoder
Model->>NormalizationHelpers: copy and normalize identifiers
NormalizationHelpers->>JSONEncoder: marshal alias payload
JSONEncoder-->>Model: normalized JSON
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
models/app-fic.go (1)
36-50: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winMissing safe handling for empty
json.RawMessagefields during serialization.The
MarshalJSONimplementations forAppFIC,GroupMember, andDeviceOwnercrash when their raw JSON fields (FIC,Member,Owner) are empty ornil.
- In
AppFIC.MarshalJSON, returningnil, nildirectly violates thejson.Marshalercontract and causesjson.Marshalon the parent object to fail withunexpected end of JSON input.- In
GroupMember.MarshalJSONandDeviceOwner.MarshalJSON, passing an emptyjson.RawMessagetoOmitEmptyUpperwithout a length check causes its internaljson.Unmarshalcall to fail withunexpected end of JSON input.To resolve this safely, check the length of the
json.RawMessagebefore processing it. Additionally, these methods can be refactored to use the same elegantAliaspattern adopted elsewhere in this PR (e.g.,App,VirtualMachineOwner), which avoids unnecessary map allocations and preserves strict struct field ordering.
models/app-fic.go#L36-L50: replace the method with theAliaspattern and an empty-length guard.models/group-member.go#L30-L40: replace the method with theAliaspattern and an empty-length guard.models/device-owner.go#L30-L40: replace the method with theAliaspattern and an empty-length guard.🛠️ Proposed fixes
models/app-fic.go
-func (s *AppFIC) MarshalJSON() ([]byte, error) { - output := make(map[string]any) - output["appId"] = strings.ToUpper(s.AppId) - - if s.FIC == nil { - return nil, nil - } - - if fic, err := OmitEmptyUpper(s.FIC, "id"); err != nil { - return nil, err - } else { - output["fic"] = fic - return json.Marshal(output) - } -} +func (s *AppFIC) MarshalJSON() ([]byte, error) { + type Alias AppFIC + a := Alias(*s) + a.AppId = strings.ToUpper(a.AppId) + + if len(a.FIC) > 0 { + fic, err := OmitEmptyUpper(a.FIC, "id") + if err != nil { + return nil, err + } + a.FIC = fic + } else { + a.FIC = nil + } + return json.Marshal(a) +}models/group-member.go
-func (s GroupMember) MarshalJSON() ([]byte, error) { - output := make(map[string]any) - output["groupId"] = strings.ToUpper(s.GroupId) - - if member, err := OmitEmptyUpper(s.Member, "id"); err != nil { - return nil, err - } else { - output["member"] = member - return json.Marshal(output) - } -} +func (s GroupMember) MarshalJSON() ([]byte, error) { + type Alias GroupMember + a := Alias(s) + a.GroupId = strings.ToUpper(a.GroupId) + + if len(a.Member) > 0 { + member, err := OmitEmptyUpper(a.Member, "id") + if err != nil { + return nil, err + } + a.Member = member + } else { + a.Member = nil + } + return json.Marshal(a) +}models/device-owner.go
-func (s DeviceOwner) MarshalJSON() ([]byte, error) { - output := make(map[string]any) - output["deviceId"] = strings.ToUpper(s.DeviceId) - - if owner, err := OmitEmptyUpper(s.Owner, "id"); err != nil { - return nil, err - } else { - output["owner"] = owner - return json.Marshal(output) - } -} +func (s DeviceOwner) MarshalJSON() ([]byte, error) { + type Alias DeviceOwner + a := Alias(s) + a.DeviceId = strings.ToUpper(a.DeviceId) + + if len(a.Owner) > 0 { + owner, err := OmitEmptyUpper(a.Owner, "id") + if err != nil { + return nil, err + } + a.Owner = owner + } else { + a.Owner = nil + } + return json.Marshal(a) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@models/app-fic.go` around lines 36 - 50, Update MarshalJSON in models/app-fic.go:36-50, models/group-member.go:30-40, and models/device-owner.go:30-40 to use the established Alias pattern, checking each raw JSON field (FIC, Member, or Owner) for zero length before calling OmitEmptyUpper. Preserve normal serialization for populated fields and return valid JSON for empty or nil fields instead of returning nil bytes or unmarshalling empty data.
🧹 Nitpick comments (1)
models/utils.go (1)
66-79: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueAvoid
elseafterreturnand preventfloat64precision loss.The
elseblock is unnecessary since theifblock returns early. Additionally,json.Unmarshalintoanyparses all numbers asfloat64, which can silently truncate precision for large integers. Consider usingjson.NewDecoderwithUseNumber()to prevent potential data loss.🛠️ Proposed refactor
- if err := json.Unmarshal(raw, &data); err != nil { - return nil, err - } else { - StripEmptyEntries(data) - for _, key := range keys { - if value, ok := data[key].(string); ok { - data[key] = strings.ToUpper(value) - } - } - return json.Marshal(data) - } + dec := json.NewDecoder(bytes.NewReader(raw)) + dec.UseNumber() + if err := dec.Decode(&data); err != nil { + return nil, err + } + + StripEmptyEntries(data) + for _, key := range keys { + if value, ok := data[key].(string); ok { + data[key] = strings.ToUpper(value) + } + } + return json.Marshal(data)(Note: You will need to import
"bytes"if you apply this fix.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@models/utils.go` around lines 66 - 79, Update OmitEmptyUpper to remove the unnecessary else after the early unmarshal error return, and decode through json.Decoder configured with UseNumber so numeric values retain their original precision instead of becoming float64. Preserve the existing StripEmptyEntries, uppercase-key processing, and JSON marshaling behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@models/app-role-assignments.go`:
- Around line 46-52: Update the JSON patching logic around the output map to use
map[string]json.RawMessage instead of map[string]any, preserving embedded
numeric values without float64 conversion. Continue replacing the principalId
entry with the uppercase s.PrincipalId.String() value, encoding it as the
appropriate raw JSON value before returning.
In `@models/automation-account.go`:
- Around line 34-43: Update AutomationAccount.MarshalJSON to uppercase-normalize
the SubscriptionId field alongside Id, ResourceGroupId, and TenantId before
marshaling, matching the normalization used by ResourceGroup.
In `@models/container-registry.go`:
- Around line 35-43: Update MarshalJSON in models/container-registry.go (lines
35-43), models/function-app.go (lines 35-43), models/logic-app.go (lines 35-43),
models/vm-scale-set.go (lines 34-42), and models/web-app.go (lines 35-43) to
uppercase the alias’s SubscriptionId before marshaling, alongside the existing
identifier normalization. Review similar core wrappers such as AutomationAccount
for the same omission and apply the normalization there if applicable.
In `@models/key-vault-access-policy.go`:
- Around line 32-38: Update KeyVaultAccessPolicy.MarshalJSON to also uppercase
the embedded AccessPolicyEntry identifiers ApplicationId and TenantId before
marshaling, alongside the existing ObjectId and KeyVaultId normalization.
In `@models/key-vault.go`:
- Around line 34-41: Update the MarshalJSON methods for KeyVault in
models/key-vault.go, AutomationAccount in models/automation-account.go, and
ManagedCluster in models/managed-cluster.go to uppercase SubscriptionId
alongside the existing identifier fields before JSON marshaling.
In `@models/role-eligibility-schedule-instance.go`:
- Around line 34-41: Update RoleEligibilityScheduleInstance.MarshalJSON to
uppercase the Id and DirectoryScopeId fields alongside RoleDefinitionId,
PrincipalId, and TenantId before marshaling. Preserve the existing alias-based
serialization flow.
In `@models/virtual-machine.go`:
- Around line 34-41: Complete identifier normalization in
VirtualMachine.MarshalJSON by uppercasing SubscriptionId and update its
serialization test to assert “SUB-1”. In
RoleManagementPolicyAssignment.MarshalJSON, uppercase Id and add serialization
coverage verifying the normalized value; apply these changes in
models/virtual-machine.go lines 34-41 and
models/role-management-policy-assignment.go lines 42-50.
---
Outside diff comments:
In `@models/app-fic.go`:
- Around line 36-50: Update MarshalJSON in models/app-fic.go:36-50,
models/group-member.go:30-40, and models/device-owner.go:30-40 to use the
established Alias pattern, checking each raw JSON field (FIC, Member, or Owner)
for zero length before calling OmitEmptyUpper. Preserve normal serialization for
populated fields and return valid JSON for empty or nil fields instead of
returning nil bytes or unmarshalling empty data.
---
Nitpick comments:
In `@models/utils.go`:
- Around line 66-79: Update OmitEmptyUpper to remove the unnecessary else after
the early unmarshal error return, and decode through json.Decoder configured
with UseNumber so numeric values retain their original precision instead of
becoming float64. Preserve the existing StripEmptyEntries, uppercase-key
processing, and JSON marshaling behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1171a94a-4312-43b8-ad04-770de45339ba
📒 Files selected for processing (60)
models/app-fic.gomodels/app-member.gomodels/app-owner.gomodels/app-role-assignments.gomodels/app.gomodels/automation-account.gomodels/azure-role-assignment.gomodels/azure/descendant-info.gomodels/container-registry.gomodels/device-owner.gomodels/device.gomodels/function-app.gomodels/group-member.gomodels/group-owner.gomodels/group.gomodels/key-vault-access-policy.gomodels/key-vault-contributor.gomodels/key-vault-kvcontributor.gomodels/key-vault-owner.gomodels/key-vault-role-assignment.gomodels/key-vault-user-access-admin.gomodels/key-vault.gomodels/logic-app.gomodels/managed-cluster.gomodels/management-group-contributor.gomodels/management-group-owner.gomodels/management-group-role-assignment.gomodels/management-group-user-access-admin.gomodels/marshal_test.gomodels/mgmt-group.gomodels/resource-group-contributor.gomodels/resource-group-owner.gomodels/resource-group-role-assignment.gomodels/resource-group-user-access-admin.gomodels/resource-group.gomodels/role-assignments.gomodels/role-eligibility-schedule-instance.gomodels/role-management-policy-assignment.gomodels/role.gomodels/service-principal-owner.gomodels/service-principal.gomodels/subscription-contributor.gomodels/subscription-owner.gomodels/subscription-role-assignment.gomodels/subscription-user-access-admin.gomodels/subscription.gomodels/tenant.gomodels/user.gomodels/utils.gomodels/utils_test.gomodels/virtual-machine-admin-login.gomodels/virtual-machine-avere-contributor.gomodels/virtual-machine-contributor.gomodels/virtual-machine-owner.gomodels/virtual-machine-role-assignment.gomodels/virtual-machine-user-access-admin.gomodels/virtual-machine-vmcontributor.gomodels/virtual-machine.gomodels/vm-scale-set.gomodels/web-app.go
StranDutton
approved these changes
Jul 17, 2026
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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
AzureHound now emits uppercased identifier fields (object IDs, tenant IDs, principal IDs, ARM resource scopes, etc.) directly in its JSON output. Previously BloodHound Enterprise normalized these identifiers to uppercase internally (toUpper) during ingest. This change moves that normalization to the collector so BHE can trust the collector's casing and eventually retire its internal string-normalization pass (gated behind the use_raw_object_id flag on the BHE side).
The uppercasing is implemented at the JSON-serialization boundary via MarshalJSON on the wrapper models, so the collector's in-memory data and API interactions are unchanged — only the emitted output is normalized. All conversions are non-mutating (they operate on copies/aliases).
Motivation and Context
BHE historically uppercased Azure identifiers during ingest to make graph node lookups case-insensitive. Doing this at collection time instead:
Resolves: BED-8944
What changed
models/utils.go— shared, non-mutating helpers:models/*.gowrapper types gained (or had extended) MarshalJSON methods that uppercase their identifier fields (object IDs, tenant IDs, resource IDs, scopes, on-prem SIDs, role-assignment endpoints, FIC IDs, etc.). UUID-typed fields (e.g. PrincipalId uuid.UUID) are uppercased via a map[string]any override since the typed value can't hold an uppercased string.models/azure/descendant-info.go— management-group descendant IDs uppercased.Explicitly not uppercased (deliberate)
Validation
The output was validated end-to-end against BHE by comparing three independently-produced graphs of the same tenant:
Case-insensitive per-node and per-edge diffs across all three pairs were identical (0 additions, 0 removals), with matching per-node-kind and all 47 per-rel-kind counts, and 0 case-split groups / 0 orphan stub nodes. The uppercased-collector path produces a graph equivalent (modulo uniform casing) to the current production behavior.
Compatibility / rollout
Testing
go test ./models/...
All MarshalJSON tests pass, including assertions that display/constant fields are preserved and that conversions are non-mutating.
Summary by CodeRabbit
nullvs omitted/normalized), while preserving intended display-oriented fields and non-mutating source values where required.