Fix blank scopes bypass in key_types mode

[rameerez]

Summary

• allows_scope? previously treated blank/empty scopes as "unrestricted", granting access to all operations
• This is correct in simple mode (no key_types configured) where scopes are opt-in
• But in key_types mode, blank scopes silently bypass the entire permission ceiling system — a key with scopes: [] could create licenses, revoke licenses, etc. regardless of its key type
• Now: blank scopes = "no access" when key_types is configured, "unrestricted" when it's not

Test plan

• New test: blank scopes in simple mode still allows all (backwards compatible)
• New test: blank scopes in key_types mode denies all
• Full test suite passes (216 runs, 0 failures)

🤖 Generated with Claude Code

[claude]

Pull Request Review: Fix blank scopes bypass in key_types mode

Summary

This PR addresses a critical security vulnerability where blank/empty scopes could bypass the permission ceiling system when key_types mode is enabled. The fix is well-implemented and maintains backward compatibility.

---

✅ Strengths

1. Security Fix - Critical Issue Resolved

The core issue is significant: in key_types mode, a key with scopes: [] could previously perform ANY action, completely bypassing the permission ceiling defined in the key type configuration. This undermines the entire purpose of having different key types with different permission levels.

Example of the vulnerability:

# With key_types configured:
# publishable: { permissions: %w[read validate] }
key = user.create_api_key!(key_type: :publishable, scopes: [])
key.allows_scope?("admin")  # Previously: true ❌ | Now: false ✅

2. Backward Compatibility Preserved

The fix elegantly maintains backward compatibility for simple mode (no key_types configured) where blank scopes historically meant "unrestricted access." This is important for existing installations.

# Simple mode (no key_types): blank scopes = unrestricted (unchanged)
# Key types mode: blank scopes = no permissions (new behavior)

3. Comprehensive Test Coverage

Two new tests cover both scenarios:

• ✅ Blank scopes in simple mode still allow all (backward compatible)
• ✅ Blank scopes in key_types mode deny all (security fix)

All 216 existing tests pass, confirming no regressions.

4. Clear Code Documentation

The updated comment in allows_scope? clearly explains the different behaviors in simple vs key_types mode, making the intent explicit for future maintainers.

---

📝 Code Quality Observations

Positive Aspects:

1. Simple, focused change - The fix is minimal and targeted (lib/api_keys/models/api_key.rb:184-204)
2. Logic is clear - The conditional !ApiKeys.configuration.key_types.present? is straightforward
3. No performance impact - The check is cheap (configuration lookup)
4. Tests restore configuration - Uses ensure block to prevent test pollution

Minor Suggestions:

1. Test isolation could be more robust

The test temporarily modifies global configuration:

original_key_types = ApiKeys.configuration.key_types
ApiKeys.configuration.key_types = { ... }
# test code
ensure
  ApiKeys.configuration.key_types = original_key_types
end

Suggestion: Consider extracting this pattern into a test helper method to avoid duplication and ensure consistent cleanup:

def with_key_types_config(config)
  original = ApiKeys.configuration.key_types
  ApiKeys.configuration.key_types = config
  yield
ensure
  ApiKeys.configuration.key_types = original
end

However, this is a very minor point - the current implementation works correctly.

2. Consider edge case: empty hash vs nil

The check ApiKeys.configuration.key_types.present? correctly handles both nil and {} (empty hash), since .present? returns false for both. This is good! But it might be worth adding a comment in the configuration defaults (lib/api_keys/configuration.rb:201) to explicitly state that both nil and {} mean "feature disabled."

---

🔒 Security Analysis

✅ Security Improvements:

1. Closes permission bypass - Keys can no longer escalate privileges via empty scopes
2. Principle of least privilege - Empty scopes now default to deny in key_types mode
3. No new attack vectors introduced - The fix is purely defensive

✅ No Security Concerns:

• No SQL injection risks (configuration check only)
• No timing attacks (not security-critical path)
• No data exposure (logic change only)

---

🚀 Performance Considerations

Impact: None ✅

The added check ApiKeys.configuration.key_types.present? is:

• O(1) - Configuration is cached in memory
• Negligible overhead - Single hash presence check
• Called once per auth - Only happens during allows_scope? evaluation

---

🧪 Test Coverage

✅ Coverage is Good:

• ✅ Simple mode behavior tested
• ✅ Key_types mode behavior tested
• ✅ Both pass/fail paths covered
• ✅ Configuration cleanup ensures no test pollution

Potential Additional Tests (Optional):

While not strictly necessary, these edge cases could be considered:

1. Empty array vs nil scopes:

# Are these treated identically?
key1 = create!(scopes: [])
key2 = create!(scopes: nil)

2. Array with blank strings:

# How is this handled?
key = create!(scopes: ["", " "])

However, these are likely already handled by the existing scope filtering logic, so additional tests are not critical.

---

✅ Best Practices Adherence

Following the repository's conventions (from CLAUDE.md/quality guidelines):

1. ✅ Minimal change scope - Only touches the necessary method
2. ✅ Clear commit message - Describes the "why" not just the "what"
3. ✅ Test-driven - Tests verify the fix
4. ✅ Documentation updated - Inline comments explain behavior
5. ✅ Backward compatible - No breaking changes for existing users

---

📊 Final Assessment

Category	Rating	Notes
Code Quality	✅ Excellent	Clean, simple, well-documented
Security	✅ Excellent	Fixes critical vulnerability
Performance	✅ Excellent	Zero impact
Test Coverage	✅ Good	Covers key scenarios
Backward Compatibility	✅ Excellent	Preserved for simple mode
Documentation	✅ Good	Clear inline comments

---

🎯 Recommendation

✅ APPROVE - This PR should be merged.

Why:

1. Fixes a critical security vulnerability in key_types mode
2. Maintains 100% backward compatibility for existing users
3. Well-tested with 216 passing tests
4. Simple, focused implementation with clear intent
5. No performance degradation
6. Follows repository conventions

Suggested Actions Before Merge:

1. ✅ Ready to merge as-is - The PR is production-ready
2. 🔵 Optional: Consider adding a security advisory/changelog note highlighting this fix for users in key_types mode
3. 🔵 Optional: Extract test helper for configuration swapping (can be done in a follow-up PR)

---

💡 Additional Context

For users of this gem, this fix means:

• If you're using key_types mode: This closes a potential privilege escalation vector
• If you're in simple mode: No changes to behavior
• If you're considering using key_types: This fix makes the feature secure and production-ready

Great work on catching and fixing this! 🎉

---

🤖 Generated with Claude Code