MemberJunction / MemberJunction/MJ
MJServer auth fail-open: CheckAPIKeyScopeAndLog ignores app IsActive; REST include/exclude lists are case-sensitive
- Dominant language
- TSQL
- Stars
- 29
- Forks
- 6
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 323
Description
## Summary
Two independent authorization gaps in the MJServer API-key / REST layer that both **fail open** — access is granted where it should be denied. Surfaced during the review of #3542 and pinned in tests as `SECURITY DIVERGENCE` / `SECURITY GAP` (asserting current behavior so a fix flips them), not fixed there.
## 1. `CheckAPIKeyScopeAndLog` skips the application `IsActive` ceiling
- `CheckAPIKeyScope` rejects a request whose target application is deactivated — `packages/MJServer/src/auth/APIKeyScopeAuth.ts:138` (`if (!app.IsActive) { ... }`).
- `CheckAPIKeyScopeAndLog` (`:191`) — the logging variant used on the actual request path — does **not** carry that branch, so a deactivated application is still authorized.
**Impact:** deactivating an application does not revoke API-key access through the logged path. **Test:** `packages/MJServer/src/__tests__/APIKeyScopeAuth.test.ts` (the `IsActive: false` cases pinned as `SECURITY DIVERGENCE`).
**Fix:** hoist the `IsActive` check into the shared path so both entry points enforce it (or have `CheckAPIKeyScopeAndLog` delegate to `CheckAPIKeyScope` for the decision and only add logging).
## 2. REST include/exclude exact-match lists are case-sensitive against a lowercased name
- `RESTEndpointHandler` lowercases the entity name — `packages/MJServer/src/rest/RESTEndpointHandler.ts:76` (`const name = entityName.toLowerCase()`).
- But the exact-match allow/deny lists compare with a raw `.includes(name)`:
- `:97` `this.options.excludeEntities.includes(name)`
- `:122` `this.options.includeEntities.includes(name)`
So a mixed-case entry (e.g. `excludeEntities: ['Users']`) never matches the lowercased `name` (`users`) — an **exclusion silently fails to exclude** (fail-open), and symmetrically an inclusion silently fails to include. The wildcard branch (`:104`) already lowercases the pattern; the exact-match branch does not.
**Impact:** an operator who lists an entity to exclude from the REST surface with any non-lowercase spelling gets it exposed anyway. **Test:** `RESTEndpointHandler.test.ts:215,237` (`SECURITY GAP: a mixed-case exact exclusion silently fails to exclude`).
**Fix:** lowercase the list entries at comparison time (`excludeEntities.some(e => e.toLowerCase() === name)`) or normalize the lists once at config load.
## References
- PR #3542 (review finding #2)
Contributor guide
Research direction
Start with packages/MJServer/src/auth/APIKeyScopeAuth.ts and compare CheckAPIKeyScope with CheckAPIKeyScopeAndLog, then read the IsActive cases in packages/MJServer/src/__tests__/APIKeyScopeAuth.test.ts. Next inspect the exact-match checks in packages/MJServer/src/rest/RESTEndpointHandler.ts and the cases at RESTEndpointHandler.test.ts:215,237. Done means deactivated applications are denied and mixed-case include/exclude entries behave case-insensitively.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- api, authentication, security
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100