Skip to content

[CSM-1857] Stabilize Azure detector hash_v2 with deterministic iteration#4846

Draft
dipto-truffle wants to merge 1 commit intomainfrom
test/csm-1857-hashv2-instability
Draft

[CSM-1857] Stabilize Azure detector hash_v2 with deterministic iteration#4846
dipto-truffle wants to merge 1 commit intomainfrom
test/csm-1857-hashv2-instability

Conversation

@dipto-truffle
Copy link
Copy Markdown

@dipto-truffle dipto-truffle commented Mar 27, 2026

Summary

  • Root cause: ProcessData in the Azure Entra v2 detector iterated Go maps (clientSecrets, clientIds, tenantIds) non-deterministically, causing the same credential to produce different (clientId, tenantId) pairings across scanner runs. This yielded different RawV2 values, different hash_v2 hashes, and duplicate secret rows in the database (the secondary issue in CSM-1857).
  • Fix: Iterate map keys in sorted order via slices.Sorted(maps.Keys(...)) and clone caller maps with maps.Clone before verification-driven mutations. Same inputs now always produce the same RawV2/hash_v2.
  • Tests: Added unit tests for RawV2 stability (ID count sensitivity, determinism over 50 runs, caller map immutability, hash divergence chain).

Corresponds to trufflesecurity/thog#5936.

Test plan

  • go test ./pkg/detectors/azure_entra/serviceprincipal/v2/ -v — all 8 tests pass
  • TestProcessData_DeterministicRawV2 confirms identical RawV2 across 50 repeated calls with the same inputs
  • TestProcessData_DoesNotMutateCallerMaps confirms caller maps are not modified
  • TestProcessData_RawV2DependsOnIDCount confirms RawV2 is populated only with unambiguous IDs
  • TestProcessData_SameSecretDifferentRawV2 documents the RawV2 divergence when chunk context differs
  • go test ./pkg/detectors/azure_entra/... — all pass

Made with Cursor


Note

Medium Risk
Changes secret result generation order and verification-side pruning behavior in the Azure Entra v2 detector, which could affect which (tenantId, clientId) pairing is chosen and thus hash_v2/dedupe behavior. Logic is well-scoped and covered by new determinism and non-mutation tests.

Overview
Stabilizes Azure Entra Service Principal v2 detection output by making ProcessData iterate clientSecrets, clientIds, and tenantIds in sorted key order and by cloning the input maps before verification-driven deletions.

Adds unit tests to lock in deterministic RawV2 generation across repeated runs, document when RawV2 is omitted due to ambiguous IDs, and ensure caller-provided maps are not mutated.

Written by Cursor Bugbot for commit 3461a39. This will update automatically on new commits. Configure here.

ProcessData in the Azure Entra v2 detector iterated Go maps
non-deterministically, causing the same credential to produce different
(clientId, tenantId) pairings across scanner runs. This yielded
different RawV2 values, different hash_v2 hashes, and duplicate secret
rows in the database (CSM-1857 secondary issue).

Iterate map keys in sorted order via slices.Sorted(maps.Keys(...)) and
clone caller maps with maps.Clone before verification-driven mutations.
Same inputs now always produce the same RawV2/hash_v2.

Made-with: Cursor
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 27, 2026

CLA assistant check
All committers have signed the CLA.

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

origClientLen := len(clientIDs)
origTenantLen := len(tenantIDs)

_ = ProcessData(context.Background(), secrets, clientIDs, tenantIDs, false, nil)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caller-map-immutability test never exercises mutation code path

Low Severity

TestProcessData_DoesNotMutateCallerMaps passes verify=false, but every delete call that could mutate maps is inside the if verify block. The test always passes regardless of whether maps.Clone is used, providing false confidence that the cloning fix works. If the cloning were removed in a future refactor, this test would not catch the regression.

Additional Locations (1)
Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants