terrymanu commented on PR #38808:
URL: https://github.com/apache/shardingsphere/pull/38808#issuecomment-4623667788
### Decision
- **Merge Verdict: Mergeable**
- **Reviewed Scope:** PR #38808 latest head
`fe8ddb167008b27ae4bb7f1a2674a7ac1181f3da`, merge-base
`b5aa888dbfb9ca3d1c9c48db3d7b957ee58f2d85`; GitHub file list matched the local
triple-dot diff: `deployment.en.md` and `deployment.cn.md`.
- **Not Reviewed Scope:** CI/check-runs, rendered website output, live
Nginx/Ingress/ALB runtime, and unrelated MCP execution code outside the HTTP
attribution path.
- **Need Expert Review:** No mandatory expert-review blocker remains;
optional security/operator review can still help before publishing
remote-exposure guidance.
### Basis
No blocking issue found in the latest revision.
The previous header-forgery risk is addressed. Both Nginx examples now use
`proxy_pass_request_headers off` and explicitly allow-list the request headers
forwarded to MCP, including `MCP-Session-Id` and `MCP-Protocol-Version`. The
attribution example then injects trusted `X-ShardingSphere-MCP-*` values from
the gateway side, and the text also tells operators to allow-list, strip, or
reject client-supplied attribution headers before trusted injection.
This matches the runtime semantics I checked: `SessionAttributionResolver`
collects all headers matching `attributeHeaderPrefix`, so preventing arbitrary
client-supplied prefixed headers is required; `MCPSessionManager` and the HTTP
validator keep attribution stable for an existing session; omitting
`sessionAttributionSource` remains documented for deployments that do not need
attribution binding.
The docs also keep the intended security boundary clear: built-in HTTP still
does not provide authentication/authorization, and remote exposure remains
delegated to a trusted gateway.
### Verification
- Refreshed PR metadata and files from GitHub: 2 changed files, 184
additions, no review comments or issue comments.
- `git fetch apache master:refs/remotes/apache/master
pull/38808/head:refs/remotes/apache/pr/38808` exited 0.
- `git diff --name-status
refs/remotes/apache/master...refs/remotes/apache/pr/38808` exited 0 and matched
GitHub’s file list.
- `git diff --check
refs/remotes/apache/master...refs/remotes/apache/pr/38808` exited 0.
- Inspected official Nginx proxy header directive docs for the allow-list
approach:
-
https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_pass_request_headers
-
https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_set_header
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
