Benchmark: grafana PR 103633

[celmis-codereviewer]

Benchmark reproduction of grafana#103633

[celmis-codereviewer]

💬 COMMENT — findings to consider

Full findings and scope are in the review summary comment on this pull request — one persistent comment, updated in place on every run.

[celmis-codereviewer]

💬 COMMENT — findings to consider

Full findings and scope are in the review summary comment on this pull request — one persistent comment, updated in place on every run.

[celmis-codereviewer]

Why: userPermDenialCacheKey on line 31 uses the ".perm_" prefix identical to userPermCacheKey on line 27, causing key collisions and type mismatches in the shared cache when actions contain underscores.

🟠 Cache key collision between userPermDenialCacheKey and userPermCacheKey

userPermDenialCacheKey constructs keys starting with namespace + ".perm_" + userUID + "_" + action + .... This uses the same key prefix (.perm_) as userPermCacheKey (namespace + ".perm_" + userUID + "_" + action). When an action name contains underscores (e.g. read_dash1_fold1), userPermCacheKey generates the exact same string key as userPermDenialCacheKey for action read, name dash1, and parent fold1. Because both cache wrappers share the same underlying cache instance but store different value types (bool vs map[string]bool), this collision causes cache entry corruption and type assertion failures.

Suggested change

	return namespace + ".perm_" + userUID + "_" + action + "_" + name + "_" + parent
	func userPermDenialCacheKey(namespace, userUID, action, name, parent string) string {
	return namespace + ".perm_denial_" + userUID + "_" + action + "_" + name + "_" + parent
	}

_{agent: defect · rule: defect.type-mismatch · confidence: 0.95}

[celmis-codereviewer]

Why: On line 978, false is passed in the permCache map instead of true, causing checkPermission to evaluate to false and pass assert.False even if permDenialCache lookup on line 117 is removed or fails.

🟡 Test sets permission cache to false when attempting to test denial override

In TestService_CacheCheck/Should deny on explicit cache deny entry, the test sets map[string]bool{"dashboards:uid:dash1": false} in permCache. Because the cached permission value is false, checkPermission evaluates to false regardless of whether permDenialCache is checked. To properly verify that permDenialCache short-circuits the check when permission would otherwise be granted, permCache must be set to true for dash1.

Suggested change

	// Explicitly deny access to the dashboard
	// Allow access to the dashboard to prove this is not checked
	s.permCache.Set(ctx, userPermCacheKey("org-12", "test-uid", "dashboards:read"), map[string]bool{"dashboards:uid:dash1": true})

_{agent: defect · rule: defect.untested-branch · confidence: 0.95}

[celmis-codereviewer]

🤖 Code Review for PR #12

💬 COMMENT — findings to consider

Findings

• 🟠 Error: 1
• 🟡 Warning: 1

Scope

• Files changed: 4
• Lines: +240 / -33

Performance

• Analysis time: 164.5s · agents: structural, cve, contract, security, defect · tokens: 33,754/44,749

---

_{Powered by Code Analyzer · context: tree-sitter graph + structural, cve, contract, security, defect}