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
This commit is contained in:
liyuan
@@ -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
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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},
|
||||
|
||||
Reference in New Issue
Block a user