From 1ba6bec8c11fe25b7cf7d7d41a4d7d087e072fd5 Mon Sep 17 00:00:00 2001 From: liyuan Date: Tue, 25 Aug 2026 19:25:20 +0800 Subject: [PATCH] fix(security): accept non-default HTTPS ports for download URLs Customer testing on a dedicated deployment found real download URLs served on a non-default HTTPS port (e.g. 8443) by the dedicated storage domain, which the inherited default-port-only rule rejected. - drop the 443-only restriction from ValidateDownloadURL; HTTPS scheme, domain-only hosts (no IP literals), and no-userinfo rules stay - the port is not a trust signal: SSRF protection lives at dial time in the port-agnostic public-IP policy - redirect hygiene unchanged: a port change is a cross-origin redirect and still strips service credential headers (new regression guard) - dedicated-deployment regression: same host on a non-default port is accepted by URL validation and downloads successfully --- .changes/download-host-allowlist-removal.md | 7 +++++-- internal/localio/download.go | 12 ++++++------ internal/localio/download_test.go | 17 ++++++++++++++++- .../chat/resource_download_coverage_test.go | 8 +++++--- .../shortcut/chat/resource_download_test.go | 5 ++++- 5 files changed, 36 insertions(+), 13 deletions(-) diff --git a/.changes/download-host-allowlist-removal.md b/.changes/download-host-allowlist-removal.md index bbb45b27..ce5088f6 100644 --- a/.changes/download-host-allowlist-removal.md +++ b/.changes/download-host-allowlist-removal.md @@ -6,8 +6,11 @@ category: Changed host allowlist 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 on the - default port (IP literals and userinfo URLs stay rejected), and SSRF + `--download-resources`). Download URLs still require an HTTPS domain (IP + literals and userinfo URLs stay rejected), and non-default HTTPS ports are + now accepted because dedicated-deployment storage domains legitimately + serve on them; the port is not a trust signal and the dial-time SSRF policy + is port-agnostic. SSRF protection now happens at dial time through a shared public-IP policy that refuses hosts resolving to private, loopback, link-local, or otherwise non-public addresses. Dedicated-deployment storage domains now download diff --git a/internal/localio/download.go b/internal/localio/download.go index b2078428..549ddcf4 100644 --- a/internal/localio/download.go +++ b/internal/localio/download.go @@ -292,9 +292,12 @@ func SafeFilename(preferredName, rawURL string) string { return "download" } -// ValidateDownloadURL accepts HTTPS download URLs on the default port. Hosts -// must be domain names, never IP literals or userinfo URLs; SSRF protection is -// enforced at dial time by the public-IP policy in secureHTTPClient. +// ValidateDownloadURL accepts HTTPS download URLs on any port. Hosts must be +// domain names, never IP literals or userinfo URLs; SSRF protection is +// enforced at dial time by the public-IP policy in secureHTTPClient, which is +// port-agnostic because the destination IP is what determines reachability. +// Dedicated-deployment storage domains legitimately serve HTTPS on +// non-default ports, so the port is not a trust signal. 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 { @@ -304,9 +307,6 @@ func ValidateDownloadURL(rawURL string) (*url.URL, error) { if host == "" || net.ParseIP(host) != nil { return nil, fmt.Errorf("下载地址不允许使用 IP 直连，必须是 HTTPS 域名") } - if port := parsed.Port(); port != "" && port != "443" { - return nil, fmt.Errorf("下载地址只允许 HTTPS 默认端口") - } return parsed, nil } diff --git a/internal/localio/download_test.go b/internal/localio/download_test.go index f65357d0..e9a628f9 100644 --- a/internal/localio/download_test.go +++ b/internal/localio/download_test.go @@ -67,6 +67,9 @@ func TestCrossPlatformCoverageDownloadURLAndPublicIPPolicy(t *testing.T) { // 域名白名单已移除：任意 HTTPS 域名在 URL 校验层放行， // SSRF 防护由拨号层的公网 IP 强制校验承担。 "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", } @@ -79,7 +82,8 @@ func TestCrossPlatformCoverageDownloadURLAndPublicIPPolicy(t *testing.T) { "http://alidocs.dingtalk.com/file.docx", "https://127.0.0.1/file.docx", "https://user@alidocs.dingtalk.com/file.docx", - "https://alidocs.dingtalk.com:8443/file.docx", + "http://alidocs.dingtalk.com:8443/file.docx", + "https://alidocs.dingtalk.com:NOTAPORT/file.docx", } for _, raw := range invalid { if _, err := ValidateDownloadURL(raw); err == nil { @@ -591,6 +595,17 @@ func TestCrossPlatformCoverageSecureHTTPClientStripsCrossOriginHeaders(t *testin t.Fatal("same-origin redirect unexpectedly stripped request headers") } + sameHostNonDefaultPort := &http.Request{ + URL: mustURL(t, "https://download.dingtalk.com:8443/next"), + Header: original.Header.Clone(), + } + if err := client.CheckRedirect(sameHostNonDefaultPort, []*http.Request{original}); err != nil { + t.Fatal(err) + } + if len(sameHostNonDefaultPort.Header) != 0 { + t.Fatal("port change is a cross-origin redirect; headers must be stripped") + } + crossOrigin := &http.Request{ URL: mustURL(t, "https://attacker-bucket.oss-cn-hangzhou.aliyuncs.com/next"), Header: original.Header.Clone(), diff --git a/internal/shortcut/chat/resource_download_coverage_test.go b/internal/shortcut/chat/resource_download_coverage_test.go index e6236e45..15729cec 100644 --- a/internal/shortcut/chat/resource_download_coverage_test.go +++ b/internal/shortcut/chat/resource_download_coverage_test.go @@ -190,16 +190,18 @@ func TestCrossPlatformCoverageResourceDownloadValidationAndInfo(t *testing.T) { } // Host trust is no longer a static allowlist: any HTTPS domain (including // dedicated-deployment download hosts) passes URL validation, while IP - // literals, userinfo URLs, non-default ports, and plain HTTP stay rejected. - // SSRF protection is enforced at dial time by localio.SecureHTTPClient. + // 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. 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, "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, - "https://download.dingtalk.com:8443/file": false, } { if _, err := validateResourceDownloadURL(rawURL); (err == nil) != wantOK { t.Errorf("validateResourceDownloadURL(%q) error = %v, want ok=%v", rawURL, err, wantOK) diff --git a/internal/shortcut/chat/resource_download_test.go b/internal/shortcut/chat/resource_download_test.go index c34d80d0..c3eb44d6 100644 --- a/internal/shortcut/chat/resource_download_test.go +++ b/internal/shortcut/chat/resource_download_test.go @@ -83,7 +83,7 @@ func TestCrossPlatformCoverageResourceDownloadInfo(t *testing.T) { for _, resourceURL := range []string{ "https://127.0.0.1/file", "https://user:secret@download.dingtalk.com/file", - "https://download.dingtalk.com:8443/file", + "http://download.dingtalk.com:8443/file", } { if _, _, err := resourceDownloadInfo( map[string]any{"resourceUrl": resourceURL}, @@ -94,9 +94,12 @@ func TestCrossPlatformCoverageResourceDownloadInfo(t *testing.T) { // 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). + // Non-default HTTPS ports are accepted too: dedicated storage domains + // legitimately serve on them and the dial-time policy is port-agnostic. for _, resourceURL := range []string{ "https://download.dingtalk.com/file", "https://ddoss.tenant.example.com/file", + "https://ddoss.tenant.example.com:8443/file", } { if _, _, err := resourceDownloadInfo( map[string]any{"resourceUrl": resourceURL},