Skip to content

Conversation

@tudddorrr
Copy link
Collaborator

No description provided.

@tudddorrr tudddorrr added the documentation Improvements or additions to documentation label Dec 6, 2025
@claude
Copy link

claude bot commented Dec 6, 2025

PR Review: Enrich docs with scopes

Overall, this PR successfully adds scope documentation to API endpoints, making it clearer which permissions are required for each operation. The implementation is clean and the documented scopes match the policy implementations.


🟡 Security & Code Quality

Pre-existing policy bug revealed by this PR

Location: src/policies/api/player-api.policy.ts:8-10 vs src/docs/player-api.docs.ts:7

The documentation correctly shows that identify requires both READ_PLAYERS and WRITE_PLAYERS:

scopes: [APIKeyScope.READ_PLAYERS, APIKeyScope.WRITE_PLAYERS]

However, the policy only checks for READ_PLAYERS:

async identify(): Promise<PolicyResponse> {
  return this.hasScope(APIKeyScope.READ_PLAYERS)
}

This is a pre-existing security issue (not introduced by this PR). The identify endpoint can create new players (createPlayerFromIdentifyRequest in player-api.service.ts:16-19), which is a write operation. The policy should be:

async identify(): Promise<PolicyResponse> {
  return this.hasScopes([APIKeyScope.READ_PLAYERS, APIKeyScope.WRITE_PLAYERS])
}

Recommendation: File a separate security issue to fix the policy check. The documentation in this PR is correct.


✅ Test Coverage

No test updates needed - this is purely documentation enrichment.


✅ Performance Considerations

No issues found. The scope enrichment happens at documentation generation time with minimal overhead.


✅ Best Practices

No issues found. The PR follows the codebase conventions consistently.

@tudddorrr tudddorrr enabled auto-merge December 6, 2025 15:16
@tudddorrr tudddorrr merged commit aae7ff6 into develop Dec 6, 2025
6 of 7 checks passed
@tudddorrr tudddorrr deleted the scopes-in-docs branch December 6, 2025 15:35
@codecov
Copy link

codecov bot commented Dec 6, 2025

Codecov Report

❌ Patch coverage is 99.09091% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 99.20%. Comparing base (a7f4785) to head (88e3c6b).
⚠️ Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
src/services/public/documentation.service.ts 96.15% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #730      +/-   ##
===========================================
- Coverage    99.20%   99.20%   -0.01%     
===========================================
  Files          224      225       +1     
  Lines        16511    16596      +85     
  Branches      1505     1517      +12     
===========================================
+ Hits         16380    16464      +84     
- Misses         129      130       +1     
  Partials         2        2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants