fix(security): MJ image IDOR (CVE-2026-9306) + HEIF DoS + quota trust-bypass + SSRF rebinding - #5406
fix(security): MJ image IDOR (CVE-2026-9306) + HEIF DoS + quota trust-bypass + SSRF rebinding#5406qzm wants to merge 1 commit into
Conversation
… bypass + SSRF rebinding hardening
WalkthroughThis PR adds coordinated security hardening across image access, network operations, and resource consumption. It introduces HMAC-signed Midjourney image requests, an SSRF-safe HTTP client with validated DNS handling, trusted proxy configuration, disabled trust quota bypass, and recursion depth limits for HEIF box parsing. ChangesSecurity Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
relay/mjproxy_handler.go (1)
57-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the SSRF-safe client on the direct
/mj/imagefetch path.This handler still does
ValidateURLWithFetchSetting(...)and then falls back toservice.GetHttpClient().Get(...)when no channel proxy is configured. That recreates the same DNS-rebinding window the new pinned-IP client was added to close, so the image proxy endpoint remains a bypass for the SSRF fix on its direct-fetch path.Suggested minimal fix
- if httpClient == nil { - httpClient = service.GetHttpClient() - } + if httpClient == nil { + httpClient = service.GetSSRFSafeClient() + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@relay/mjproxy_handler.go` around lines 57 - 67, The handler currently validates midjourneyTask.ImageUrl with ValidateURLWithFetchSetting but then falls back to calling service.GetHttpClient() and using httpClient.Get(...), which reintroduces a DNS-rebinding window; fix by ensuring the code uses the SSRF-safe/pinned-IP HTTP client instead of service.GetHttpClient() when httpClient == nil (i.e., replace the httpClient assignment to use the SSRF-safe/pinned-IP client exported by service and ensure the subsequent call to httpClient.Get(midjourneyTask.ImageUrl) uses that client); update the httpClient initialization site (where httpClient is set) to call the SSRF-safe client factory (the pinned-IP client) rather than service.GetHttpClient() so the validated URL is fetched via the safe client.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@common/quota.go`:
- Around line 4-8: GetTrustQuota() returning 0 doesn't actually disable the
bypass because service/pre_consume_quota.go still compares balances to that
numeric threshold; change the logic so the bypass is gated by the centralized
shouldTrust() check instead of raw numeric comparisons: in
service/pre_consume_quota.go replace the userQuota > trustQuota / tokenQuota >
trustQuota conditions with a call to shouldTrust(...) (or a boolean helper that
uses shouldTrust), and optionally make GetTrustQuota() return a clear sentinel
(e.g., -1) if you want a value-based disable; ensure the code paths that zero
out preConsumedQuota only run when shouldTrust() explicitly allows it.
In `@relay/mjproxy_handler.go`:
- Around line 483-485: Replace the unconditional log.Printf call in
mjproxy_handler.go that prints originTask.ChannelId and channel.GetBaseURL()
with the project's logger API (use the same logger used elsewhere in this
package) at an appropriate debug/trace level, and ensure sensitive fields are
redacted or omitted; specifically update the
log.Printf("检测到此操作为放大、变换、重绘，获取原channel信息: %s,%s",
strconv.Itoa(originTask.ChannelId), channel.GetBaseURL()) usage to call the
shared logger (e.g., logger.Debugf or logger.Tracef) or to log a non-sensitive
message referencing the code locations around originTask.ChannelId and
channel.GetBaseURL() so it honors repo log-level and redaction settings.
In `@service/http_client.go`:
- Around line 114-120: The Transport currently uses http.ProxyFromEnvironment
which causes DialContext (ssrfSafeDialContext) to validate the proxy host
instead of the final destination; replace Proxy: http.ProxyFromEnvironment with
a custom Proxy function that first inspects and validates the request's final
target (e.g., req.URL.Host or host from CONNECT for HTTPS) against your SSRF
allowlist using the same validation logic as ssrfSafeDialContext and only
returns the environment proxy when the final destination is permitted, otherwise
return nil (or block) so DialContext still applies to the ultimate target;
update the Transport creation (the struct using ssrfSafeDialContext and Proxy)
to use that wrapper Proxy function.
---
Outside diff comments:
In `@relay/mjproxy_handler.go`:
- Around line 57-67: The handler currently validates midjourneyTask.ImageUrl
with ValidateURLWithFetchSetting but then falls back to calling
service.GetHttpClient() and using httpClient.Get(...), which reintroduces a
DNS-rebinding window; fix by ensuring the code uses the SSRF-safe/pinned-IP HTTP
client instead of service.GetHttpClient() when httpClient == nil (i.e., replace
the httpClient assignment to use the SSRF-safe/pinned-IP client exported by
service and ensure the subsequent call to
httpClient.Get(midjourneyTask.ImageUrl) uses that client); update the httpClient
initialization site (where httpClient is set) to call the SSRF-safe client
factory (the pinned-IP client) rather than service.GetHttpClient() so the
validated URL is fetched via the safe client.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6098b164-605e-41bc-b160-e4dc6d4b9305
📒 Files selected for processing (8)
common/mjsign.gocommon/quota.gocontroller/midjourney.gomain.gorelay/mjproxy_handler.goservice/download.goservice/file_service.goservice/http_client.go
| // 安全加固：返回 0 以彻底关闭“信任额度旁路”。 | ||
| // 原值 10*QuotaPerUnit 使高余额/unlimited token 的请求在入口零预留、仅事后结算， | ||
| // 叠加无下限扣减 + 并发突发可超扣/白嫖（运营方买单）。 | ||
| // shouldTrust() 在 trustQuota<=0 时直接返回 false，所有请求改走正常预扣预留。 | ||
| return 0 |
There was a problem hiding this comment.
return 0 does not fully disable the trust-bypass path.
service/pre_consume_quota.go:31-64 still treats GetTrustQuota() as a numeric threshold. With 0 here, userQuota > trustQuota and tokenQuota > trustQuota become true for almost every positive balance, so that path can still zero out preConsumedQuota. This leaves the overspend bypass reachable and can widen it beyond the old 10*QuotaPerUnit cutoff.
Suggested follow-up
// service/pre_consume_quota.go
trustQuota := common.GetTrustQuota()
relayInfo.UserQuota = userQuota
-if userQuota > trustQuota {
+if trustQuota > 0 && userQuota > trustQuota {
if !relayInfo.TokenUnlimited {
tokenQuota := c.GetInt("token_quota")
if tokenQuota > trustQuota {
preConsumedQuota = 0
}
} else {
preConsumedQuota = 0
}
}Or better, funnel this path through the same centralized shouldTrust() gate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@common/quota.go` around lines 4 - 8, GetTrustQuota() returning 0 doesn't
actually disable the bypass because service/pre_consume_quota.go still compares
balances to that numeric threshold; change the logic so the bypass is gated by
the centralized shouldTrust() check instead of raw numeric comparisons: in
service/pre_consume_quota.go replace the userQuota > trustQuota / tokenQuota >
trustQuota conditions with a call to shouldTrust(...) (or a boolean helper that
uses shouldTrust), and optionally make GetTrustQuota() return a clear sentinel
(e.g., -1) if you want a value-based disable; ensure the code paths that zero
out preConsumedQuota only run when shouldTrust() explicitly allows it.
| c.Request.Header.Set("Authorization", fmt.Sprintf("Bearer %s", channel.Key)) | ||
| logger.LogDebug(c, "Midjourney action uses origin channel: id=%s, base_url=%s", strconv.Itoa(originTask.ChannelId), channel.GetBaseURL()) | ||
| log.Printf("检测到此操作为放大、变换、重绘，获取原channel信息: %s,%s", strconv.Itoa(originTask.ChannelId), channel.GetBaseURL()) | ||
| } |
There was a problem hiding this comment.
Keep this diagnostic on the project logger.
log.Printf makes this message unconditional in production and bypasses the repo's log-level/redaction handling, so channel IDs and base URLs will now be emitted on every upscale/variation request.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@relay/mjproxy_handler.go` around lines 483 - 485, Replace the unconditional
log.Printf call in mjproxy_handler.go that prints originTask.ChannelId and
channel.GetBaseURL() with the project's logger API (use the same logger used
elsewhere in this package) at an appropriate debug/trace level, and ensure
sensitive fields are redacted or omitted; specifically update the
log.Printf("检测到此操作为放大、变换、重绘，获取原channel信息: %s,%s",
strconv.Itoa(originTask.ChannelId), channel.GetBaseURL()) usage to call the
shared logger (e.g., logger.Debugf or logger.Tracef) or to log a non-sensitive
message referencing the code locations around originTask.ChannelId and
channel.GetBaseURL() so it honors repo log-level and redaction settings.
| transport := &http.Transport{ | ||
| MaxIdleConns: common.RelayMaxIdleConns, | ||
| MaxIdleConnsPerHost: common.RelayMaxIdleConnsPerHost, | ||
| ForceAttemptHTTP2: true, | ||
| Proxy: http.ProxyFromEnvironment, | ||
| DialContext: ssrfSafeDialContext, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go's net/http package, when Transport.Proxy is set (for example via http.ProxyFromEnvironment), does Transport.DialContext connect to the proxy address instead of the final request host, so the proxy performs destination DNS resolution?
💡 Result:
Yes, when a Transport is configured with a proxy, the DialContext method is called to establish a connection to the proxy address, not the final destination host [1]. In Go's net/http package, the Transport determines the "first hop" address using the connectMethod structure [1]. When a proxy is in use, this address is the proxy's network address (host:port) [1]. Consequently, the Transport initiates the TCP connection—via your provided DialContext or the default dialer—directly to the proxy [1]. Because the TCP connection is established with the proxy, the proxy is responsible for handling the subsequent request [1]. Whether the proxy performs the destination DNS resolution depends on the type of proxy and the protocol used: 1. HTTP Proxy: For standard HTTP requests through a proxy, the Transport sends a request to the proxy that typically includes the full URL or the host header, and the proxy performs the DNS resolution to reach the final destination. 2. HTTPS Proxy (CONNECT): When using a proxy for HTTPS (via a CONNECT request), the Transport sends a CONNECT method to the proxy, specifying the destination host and port. The proxy then performs the DNS resolution and establishes the connection to that destination on your behalf. 3. SOCKS Proxy: If using a SOCKS5 proxy, the protocol explicitly allows for the client to send the hostname to the proxy, which then performs the DNS resolution at the proxy side. In all these cases, the DialContext hook is interacting only with the proxy's address [1]. The final destination hostname is not passed to the DialContext function when a proxy is configured, as the dialer's target is the proxy itself [1].
Citations:
ProxyFromEnvironment makes ssrfSafeDialContext validate the proxy “first hop”, not the final destination.
With http.Transport.Proxy set (e.g., http.ProxyFromEnvironment), Go’s Transport establishes the TCP connection via DialContext to the proxy host:port rather than the ultimate request host; the proxy then performs the connection/DNS to the destination later. This means the SSRF “pinned destination” protection provided by ssrfSafeDialContext may not apply to the final resolved target when an environment proxy is in play.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@service/http_client.go` around lines 114 - 120, The Transport currently uses
http.ProxyFromEnvironment which causes DialContext (ssrfSafeDialContext) to
validate the proxy host instead of the final destination; replace Proxy:
http.ProxyFromEnvironment with a custom Proxy function that first inspects and
validates the request's final target (e.g., req.URL.Host or host from CONNECT
for HTTPS) against your SSRF allowlist using the same validation logic as
ssrfSafeDialContext and only returns the environment proxy when the final
destination is permitted, otherwise return nil (or block) so DialContext still
applies to the ultimate target; update the Transport creation (the struct using
ssrfSafeDialContext and Proxy) to use that wrapper Proxy function.
移植自上游 PR QuantumNous#5406，只摘安全 hunk（PR 夹带的 IdleConnTimeout 删除、 LogDebug 退回 log.Printf 等回退未采纳）。四处加固： 1. Midjourney 转发图 IDOR (CVE-2026-9306)：/mj/image/:id 免鉴权且 对任意 mj_id 无作用域校验，可枚举越权读他人生成图。新增 common.SignMjImage(HMAC-Sha256, 复用 SessionSecret) 短签名， 三处 URL 拼接（GetAllMidjourney/GetUserMidjourney/coverTaskDto） 附 sig，handler 用 hmac.Equal 常量时间校验，缺失/不符即 403。 2. HEIF 解析 DoS：findISPE 对嵌套 ipco/iprp 链无界递归，构造深链 可耗尽栈。加 depth>16 上限，正常尺寸解析路径不受影响。 3. 信任额度旁路：GetTrustQuota 由 10*QuotaPerUnit 改 0，关闭入口 零预留旁路（并发突发可在结算前集体超支）。shouldTrust 已有 trustQuota<=0→false 短路，改后所有请求走正常预扣预留。 4. XFF 伪造：main.go 补 SetTrustedProxies，默认信任本机+私网段， 反代持公网 IP 时经 TRUSTED_PROXY_CIDRS 覆盖，防公网客户端伪造 X-Forwarded-For 绕过基于 ClientIP 的注册防刷/限流/支付审计。 SSRF 下载客户端一项本 fork 已有（download.go 已用 GetSSRFProtectedHTTPClient + 拨号时逐 IP 校验），无需移植。 补 HEIF 递归深度回归测试。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
51fdfc5 to
2b6f1df
Compare
2 participants
What
Four security fixes, all reachable in current releases (verified against
v1.0.0-rc.6, and the affected code is byte-identical inv1.0.0-rc.10andmain). Builds clean (go build ./...), no DB schema changes.SetTrustedProxies(XFF-spoofable IP rate limits)Fixes
1. Midjourney image IDOR (CVE-2026-9306)
router/relay-router.goregisterMjRouterGroupregistersGET /image/:idbefore.Use(TokenAuth(), Distribute()), so the route is unauthenticated;model.GetByOnlyMJIdlooks up bymj_idwith nouser_idscope. Any validmj_id(leaked via the default-on forward URL / Referer / logs) returns another user's image.The route is intentionally token-less (so
<img>/fetch clients load images without an API key), so addingTokenAuthwould break image display. Instead this PR signs the forwarded URL withHMAC(SessionSecret, mjId)(newcommon/mjsign.go) and verifies it inRelayMidjourneyImage. Legit clients receive a signed URL from the API and load it unchanged; forged/arbitrary ids get403.2. HEIF parser recursion DoS
service/file_service.gofindISPErecurses oniprp/ipcoboxes with no depth limit (each level consumes only the 8-byte header), so a nested-ipco.heicreaches recursion depth ≈len/8→ runtime stack-exhaustionthrow(uncatchable). Reachable by a normal user via a streaming multimodalimage_url. Fix: depth cap (≤16).3. Quota trust-bypass overspend
common/quota.goGetTrustQuota()hard-codes10*QuotaPerUnit;shouldTrust()then reserves 0 at request entry for high-balance/unlimited-token requests and charges only at settle. With a floor-less wallet decrement and no per-user lock, a concurrent burst drives the balance negative (operator eats the upstream cost). Fix:GetTrustQuota()returns0(disables the trust fast-path;shouldTrustalready returns false whentrustQuota<=0), so every request takes the normal pre-consume reservation path. (AWHERE quota >= ?decrement floor + atomic reservation are recommended follow-ups.)4. SSRF DNS-rebinding / TOCTOU
common/ssrf_protection.goValidateURLresolves & checks IPs but discards them; the default client re-resolves at dial time with no pinning → rebinding bypass. Fix: the user-controlled download path (service/download.go) now uses a client whoseDialContextresolves once, validates each IP against the SSRF policy, and dials a pinned validated IP. Scoped to downloads so upstream channel relay traffic is unaffected.+ SetTrustedProxies
main.goaddsserver.SetTrustedProxies(<loopback+private ranges>)soc.ClientIP()(used by all rate limiters) no longer trusts a client-suppliedX-Forwarded-For— closing IP-based rate-limit bypass / credential-stuffing amplification. Operators should also overwrite XFF at their reverse proxy.Notes
?sig=) will return403; clients re-fetch the task list to get freshly signed URLs.Summary by CodeRabbit
New Features
Bug Fixes