fix: properly check write access for fine-grained PATs#4
Merged
Conversation
Update remaining references to the old jwm4 repo: - review_roadmap/main.py: footer link in generated roadmaps - tests/test_main.py: test assertion for footer URL - ADR/phase4_plan.md: pip install example URL
The write access check was incorrectly passing for tokens that lacked write permissions because it only checked the repository permissions object, which reflects user access rather than token-specific access. Changes: - Add WriteAccessStatus enum and WriteAccessResult model for detailed access check results (GRANTED, DENIED, UNCERTAIN) - For classic PATs: verify OAuth scopes via X-OAuth-Scopes header - For fine-grained PATs: perform live write test using reactions (create and immediately delete an 'eyes' reaction on the PR) - Improve error messages with troubleshooting guidance - Add comprehensive GitHub token setup documentation to README The live write test allows definitive verification for fine-grained PATs, which cannot be checked via headers since they don't include scope info and the permissions object reflects user access, not token configuration.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
--postflag's write access check was incorrectly passing for tokens that lacked write permissions. This happened because the check only looked at the repositorypermissionsobject, which reflects the user's access level rather than the token's specific permissions.For example, if a user has push access to a repository but their fine-grained PAT is only configured for a different repo, the check would pass but the actual post would fail with 403.
Solution
1. Structured Access Check Results
Added
WriteAccessStatusenum (GRANTED,DENIED,UNCERTAIN) andWriteAccessResultmodel to provide detailed feedback about the access check.2. Classic PAT Verification
For classic PATs and OAuth tokens, verify OAuth scopes via the
X-OAuth-Scopesresponse header:reposcoperepoorpublic_reposcope3. Fine-Grained PAT Live Test
For fine-grained PATs (which don't include scope info in headers), perform a live write test:
4. Improved Error Messages
Better error messages with troubleshooting guidance when access is denied.
5. Documentation
Added comprehensive "GitHub Token Setup" section to README explaining:
Testing
All 58 tests pass with 84% coverage. Added tests for:
Files Changed
review_roadmap/models.py- Added WriteAccessStatus and WriteAccessResultreview_roadmap/github/client.py- Updated check_write_access with scope checking and live testreview_roadmap/main.py- Handle new result type, improved error messagestests/test_github_client.py- Comprehensive tests for all scenariostests/test_main.py- Updated mocks for new return typeREADME.md- Added GitHub token setup documentation