Fix command permissions and integration preferences - #84
Conversation
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe pull request adds Notion desktop OAuth support, improves GitHub callback failures, preserves omitted email preferences, adds safe cross-platform command matching, and renames OAuth credential variables. ChangesAPI proxy authentication and preferences
Command permission matching
OAuth configuration and error messaging
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DesktopApp
participant ApiProxyCF as api-proxy-cf
participant OAuthProvider as OAuthProvider
DesktopApp->>ApiProxyCF: Send OAuth callback with signed state and code
ApiProxyCF->>OAuthProvider: Exchange authorization code with redirect URI
OAuthProvider-->>ApiProxyCF: Return token or provider error
ApiProxyCF-->>DesktopApp: Redirect with authentication result or provider_error
Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api-proxy-cf/src/index.js`:
- Around line 3956-3972: Update the preference persistence logic around the
preference helper and email_prefs upsert so omitted fields are preserved from
the current database row at write time rather than a stale snapshot, preventing
concurrent partial updates from overwriting unrelated changes. Use a conditional
update/upsert that changes only validated fields while retaining existing values
for omitted fields, and add a test covering overlapping updates to notify_limit
and notify_announcements.
In `@api-proxy-cf/test/preferences.test.mjs`:
- Around line 42-59: Expand the test around the partial-update flow to use
table-driven cases for each preference field: notify_limit,
notify_announcements, and notify_scheduled. For every field, submit an invalid
value and verify the update does not alter that field or omitted preferences,
then submit a valid partial update and verify only that field changes while the
others retain their stored values.
- Line 3: Update the api-proxy-cf package engine declaration to require Node.js
>=22.13.0, aligning it with the DatabaseSync import in preferences.test.mjs; do
not replace the SQLite implementation.
In `@src/agent/commandPermissions.js`:
- Around line 27-39: Update command parsing in src/agent/commandPermissions.js,
specifically the escape and quote handling around the tokenizer, to reject & as
a shell-control character for cmd.exe commands even when preceded by a backslash
or enclosed in single quotes; preserve existing behavior for supported safe
inputs. Add regression cases in test/commandPermissions.test.mjs covering npm
test \& whoami and npm test '& whoami & ', asserting both are rejected before
auto-approval.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 25d62d2f-0edd-4219-b281-5bc21bfaf4f6
📒 Files selected for processing (5)
api-proxy-cf/src/index.jsapi-proxy-cf/test/desktop-integrations.test.mjsapi-proxy-cf/test/preferences.test.mjssrc/agent/commandPermissions.jstest/commandPermissions.test.mjs
e88cd1d to
24cd6fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/commandPermissions.test.js (1)
5-15: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a negative case for intermediate-argument mismatches.
The current PowerShell assertions cover a matching
-Fileargument and a mismatched executable. They do not verify that a different intermediate argument is rejected. The matcher insrc/agent/commandPermissions.jsLines 70-85 compares intermediate arguments exactly.Proposed test
assert.equal(commandMatchesPermissionRule('pwsh -File YDeploy.ps1', { executable: 'pwsh', argumentPrefix: '-File X', }), false); + assert.equal(commandMatchesPermissionRule('powershell -Command XDeploy.ps1', { + executable: 'powershell', argumentPrefix: '-File X', + }), false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/commandPermissions.test.js` around lines 5 - 15, Add a negative assertion in the commandMatchesPermissionRule test for a PowerShell command using the same executable and argumentPrefix but a different intermediate argument, such as a non-matching file flag or value. Keep the existing positive and executable-mismatch cases unchanged, and verify the matcher returns false.test/oauthConfiguration.test.js (1)
5-12: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover all renamed credentials and the runtime error path.
Line [9] checks only
SENNORIC_NOTION_CLIENT_ID. It does not verify the GitHub or Google identifiers, or any of the three client secrets changed insrc/oauth/providers.js. Lines [10-12] inspect source text instead of loading the provider map or invokingconnectOAuth, so runtime configuration can regress without failing the test. Assert all six mappings and exercise the missing-credential branch.Cross-file scope covers the three provider changes in
src/oauth/providers.js.Minimum coverage expansion
- assert.match(providers, /SENNORIC_NOTION_CLIENT_ID/); + for (const provider of ['GITHUB', 'GOOGLE', 'NOTION']) { + assert.match(providers, new RegExp(`SENNORIC_${provider}_CLIENT_ID`)); + assert.match(providers, new RegExp(`SENNORIC_${provider}_CLIENT_SECRET`)); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/oauthConfiguration.test.js` around lines 5 - 12, Expand the OAuth test around the provider configuration and connectOAuth flow to verify all six renamed GitHub, Google, and Notion client ID/secret mappings use the SENNORIC credentials and exclude the old AXION identifiers. Load the provider map rather than relying on source-text matches, and invoke connectOAuth with missing credentials to assert the runtime unavailable-error branch and ensure no build instructions are exposed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/oauth/providers.js`:
- Around line 4-5: Update the OAuth configuration around clientId and
clientSecret to use the documented AXION_* environment variable names, and
update README.md and .env.example accordingly. Define an explicit migration
policy for existing SENNORIC_* deployments, such as temporary fallback support
or a required rename, while preserving the unavailable-error behavior when no
credentials are configured.
In `@test/commandPermissions.test.js`:
- Around line 24-35: Update the variable-expansion cases in the test for
commandMatchesPermissionRule so their first argument is test while still
including dynamic expansion, such as test-prefixed forms of $TASK and
"$(whoami)". Keep the existing rule and rejection assertions, ensuring failures
specifically exercise tokenizer handling of variable expansion rather than
argumentPrefix mismatch.
---
Nitpick comments:
In `@test/commandPermissions.test.js`:
- Around line 5-15: Add a negative assertion in the commandMatchesPermissionRule
test for a PowerShell command using the same executable and argumentPrefix but a
different intermediate argument, such as a non-matching file flag or value. Keep
the existing positive and executable-mismatch cases unchanged, and verify the
matcher returns false.
In `@test/oauthConfiguration.test.js`:
- Around line 5-12: Expand the OAuth test around the provider configuration and
connectOAuth flow to verify all six renamed GitHub, Google, and Notion client
ID/secret mappings use the SENNORIC credentials and exclude the old AXION
identifiers. Load the provider map rather than relying on source-text matches,
and invoke connectOAuth with missing credentials to assert the runtime
unavailable-error branch and ensure no build instructions are exposed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 656bb2ea-aa8b-4521-9fe7-8e6f913f0cb6
📒 Files selected for processing (4)
src/oauth/oauth.jssrc/oauth/providers.jstest/commandPermissions.test.jstest/oauthConfiguration.test.js
24cd6fa to
f3c99cf
Compare
f3c99cf to
3ebbc9d
Compare
1 participant
Summary
Verification
Summary by CodeRabbit