MemberJunction / MemberJunction/MJ

MJServer auth fail-open: CheckAPIKeyScopeAndLog ignores app IsActive; REST include/exclude lists are case-sensitive

Open
#3,788 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.