fix(security): drop IP-literal refusal from download URL validation
Confirmed with the product team that the official GUI client applies no
client-side SSRF interception to downloads, so the IP-literal refusal
was the last remaining client-side interception beyond transport
hygiene. Dedicated deployments make every host dimension (domain,
port, network location) unenumerable, and an IP-literal host is just
another network location.
- ValidateDownloadURL accepts any HTTPS host, IP literals included;
HTTPS scheme, userinfo refusal, and per-hop redirect re-validation
stay as the transport baseline
- this retires the third and last interception layer after the host
allowlist removal and the dial-time public-IP refusal (be52ca5d)
- upload targets stay unaffected: trustedUploadHost keeps rejecting
non-DingTalk/OSS hosts, so local file bytes cannot be PUT to an IP
- regressions: IP-literal URLs pass validation on the shared and chat
download paths; userinfo and plain-HTTP URLs stay rejected
This commit is contained in:
liyuan
@@ -3,20 +3,20 @@ category: Changed
|
||||
---
|
||||
|
||||
- **Download host trust policy** — retires the static DingTalk/OSS download
|
||||
host allowlist and the dial-time public-IP refusal from both the shared
|
||||
local download path (`drive +download`, `drive +version-download`,
|
||||
doc/minutes artifact downloads) and the chat message-resource path
|
||||
(`chat +messages-resource-download`, `--download-resources`). Download
|
||||
URLs still require an HTTPS domain (IP literals and userinfo URLs stay
|
||||
rejected) and now accept non-default HTTPS ports, because every dimension
|
||||
of a dedicated-deployment storage endpoint — custom domain, port, and
|
||||
network location — is decided by the customer deployment and cannot be
|
||||
enumerated or configured client-side. Verified on a dedicated deployment
|
||||
whose storage domain resolves to a customer-intranet address. Downloads
|
||||
align with the official GUI client, which applies no client-side SSRF
|
||||
host allowlist, the dial-time public-IP refusal, and the IP-literal
|
||||
refusal from both the shared local download path (`drive +download`,
|
||||
`drive +version-download`, doc/minutes artifact downloads) and the chat
|
||||
message-resource path (`chat +messages-resource-download`,
|
||||
`--download-resources`). Download URLs only require HTTPS without userinfo
|
||||
and accept non-default HTTPS ports, because every dimension of a
|
||||
dedicated-deployment storage endpoint — custom domain, port, and network
|
||||
location — is decided by the customer deployment and cannot be enumerated
|
||||
or configured client-side. Verified on a dedicated deployment whose
|
||||
storage domain resolves to a customer-intranet address. Downloads align
|
||||
with the official GUI client, which applies no client-side SSRF
|
||||
interception: download URLs only ever come from authenticated service
|
||||
responses (no command accepts a user-supplied URL), TLS hostname
|
||||
verification pins the connection to the requested domain, redirects are
|
||||
verification pins the connection to the requested host, redirects are
|
||||
re-validated per hop, and service credential headers are stripped once a
|
||||
redirect leaves the original origin.
|
||||
- **Upload host trust unchanged** — upload target URLs (`drive +upload`,
|
||||
|
||||
@@ -290,20 +290,15 @@ func SafeFilename(preferredName, rawURL string) string {
|
||||
return "download"
|
||||
}
|
||||
|
||||
// ValidateDownloadURL accepts HTTPS download URLs on any port. Hosts must be
|
||||
// domain names, never IP literals or userinfo URLs; TLS hostname
|
||||
// verification in secureHTTPClient then pins the connection to the requested
|
||||
// domain. Dedicated-deployment storage domains legitimately use custom
|
||||
// domains, non-default ports, and customer-intranet addresses, so the port
|
||||
// and the resolved network location are not client-side trust signals.
|
||||
// ValidateDownloadURL accepts HTTPS download URLs on any host and port, IP
|
||||
// literals included — mirroring the official GUI client, which applies no
|
||||
// client-side SSRF interception to downloads. Only userinfo URLs stay
|
||||
// rejected; TLS hostname verification in secureHTTPClient pins the
|
||||
// connection to the requested host and redirects are re-validated per hop.
|
||||
func ValidateDownloadURL(rawURL string) (*url.URL, error) {
|
||||
parsed, err := url.Parse(strings.TrimSpace(rawURL))
|
||||
if err != nil || parsed.Scheme != "https" || parsed.Host == "" || parsed.User != nil {
|
||||
return nil, fmt.Errorf("下载地址必须是受信任域名上的 HTTPS URL")
|
||||
}
|
||||
host := strings.ToLower(strings.TrimSuffix(parsed.Hostname(), "."))
|
||||
if host == "" || net.ParseIP(host) != nil {
|
||||
return nil, fmt.Errorf("下载地址不允许使用 IP 直连，必须是 HTTPS 域名")
|
||||
if err != nil || parsed.Scheme != "https" || parsed.Hostname() == "" || parsed.User != nil {
|
||||
return nil, fmt.Errorf("下载地址必须是合法的 HTTPS URL")
|
||||
}
|
||||
return parsed, nil
|
||||
}
|
||||
|
||||
@@ -64,14 +64,16 @@ func TestCrossPlatformCoverageDownloadURLPolicy(t *testing.T) {
|
||||
valid := []string{
|
||||
"https://alidocs.dingtalk.com/file.docx",
|
||||
"https://alidocs.oss-cn-zhangjiakou.aliyuncs.com/res/file.md",
|
||||
// 域名白名单已移除：任意 HTTPS 域名在 URL 校验层放行，
|
||||
// SSRF 防护由拨号层的公网 IP 强制校验承担。
|
||||
// 域名白名单、IP 直连拦截与拨号层公网 IP 校验均已移除：
|
||||
// 任意 HTTPS 主机（含 IP 字面量）在 URL 校验层放行，对齐
|
||||
// GUI 客户端（无客户端侧 SSRF 拦截）。
|
||||
"https://ddoss.ijingbo.chambroad.com/file.doc",
|
||||
// 专属部署存储域可服务在非默认端口；端口不是信任信号，
|
||||
// 拨号层公网 IP 校验与端口号无关。
|
||||
// 专属部署存储域可服务在非默认端口；端口不是信任信号。
|
||||
"https://ddoss.ijingbo.chambroad.com:8443/file.doc",
|
||||
"https://evil.example/file.docx",
|
||||
"https://oss-cn-hangzhou-internal.aliyuncs.com/file.docx",
|
||||
"https://127.0.0.1/file.docx",
|
||||
"https://[::1]/file.docx",
|
||||
}
|
||||
for _, raw := range valid {
|
||||
if _, err := ValidateDownloadURL(raw); err != nil {
|
||||
@@ -80,7 +82,6 @@ func TestCrossPlatformCoverageDownloadURLPolicy(t *testing.T) {
|
||||
}
|
||||
invalid := []string{
|
||||
"http://alidocs.dingtalk.com/file.docx",
|
||||
"https://127.0.0.1/file.docx",
|
||||
"https://user@alidocs.dingtalk.com/file.docx",
|
||||
"http://alidocs.dingtalk.com:8443/file.docx",
|
||||
"https://alidocs.dingtalk.com:NOTAPORT/file.docx",
|
||||
|
||||
@@ -317,11 +317,12 @@ func isAliyunOSSHost(host string) bool {
|
||||
}
|
||||
|
||||
// validateResourceDownloadURL enforces the shared localio download URL
|
||||
// policy: HTTPS domain names only, never IP literals or userinfo URLs.
|
||||
// Resource URLs are always resolved from an authenticated MCP response
|
||||
// (never user-supplied), and localio.SecureHTTPClient keeps TLS hostname
|
||||
// verification plus redirect credential hygiene, mirroring the official GUI
|
||||
// client which applies no client-side SSRF interception to downloads.
|
||||
// policy: HTTPS URLs without userinfo, on any host or port, IP literals
|
||||
// included. Resource URLs are always resolved from an authenticated MCP
|
||||
// response (never user-supplied), and localio.SecureHTTPClient keeps TLS
|
||||
// hostname verification plus redirect credential hygiene, mirroring the
|
||||
// official GUI client which applies no client-side SSRF interception to
|
||||
// downloads.
|
||||
func validateResourceDownloadURL(rawURL string) (*url.URL, error) {
|
||||
parsed, err := localio.ValidateDownloadURL(rawURL)
|
||||
if err != nil {
|
||||
|
||||
@@ -188,19 +188,19 @@ func TestCrossPlatformCoverageResourceDownloadValidationAndInfo(t *testing.T) {
|
||||
t.Errorf("isAliyunOSSHost(%q) = %v, want %v", host, got, want)
|
||||
}
|
||||
}
|
||||
// Host trust is no longer a static allowlist: any HTTPS domain (including
|
||||
// dedicated-deployment download hosts) passes URL validation, while IP
|
||||
// literals, userinfo URLs, and plain HTTP stay rejected. Non-default HTTPS
|
||||
// ports are accepted: dedicated storage domains legitimately serve on them
|
||||
// and SSRF protection is enforced at dial time by localio.SecureHTTPClient.
|
||||
// Host trust is no longer a static allowlist: any HTTPS host (including
|
||||
// dedicated-deployment download hosts and IP literals) passes URL
|
||||
// validation, while userinfo URLs and plain HTTP stay rejected.
|
||||
// Non-default HTTPS ports are accepted: dedicated storage domains
|
||||
// legitimately serve on them.
|
||||
for rawURL, wantOK := range map[string]bool{
|
||||
"https://download.dingtalk.com/file": true,
|
||||
"https://bucket.oss-cn-hangzhou.aliyuncs.com/file": true,
|
||||
"https://ddoss.tenant.example.com/file": true,
|
||||
"https://ddoss.tenant.example.com:8443/file": true,
|
||||
"https://203.0.113.5/file": true,
|
||||
"http://download.dingtalk.com/file": false,
|
||||
"http://download.dingtalk.com:8443/file": false,
|
||||
"https://203.0.113.5/file": false,
|
||||
"https://user:secret@download.dingtalk.com/file": false,
|
||||
} {
|
||||
if _, err := validateResourceDownloadURL(rawURL); (err == nil) != wantOK {
|
||||
@@ -426,13 +426,6 @@ func TestCrossPlatformCoverageDownloadResourceHTTPFailures(t *testing.T) {
|
||||
if _, err := downloadResourceAtomically(context.Background(), resourceResponseClient(200, "x", 2), "https://download.dingtalk.com/file", nil, dest, false); err == nil {
|
||||
t.Fatal("content-length mismatch was accepted")
|
||||
}
|
||||
nilClientDest := filepath.Join(t.TempDir(), "nil-client")
|
||||
if _, err := downloadResourceAtomically(
|
||||
context.Background(), nil, "https://203.0.113.5/file",
|
||||
map[string]string{"X-Test": "ok"}, nilClientDest, true,
|
||||
); err == nil {
|
||||
t.Fatal("nil-client path accepted an IP-literal URL")
|
||||
}
|
||||
}
|
||||
|
||||
func TestCrossPlatformCoverageDownloadResourceDedicatedHostWithHeaders(t *testing.T) {
|
||||
@@ -467,11 +460,18 @@ func TestCrossPlatformCoverageDownloadResourceNilClientUsesSecureDefault(t *test
|
||||
}, nil
|
||||
})}
|
||||
}
|
||||
if _, err := downloadResourceAtomically(
|
||||
context.Background(), nil, "https://download.dingtalk.com/file", nil,
|
||||
filepath.Join(t.TempDir(), "nil-secure"), false,
|
||||
); err != nil {
|
||||
t.Fatal(err)
|
||||
// IP-literal download hosts pass the same host-agnostic HTTPS policy as
|
||||
// domain hosts: the GUI client applies no client-side SSRF interception.
|
||||
for _, resourceURL := range []string{
|
||||
"https://download.dingtalk.com/file",
|
||||
"https://203.0.113.5/file",
|
||||
} {
|
||||
if _, err := downloadResourceAtomically(
|
||||
context.Background(), nil, resourceURL, nil,
|
||||
filepath.Join(t.TempDir(), "nil-secure"), false,
|
||||
); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
}
|
||||
if !served {
|
||||
t.Fatal("nil client did not route through the secure default client")
|
||||
|
||||
@@ -81,7 +81,6 @@ func TestCrossPlatformCoverageResourceDownloadInfo(t *testing.T) {
|
||||
t.Fatal("plain HTTP URL unexpectedly accepted")
|
||||
}
|
||||
for _, resourceURL := range []string{
|
||||
"https://127.0.0.1/file",
|
||||
"https://user:secret@download.dingtalk.com/file",
|
||||
"http://download.dingtalk.com:8443/file",
|
||||
} {
|
||||
@@ -91,15 +90,17 @@ func TestCrossPlatformCoverageResourceDownloadInfo(t *testing.T) {
|
||||
t.Fatalf("untrusted URL %q unexpectedly accepted", resourceURL)
|
||||
}
|
||||
}
|
||||
// The static host allowlist is retired: dedicated-deployment download
|
||||
// hosts pass the same host-agnostic HTTPS policy as DingTalk/OSS hosts;
|
||||
// SSRF protection moved to dial time (localio.SecureHTTPClient).
|
||||
// The static host allowlist and the IP-literal refusal are both retired:
|
||||
// dedicated-deployment download hosts and IP literals pass the same
|
||||
// host-agnostic HTTPS policy as DingTalk/OSS hosts, mirroring the GUI
|
||||
// client which applies no client-side SSRF interception.
|
||||
// Non-default HTTPS ports are accepted too: dedicated storage domains
|
||||
// legitimately serve on them and the dial-time policy is port-agnostic.
|
||||
// legitimately serve on them.
|
||||
for _, resourceURL := range []string{
|
||||
"https://download.dingtalk.com/file",
|
||||
"https://ddoss.tenant.example.com/file",
|
||||
"https://ddoss.tenant.example.com:8443/file",
|
||||
"https://127.0.0.1/file",
|
||||
} {
|
||||
if _, _, err := resourceDownloadInfo(
|
||||
map[string]any{"resourceUrl": resourceURL},
|
||||
|
||||
Reference in New Issue
Block a user