feat(adt-mcp): enforce scoped safe execution#143
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
✅ Deploy Preview for adt-cli canceled.
|
MergerNeeds Review PR exceeds the merge-gate context budget (70159 tokens); escalating to a human reviewer. Commit |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds execution-scoped cancellation to the ADT client and introduces scoped MCP authorization for read and ChangesExecution safety
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant DestinationMode
participant AuthorizationHooks
participant ADTTool
participant ADTServerRuntime
MCPClient->>DestinationMode: invoke scoped safe_execute tool
DestinationMode->>AuthorizationHooks: consumeExecutionAuthorization
AuthorizationHooks-->>DestinationMode: authorization admitted
DestinationMode->>ADTServerRuntime: executeWithDeadline
ADTServerRuntime->>ADTTool: run under abort signal
ADTTool-->>DestinationMode: result or failure
DestinationMode->>AuthorizationHooks: reportExecutionOutcome
DestinationMode-->>MCPClient: controlled result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR successfully implements scoped safe-execute enforcement for ADT operations with comprehensive security controls. The implementation follows security best practices with fail-closed verification, cryptographic signature validation, and atomic grant consumption. The extensive test coverage validates all critical security boundaries including policy enforcement, grant replay prevention, and proper error handling. No blocking issues found - the code is ready for merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 500 |
| Duplication | 28 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
PR Summary by QodoEnforce execution-scoped safe execution for ADT MCP (signed adt-execution-v1)
AI Description
Diagram
High-Level Assessment
Files changed (25)
|
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…p fake JWT in test Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
|
View your CI Pipeline Execution ↗ for commit 89e5490
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
…nied / safe-execute limit payloads via utils.ts helpers\n- Track per-execution dispatch counters in destination-mode.ts\n- Include program and class alerts in AUnit resultCounts\n- Add optional objectType to get_object and fail-closed on empty scoped resourceKeys\n- Relax systemSid regex and sort arrays once in snapshotScopedAccess\n- Keep sessionPath/csrfToken for DELETE on cancellation; avoid clearing shared SessionManager state\n- Make runWithAdtAbortSignal async so synchronous abort throws become rejected promises\n- Compose test JWT fixture from segments to avoid scanner match\n- Use shared safeExecuteLimitResult helper in atc-run and run-unit-tests Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
|
test |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…alth\n\n- Extract parseHandlerArgs, isScopeAllowed, maxToolCallsReached, incrementCounter, and runSafeExecution helpers in destination-mode.ts\n- Extract buildSessionHeaders and deleteSecuritySession private methods in SessionManager to reduce initializeCsrf nesting\n- Keep abort cleanup behavior unchanged (DELETE session without clearing shared state) Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
…\n- Deduplicate safe-execute payload in run-unit-tests coverage limit path by using safeExecuteLimitResult()\n- Reserve maxToolCalls slots before any awaited work and roll back on denial\n- Evict safe-execute per-execution counter after the terminal outcome is reported\n- Keep fail-closed scope behavior for destination-mode safe and non-safe paths Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- parseSafeExecutePolicy: split into operation-specific helpers to reduce cognitive complexity - snapshotScopedAccess: use localeCompare in sorted-unique check - session helpers: add nosemgrep suppressions for ADT URL fetch Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…inted-URL rule Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… session URLs Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- destination-mode: hide safe_execute tools when execution hooks are missing - server.ts: pass full destination options to tool list projection - get-object: make objectType required and update README/tests - session.ts: add positional overload, SSRF session-path validation, cleanup on failed CSRF acquisition, avoid caching cleanup CSRF token - http/server.ts: restore uniqueness check in isSortedUniqueStrings Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ecute helper dedup - session.ts: normalize sessionPath via new URL() before origin/path checks so dot-segments like ../ cannot bypass the sessions-prefix guard - atc-run.ts & run-unit-tests.ts: share safePolicy extraction and safe-execute error handling via extractSafeExecutePolicy / handleSafeExecuteError Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…fe execution atc_run now catches the 'ATC object is unavailable' error and returns mcpErrorResult instead of letting handleSafeExecuteError rethrow it, so destination-mode records a deterministic 'failed' outcome instead of 'outcome_unknown'. Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…lists - Introduce AtcObjectUnavailableError and match it with instanceof instead of a literal message string. - Resolve the ATC scope URIs before creating the server-side worklist, so a missing-object failure no longer leaves an orphaned worklist behind. - Return mcpErrorResult for the deterministic not-found case so destination-mode records 'failed' rather than 'outcome_unknown'. Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
- require objectType in get_object_structure so scoped read can build a canonical key - parseScopedObjectKeys now rejects empty resourceKeys for read and safe_execute scopes - remove exact-key checks on JWT payload/protected header; validate only required alg/kid/typ and trusted claim fields, so issuers can add standard/extra claims without breaking existing invocation policies Co-Authored-By: Petr Plenkov <petr.plenkov@gmail.com>
|
Note Automatic reviews are paused because your trial's included automatic processing has been used for this period. Upgrade now, or comment "Gitar review" to run a review anytime. Code Review ✅ Approved 7 resolved / 7 findingsEnforces signed adt-execution-v1 policies and execution-scoped safe execution across the MCP server, client, and runtime, addressing 7 review findings including concurrency safety, cleanup delays, and error matching. ✅ 7 resolved✅ Performance: Redundant re-sorting inside .some() validators
✅ Performance: Per-execution dispatch counters map is never evicted
✅ Edge Case: maxToolCalls admission not concurrency-safe for same executionId
✅ Performance: Abort cleanup can delay cancellation by up to ~10s
✅ Bug: Sort check switched from localeCompare to default sort ordering
...and 2 more resolved from earlier reviews OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
1 participant
User description
Purpose
This PR adds execution-scoped safe execution enforcement for the ADT MCP server, implementing a generic signed
adt-execution-v1policy with strict bounds on tool calls, duration, and result sizes.Changes
adt-execution-v1policy with exact destination, resource, tool, operation, and call limitsVerification
git diff --check: passedSummary by cubic
Enforces scoped safe execution in
@abapify/adt-mcpusing signedadt-execution-v1policies, with a hard‑cancellable runtime across@abapify/adt-clientand@abapify/adt-server. Tightens scope gating, requiresobjectTypeforget_objectandget_object_structure, hardens ADT session cleanup, and relaxes JWT validation to accept extra claims.New Features
adt-execution-v1policies (destination/resources, allowlisted tools/classes, max tool calls, deadlines, result limits); prevents replay and records one terminal outcome.atc_runandrun_unit_testsonly undersafe_execute; hidessafe_executetools when hooks are missing; deterministic SAP HTTP failures become terminal outcomes; resolves ATC scope URIs before worklist creation; returns a normal MCP error for missing ATC objects; includes program/class alerts in totals.run_unit_tests; fail‑closed on emptyresourceKeysfor both read and safe‑execute; exact canonical comparisons;get_objectandget_object_structurenow require uppercasedobjectTypeand normalize base types.AbortSignalthrough client and server;runWithAdtAbortSignalis async; preserves session path/CSRF for DELETE; validates and normalizes ADT session paths (blocks dot‑segments), passesURLobjects tofetch, and cleans up on failed CSRF acquisition without caching cleanup tokens.maxToolCallsand rolls back on denial; per‑execution counters persist across calls (limits enforced after terminal outcome); deduplicates helpers (extractSafeExecutePolicy,handleSafeExecuteError,safeExecuteLimitResult); exportsparseScopedAdtInvocationPolicy,parseSafeExecutePolicy, and related types.alg/kid/typand trusted claim fields so issuers can add extra claims without breaking policies.Migration
consumeExecutionAuthorization,reportExecutionOutcome, andexecuteWithDeadlinevia@abapify/adt-server(defaults toexecuteWithDeadlineAndAbort); withoutexecuteWithDeadline, safe execution is denied andsafe_executetools are hidden.get_objectandget_object_structurecalls to includeobjectType(e.g.CLAS,PROG).agentIdadt-executionandconstraint.kindadt-execution-v1, including resource/tool scope and limits.Written for commit 89e5490. Summary will update on new commits.
Generated description
Below is a concise technical summary of the changes proposed in this PR:
graph LR subgraph "@abapify/adt-mcp" ["@abapify/adt-mcp"] createMcpServer_("createMcpServer"):::modified destinationModeServer_("destinationModeServer"):::modified parseScopedAdtInvocationPolicy_("parseScopedAdtInvocationPolicy"):::added parseSafeExecutePolicy_("parseSafeExecutePolicy"):::added registerRunUnitTestsTool_("registerRunUnitTestsTool"):::modified extractSafeExecutePolicy_("extractSafeExecutePolicy"):::added handleSafeExecuteError_("handleSafeExecuteError"):::added registerAtcRunTool_("registerAtcRunTool"):::modified createMcpServer_ -- "Pass scoped authorization/outcome/deadline hooks to destinationModeServer." --> destinationModeServer_ parseScopedAdtInvocationPolicy_ -- "Materializes scoped safeExecutePolicy from constraint for safe execution." --> parseSafeExecutePolicy_ registerRunUnitTestsTool_ -- "Derives run_unit_tests safePolicy limits from requestAccess.scoped." --> extractSafeExecutePolicy_ registerRunUnitTestsTool_ -- "Formats known HTTP failures; rethrows nondeterministic under safePolicy." --> handleSafeExecuteError_ registerAtcRunTool_ -- "Derives atc_run safePolicy limits from requestAccess.scoped constraints." --> extractSafeExecutePolicy_ registerAtcRunTool_ -- "Handles deterministic ATC failures; propagates unknown outcomes." --> handleSafeExecuteError_ classDef added stroke:#15AA7A classDef removed stroke:#CD5270 classDef modified stroke:#EDAC4C linkStyle default stroke:#CBD5E1,font-size:13px end subgraph "@abapify/adt-client" ["@abapify/adt-client"] SessionManager_initializeCsrf_("SessionManager.initializeCsrf"):::modified SessionManager_buildSessionsUrl_("SessionManager.buildSessionsUrl"):::added SessionManager_createSecuritySession_("SessionManager.createSecuritySession"):::added SessionManager_acquireCsrfToken_("SessionManager.acquireCsrfToken"):::added SessionManager_deleteSecuritySession_("SessionManager.deleteSecuritySession"):::added SessionManager_deleteSecuritySessionWithFallback_("SessionManager.deleteSecuritySessionWithFallback"):::added SessionManager_handleCsrfInitializationError_("SessionManager.handleCsrfInitializationError"):::added SessionManager_fetchSecuritySessionCsrfToken_("SessionManager.fetchSecuritySessionCsrfToken"):::added SessionManager_initializeCsrf_ -- "Builds trusted sessions URL with optional sap-client/sap-language." --> SessionManager_buildSessionsUrl_ SessionManager_initializeCsrf_ -- "Creates temporary security session via GET, extracts trusted session path." --> SessionManager_createSecuritySession_ SessionManager_initializeCsrf_ -- "Acquires CSRF token using x-sap-security-session use, respects abort signal." --> SessionManager_acquireCsrfToken_ SessionManager_initializeCsrf_ -- "Deletes temporary session with cached CSRF, passes abort signal." --> SessionManager_deleteSecuritySession_ SessionManager_initializeCsrf_ -- "On failure, performs fallback cleanup using fresh CSRF token." --> SessionManager_deleteSecuritySessionWithFallback_ SessionManager_initializeCsrf_ -- "Centralizes initialization errors; triggers best-effort session slot cleanup." --> SessionManager_handleCsrfInitializationError_ SessionManager_deleteSecuritySessionWithFallback_ -- "Fetches fresh cleanup CSRF token, avoiding shared CSRF pollution." --> SessionManager_fetchSecuritySessionCsrfToken_ SessionManager_fetchSecuritySessionCsrfToken_ -- "Uses fetched token to issue DELETE for temporary session." --> SessionManager_deleteSecuritySession_ classDef added stroke:#15AA7A classDef removed stroke:#CD5270 classDef modified stroke:#EDAC4C linkStyle default stroke:#CBD5E1,font-size:13px end subgraph "@abapify/adt-server" ["@abapify/adt-server"] main_("main"):::modified createAdtServerMcpOptions_("createAdtServerMcpOptions"):::modified executeWithDeadlineAndAbort_("executeWithDeadlineAndAbort"):::added AdtServerMcpRuntimeOptions_("AdtServerMcpRuntimeOptions"):::modified ADT_MCP_("ADT_MCP"):::modified ADT_CLIENT_("ADT_CLIENT"):::added createServerMcpHandler_("createServerMcpHandler"):::modified AdtServerMcpOptions_("AdtServerMcpOptions"):::modified main_ -- "Passes executeWithDeadlineAndAbort into createAdtServerMcpOptions configuration." --> createAdtServerMcpOptions_ main_ -- "Supplies execution deadline handler to MCP runtime options." --> executeWithDeadlineAndAbort_ createAdtServerMcpOptions_ -- "AdtServerMcpRuntimeOptions now includes authorization, outcome, and deadline overrides." --> AdtServerMcpRuntimeOptions_ createAdtServerMcpOptions_ -- "Defaults executeWithDeadline to executeWithDeadlineAndAbort when absent." --> executeWithDeadlineAndAbort_ createAdtServerMcpOptions_ -- "ADt-mcp setup now supports safe_execute authorization/outcome integration." --> ADT_MCP_ executeWithDeadlineAndAbort_ -- "Uses runWithAdtAbortSignal to abort and await deadline enforcement." --> ADT_CLIENT_ createServerMcpHandler_ -- "createServerMcpHandler forwards safe-execute hooks from AdtServerMcpOptions." --> AdtServerMcpOptions_ classDef added stroke:#15AA7A classDef removed stroke:#CD5270 classDef modified stroke:#EDAC4C linkStyle default stroke:#CBD5E1,font-size:13px endEnforce execution-scoped safe execution across the ADT MCP flow by adding signed
adt-execution-v1policies, scoped tool/resource validation, and one-shot grant consumption with terminal outcome reporting. Propagate execution abort signals through@abapify/adt-clientand@abapify/adt-server, harden ADT session cleanup, and requireobjectTypeforget_object.Modified files (5)
Latest Contributors(2)
adt-execution-v1grants for read and safe-execute tools, including exact resource/tool matching,maxToolCalls, deadlines, result limits, and terminal outcome reporting.Modified files (21)
Latest Contributors(2)
Modified files (6)
Latest Contributors(2)