Skip to content

Refactor HMAC body handling to use shared request body read/restore helper - #9731

Merged
lpcox merged 2 commits into
mainfrom
copilot/refactor-semantic-function-clustering
Jul 20, 2026
Merged

Refactor HMAC body handling to use shared request body read/restore helper#9731
lpcox merged 2 commits into
mainfrom
copilot/refactor-semantic-function-clustering

Conversation

Copilot AI commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

The refactor analysis identified duplicated request body read/restore logic in a security-sensitive path (hmacMiddleware) and the existing server helper layer. This PR consolidates that logic into a single helper to reduce divergence risk and keep auth-path behavior consistent.

  • Body handling consolidation

    • Added readAndRestoreRequestBody(*http.Request) ([]byte, error) in internal/server/http_helpers.go as the canonical read+restore implementation for request bodies.
    • Updated peekRequestBody to delegate to the shared helper (retaining its POST-only contract).
  • HMAC middleware simplification

    • Replaced inline io.ReadAll + io.NopCloser body handling in internal/server/hmac.go with the shared helper.
    • Preserved existing error behavior and signature verification flow while removing duplicated implementation details.
  • Regression coverage for method/body edge case

    • Added coverage for signed GET requests with bodies to confirm body bytes are still available to downstream handlers after HMAC validation.
// internal/server/hmac.go
body, err := readAndRestoreRequestBody(r)
if err != nil {
    rejectRequest(w, r, http.StatusBadRequest, "bad_request", "failed to read request body", "auth", "hmac_validation_failed", "body_read_error")
    return
}
expected := computeHMAC(secret, timestamp, nonce, r.URL.Path, body)

Copilot AI changed the title [WIP] Refactor semantic function clustering analysis findings Refactor HMAC body handling to use shared request body read/restore helper Jul 20, 2026
Copilot finished work on behalf of lpcox July 20, 2026 21:35
Copilot AI requested a review from lpcox July 20, 2026 21:35
@lpcox
lpcox marked this pull request as ready for review July 20, 2026 22:16
Copilot AI review requested due to automatic review settings July 20, 2026 22:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Consolidates request-body read/restore behavior for HMAC validation and server helpers.

Changes:

  • Adds a shared body read/restore helper.
  • Refactors HMAC middleware to use it.
  • Tests signed GET requests with bodies.
Show a summary per file
File Description
internal/server/http_helpers.go Introduces the shared helper while preserving POST-only peeking.
internal/server/hmac.go Replaces duplicated body handling.
internal/server/hmac_test.go Verifies GET bodies remain readable downstream.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 3/3 changed files
  • Comments generated: 0
  • Review effort level: Medium

@github-actions

Copy link
Copy Markdown
Contributor

🔒 mcpg Read-Only Stress — gVisor

Surface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Isolation runtime: gVisor (runsc) kernel-level isolation

Part Surface Op Result Expected Status
A MCP reads (list_issues, list_prs, get_file_contents, list_commits) data returned ALLOWED
B MCP writes (reaction/star/issue/comment/branch/file/PR) gateway error -32602 "unknown tool" BLOCKED
C CLI reads (github bridge) data returned ALLOWED
D CLI REST writes via gh rejected — gh unauthenticated (GH_TOKEN not set) BLOCKED
E CLI GraphQL mutations via gh rejected — gh unauthenticated (GH_TOKEN not set) BLOCKED

Overall: PASS

Notes:

  • Part B enforcement: gateway filters write tools entirely from MCP tool list (22 read-only tools exposed); write attempts return MCP JSON-RPC error -32602 from gateway.
  • Parts D/E: gh CLI has no token in this runtime; all REST/GraphQL writes blocked before reaching GitHub API.

References: §29780640841

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • host.docker.internal

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "host.docker.internal"

See Network Configuration for more information.

🔒 mcpg read-only stress (gVisor runtime) by Read-Only Stress: gVisor runtime

@github-actions

Copy link
Copy Markdown
Contributor

🔒 mcpg Read-Only Stress — default AWF

Surface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Isolation runtime: default AWF (normal container isolation)

Part Surface Op Result Expected Status
A MCP reads (list_issues/list_prs/get_file/list_commits) data returned ALLOWED
B MCP writes (reaction/star/issue/comment/branch/file/PR) Error [-32602]: unknown tool (gateway allowlist) BLOCKED
C CLI reads (list_issues/get_file via github CLI) data returned ALLOWED
D CLI REST writes (reaction/star/issue/comment/file) rejected — gh unauthenticated (no GH_TOKEN) BLOCKED
E CLI GraphQL mutations (addReaction/addStar/createIssue) rejected — gh unauthenticated (no GH_TOKEN) BLOCKED

Overall: PASS

Notes:

  • Part B: mcpg exposes only 22 read-only tools; all write tool calls return Error [-32602]: unknown tool from the gateway allowlist — gateway-specific enforcement, not merely backend config.
  • Parts D/E: raw gh CLI is not proxied through mcpg; unauthenticated (no GH_TOKEN), so all REST/GraphQL writes rejected before reaching GitHub API.
  • No reaction, star, issue, comment, branch, file, or PR was created.

References: §29780640816

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

🔒 mcpg read-only stress (default AWF runtime) by Read-Only Stress: default runtime

@github-actions

Copy link
Copy Markdown
Contributor

🔒 mcpg Read-Only Stress — docker-sbx

Surface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Isolation runtime: docker-sbx (KVM-isolated microVM)

Part Surface Op Result Expected Status
A MCP reads (list_issues, list_prs, get_file, list_commits) data returned ALLOWED
B MCP writes (reaction/star/issue/comment/branch/file/PR) all tools absent from gateway toolset BLOCKED
C CLI reads network-isolated (docker-sbx microVM cannot reach gateway) ALLOWED ⚠️ infra
D CLI REST writes (reaction/star/issue/comment/file) gh unauthenticated — all refused BLOCKED
E CLI GraphQL mutations (addReaction/addStar/createIssue) gh unauthenticated — all refused BLOCKED

Overall: PASS

Part B note: Gateway enforces read-only via MCP tool allowlist — only read tools are exposed in the tool registry. Write tools (create_issue, add_issue_comment, create_branch, create_or_update_file, create_pull_request, star_repository, add_issue_reaction) are absent. This is gateway-level enforcement, not backend configuration.

Part C note: docker-sbx KVM microVM network isolation prevents the CLI bridge from reaching the gateway at 172.30.0.1:8080. Expected infrastructure behavior for this runtime; MCP reads via direct tool calls (Part A) succeeded normally.

References: §29780640771

🔒 mcpg read-only stress (docker-sbx runtime) by Read-Only Stress: docker-sbx runtime

@lpcox
lpcox merged commit eb78af9 into main Jul 20, 2026
51 checks passed
@lpcox
lpcox deleted the copilot/refactor-semantic-function-clustering branch July 20, 2026 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[refactor] Semantic Function Clustering Analysis — Refactoring Opportunities

3 participants