Solid security fix — the tenant-binding gap for global-app anonymous JWTs is real and the approach is correct. One concern about backwards compatibility with already-issued JWTs that have null tid/pid.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

PR Review Summary

(1) Total Issues | Risk: Medium

🟠⚠️ Major (1) 🟠⚠️

🟠 1) system Missing test coverage for security-critical anonymous JWT paths

Issue: The PR adds security-critical logic for binding tenant/project into anonymous session JWTs for global apps, but the existing test suite does not cover these new code paths. Specifically:

1. auth.ts:229-234 — Global app anonymous sessions embedding tid/pid from request headers is untested
2. auth.ts:229-234 — Rolling refresh preserving original tid/pid (preventing rebinding via different headers) is untested
3. runAuth.ts:797-798, 832-833 — Middleware extracting tid/pid from anonymous JWT claims (instead of headers) for global apps is untested

The existing tests at auth.test.ts:103-106 and auth.test.ts:361-362 only verify tenant-scoped apps (where appRecord.tenantId is non-null). The runAuth-appCredentialAuth.test.ts:567-673 tests global apps but only the authenticated RS256 JWT path, not the anonymous HS256 JWT path that this PR modifies.

Why: Without tests for these paths:

• A regression could silently break tenant isolation — anonymous JWTs would be issued with null tid/pid, and the middleware would fail to resolve tenant context
• The security guarantee this PR claims (callers cannot rebind tenant on refresh) is not verified
• Future refactors have no safety net for this critical auth flow

Fix: Add the following tests to auth.test.ts:

// 1. Test global app embeds tid/pid from headers
it('should embed tid/pid from request headers for global apps', async () => {
  // Create app with tenantId: null, projectId: null
  // Request session with x-inkeep-tenant-id and x-inkeep-project-id headers
  // Verify JWT contains those values (not null)
});

// 2. Test rolling refresh preserves original values
it('should preserve original tid/pid during refresh even with different headers', async () => {
  // Create session for global app with headers tenant-A/project-A
  // Refresh with Authorization header AND different headers tenant-B/project-B
  // Verify new JWT still has tenant-A/project-A
});

And to runAuth-appCredentialAuth.test.ts:

// 3. Test middleware uses JWT claims not headers
it('should use tid/pid from anonymous JWT claims for global apps', async () => {
  // Create global app
  // Mock anonymous JWT with tid/pid claims
  // Send request with DIFFERENT x-inkeep-tenant-id headers
  // Verify executionContext uses JWT claim values, not header values
});

Refs:

• auth.test.ts — existing tenant-scoped test
• runAuth-appCredentialAuth.test.ts — global app authenticated path

🕐 Pending Recommendations (2)

• 🟠 runAuth.ts:832-833 Pre-existing anonymous JWTs with null tid/pid will resolve to empty string instead of falling back to headers — confirm this is intentional and downstream code handles tenantId: ''
• 🟡 auth.ts:229-234 Token refresh preserves null tid/pid from pre-fix tokens — consider forcing re-binding on refresh or documenting that pre-fix token holders must clear session storage

Summary: The security fix is sound and the implementation is correct — this properly addresses the tenant-binding gap for global app anonymous JWTs. The main recommendation is to add test coverage for the security-critical code paths before merging. Once tests are added, this provides a solid security improvement. The backward compatibility concern (pre-fix JWTs) was already raised by a prior reviewer and is a reasonable tradeoff for a security fix.

PR Review Summary

(2) Total Issues | Risk: Medium

🟠⚠️ Major (0) 🟠⚠️

No new Major issues identified.

🟡 Minor (0) 🟡

No new Minor issues identified.

🕐 Pending Recommendations (2)

• 🟠 auth.ts:229-234 Missing test coverage for security-critical anonymous JWT paths — global app sessions embedding tid/pid from headers, rolling refresh preserving original values, and middleware using JWT claims instead of headers
• 🟡 runAuth.ts:832-833 Pre-existing anonymous JWTs with null tid/pid will resolve to empty string instead of header fallback — confirm this is intentional for the security fix

Summary: The security fix is well-implemented — tenant/project binding is now properly locked into anonymous JWTs at creation time, and refresh correctly preserves the original binding. The domain reviewers (IAM security, standards, tests) confirmed the implementation is sound with no new issues. The two pending recommendations from prior reviews remain relevant: (1) adding test coverage for the global app anonymous JWT paths would strengthen confidence in this security-critical code, and (2) the backward compatibility behavior for pre-existing JWTs should be explicitly acknowledged as intentional.