fix(api): prevent update_news auth bypass from broad rescue - #3729
fix(api): prevent update_news auth bypass from broad rescue#3729jmthomas wants to merge 2 commits into
Conversation
The method-level `rescue Exception` swallowed the AuthError raised by authorize, so unauthenticated JSON-RPC callers got 200 OK and an anonymous write to the openc3_news Redis key. Authorize now runs outside the rescued region and the rescue is narrowed to StandardError. Also disables the User Menu Refresh button for non-admins, which previously wiped the feed silently on Enterprise, and adds the missing Extract/Authorization requires to three specs that could not load standalone. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes an authorization bypass in the Settings API’s update_news endpoint where a broad rescue Exception previously swallowed AuthError, allowing unauthenticated callers to receive 200 OK and trigger a write to the news feed key. It also aligns the frontend UI with the backend permission requirement and stabilizes specs by adding missing requires.
Changes:
- Moved
authorize(...)outside the rescued region inupdate_newsand narrowed the rescue toStandardErrorso auth failures propagate correctly. - Disabled the “Refresh” button for the news feed in the User Menu when the user is not an admin.
- Added missing
Extract/Authorizationrequires and introduced targeted specs forupdate_newsauth and connectivity failures.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| openc3/spec/microservices/decom_microservice_spec.rb | Adds missing Extract require to allow the spec to load standalone. |
| openc3/spec/api/stash_api_spec.rb | Adds missing Extract / Authorization requires for standalone spec loading. |
| openc3/spec/api/settings_api_spec.rb | Adds missing requires and new coverage asserting update_news auth failure does not write the feed. |
| openc3/lib/openc3/api/settings_api.rb | Fixes the auth-bypass by moving authorization outside the rescue and narrowing the rescued exception type. |
| openc3-cosmos-init/plugins/packages/openc3-vue-common/src/tools/base/UserMenu.vue | Disables the news refresh button for non-admins and adds a test selector attribute. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3729 +/- ##
==========================================
+ Coverage 79.58% 80.06% +0.47%
==========================================
Files 885 885
Lines 65382 65383 +1
Branches 2543 2592 +49
==========================================
+ Hits 52035 52347 +312
+ Misses 12674 12374 -300
+ Partials 673 662 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
clayandgen
left a comment
clayandgen
left a comment
There was a problem hiding this comment.
The periodic microservice might have this same issue? Let me know what you think:
|
Sort of. This is an internal microservice with no permissions checks. However it did have the broad |
The method-level
rescue Exceptionswallowed the AuthError raised by authorize, so unauthenticated JSON-RPC callers got 200 OK and an anonymous write to the openc3_news Redis key. Authorize now runs outside the rescued region and the rescue is narrowed to StandardError.Also disables the User Menu Refresh button for non-admins, which previously wiped the feed on Enterprise, and adds the missing Extract/Authorization requires to three specs that could not load standalone.
🤖 Generated with Claude Code