Basekick-Labs / Basekick-Labs/arc
security(rbac): table-reference dedup key case-folds while the permission check does not, so one grant authorizes reads of two measurements
- Dominant language
- Go
- Stars
- 677
- Forks
- 53
- Avg merge
- 9h 14m
- Merged PRs (30d)
- 164
Description
## Summary
The RBAC table-reference dedup key is lower-cased, but the permission check that consumes those references compares **case-sensitively**. A query naming the same measurement in two different cases is deduped to one reference, so one grant is checked while two measurements are read.
Split out of #750 (item 2) so it can be milestoned and closed on its own.
## The mismatch
The dedup key folds case — `internal/api/query.go:1225` and `:1274`:
```go
tableName := resolve(sql[matchIdx[2]:matchIdx[3]])
table := strings.ToLower(tableName)
```
The `TableReference` that reaches `CheckPermissionsBatch` keeps the **original** case, and `matchPattern` (`internal/auth/rbac_manager.go:1967`) compares case-sensitively:
```go
func matchPattern(pattern, value string) bool {
// Exact wildcard
if pattern == "*" {
return true
}
// Prefix wildcard (e.g., "prod_*" matches "prod_us", "prod_eu")
if strings.HasSuffix(pattern, "_*") {
...
```
## Repro shape
```sql
SELECT * FROM cpu WHERE x IN (SELECT y FROM CPU)
```
`cpu` and `CPU` fold to one dedup key, so the permission for one is checked, and both are read.
This matters because **object keys are case-sensitive on S3**: `cpu` and `CPU` are two distinct measurements with two separate grants, not two spellings of one. A principal granted `cpu` reads `CPU` without a grant for it.
## Why it is security-relevant
The authorization decision is made on a set that is smaller than the set actually read. Any measurement whose name differs only by case from one the principal *is* granted becomes readable.
Distinct from #740, which covers spoke identifiers rather than table references.
## Suggested fix
Make the two sides agree. Either:
1. **Preserve case in the dedup key** — dedup on the original `tableName` so distinct spellings stay distinct references and each is checked. Safest: it cannot under-check, only over-check.
2. Fold case in `matchPattern` too — but that changes grant semantics against case-sensitive object storage, so it is the wrong direction.
Option 1 is preferred: the check set should never be smaller than the read set.
## Test plan
- [ ] Test asserting `SELECT * FROM cpu WHERE x IN (SELECT y FROM CPU)` checks permissions for **both** `cpu` and `CPU`
- [ ] Test asserting a principal granted only `cpu` is denied that query
- [ ] Same for the JOIN path (`query.go:1274`)
- [ ] Regression test must FAIL pre-fix (revert-run-restore)
Split from #750.
Contributor guide
Research direction
Start in internal/api/query.go at lines 1225 and 1274, then read TableReference handling through CheckPermissionsBatch and matchPattern in internal/auth/rbac_manager.go:1967. Add regression coverage for mixed-case subquery and JOIN references, including denial when only cpu is granted. Done means both cpu and CPU are checked independently and the pre-fix behavior fails the regression tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- databases, security
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100