Extract shared enclave/delegated request parse-and-route helper - #13694
Conversation
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…l branch Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Enclave route planning now runs before capability verification, changing the documented security-check order and causing pre-authentication route logging.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
What changed in this PR
Extracts shared enclave/delegation request parsing and routing logic to reduce security-critical duplication.
Changes:
- Adds staged request planning and route/tool resolution.
- Updates enclave and delegated handlers to consume shared plans.
- Adds request-planning tests.
| File | Description |
|---|---|
internal/proxy/enclave.go |
Uses shared request plans. |
internal/proxy/enclave_request.go |
Implements shared planning logic. |
internal/proxy/enclave_request_test.go |
Tests planning behavior. |
internal/proxy/delegation.go |
Uses shared plans for delegated requests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func (h *proxyHandler) handleEnclaveRequest(w http.ResponseWriter, r *http.Request) { | ||
| path, ok := enclavePath(r.URL.Path, r.URL.RawPath) | ||
| if !ok { | ||
| plan := planEnclaveRequest(r) |
🔒 mcpg Read-Only Stress — defaultSurface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Overall: INCONCLUSIVE No write leaked on any surface. Part A/C reads confirmed live data (README.md,
No artifacts were created; no reactions, stars, issues, comments, branches, or files were added.
|
🔒 mcpg Read-Only Stress — gvisorSurface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Overall: INCONCLUSIVE Notes:
|
|
@copilot address review feedback |
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Addressed in |

handleEnclaveRequest(internal/proxy/enclave.go) andhandleDelegatedRequest(internal/proxy/delegation.go) each re-implemented the same admission skeleton — path extraction, GET/body validation, query parse, route match, tool/args resolution,fullPathreconstruction,handleWithDIFCdispatch — differing only in the authorization mechanism. Since both are security enforcement points, the copies could silently drift on validation order or denial conditions.Changes
internal/proxy/enclave_request.go:planEnclaveRequest(r)performs all shared request-shape work and returns anenclaveRequestPlancarryingpath,fullPath,route,toolName,args, and adenialstage (path/requestShape/route/tool). The staged denial lets each handler keep its own log message while sharing one code path; every stage still yields the same 403 to clients.routeAllowedBy(claims): nil-safe operation-policy check soplan.routeis only dereferenced after a successful match.enclave_request_test.go): table-driven coverage for accepted routes, host-prefix stripping, invalid path, non-GET and GET-with-body, unmatched route, and tool resolution.