Skip to content

fix(s3): only skip direct redirect for true sub-resource queries#2604

Merged
jyxjjj merged 2 commits into
OpenListTeam:mainfrom
syscc:fix/s3-direct-redirect-query-filter
Jun 12, 2026
Merged

fix(s3): only skip direct redirect for true sub-resource queries#2604
jyxjjj merged 2 commits into
OpenListTeam:mainfrom
syscc:fix/s3-direct-redirect-query-filter

Conversation

@syscc

@syscc syscc commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Summary / 摘要

Follow-up fix for #2598 (feat(s3): support direct transfer redirects).

After #2598 shipped, S3 GET Object requests that carried standard
response-* override parameters (most commonly response-content-disposition,
which clients add to set the download filename) were incorrectly forced through
the gofakes3 server-side streaming path instead of being redirected to the
upstream direct link.

Root cause: hasNonObjectQuery used an allow-list — it let through only the
SigV4 signing parameters (x-amz-*, awsaccesskeyid, signature, expires,
x-id) and treated every other query parameter as a "non-object" request,
disabling the redirect. Any client that appended response-content-disposition
(or any other unanticipated parameter) silently lost direct-download.

This affects every direct-link-capable driver, not a specific one: the
allow-list runs before storage/driver selection, so the regression is generic.

Changes

  • Reverse hasNonObjectQuery from an allow-list to a block-list: only the
    query keys that make gofakes3 route a request to a sub-resource handler
    (uploadId, uploads, versioning, versions, location, and a non-null
    versionId) disable the direct redirect. This list is derived directly from
    gofakes3 routing.go routeBase/routeBucket, so response-* and all other
    download-irrelevant parameters now correctly allow a 302.
  • Align the download redirect with the native handles.redirect path by setting
    Referrer-Policy: no-referrer (avoid leaking the signed upstream URL via
    Referer) and Cache-Control: max-age=0, no-cache, no-store, must-revalidate
    (avoid caching short-lived signed links). Also set Referrer-Policy on the
    upload redirect branch for consistency.
  • Use the shared utils.ClientIP(r) (X-Forwarded-For / X-Real-Ip aware) instead
    of a local RemoteAddr-only helper, so drivers that bind direct links to the
    client IP receive the real client IP when running behind a reverse proxy.

No public API, config, or storage format changes. Upload-path logic from #2598
is untouched.

  • This PR has breaking changes.
    / 此 PR 包含破坏性变更。
  • This PR changes public API, config, storage format, or migration behavior.
    / 此 PR 修改了公开 API、配置、存储格式或迁移行为。
  • This PR requires corresponding changes in related repositories.
    / 此 PR 需要关联仓库同步修改。

Related repository PRs / 关联仓库 PR:

  • OpenList-Frontend:
  • OpenList-Docs:

Related Issues / 关联 Issue

Relates to #2598

Testing / 测试

  • go build ./...
  • gofmt clean on changed file
  • Manual test / 手动测试:

Built from source with S3 enabled on port 5246 behind an openresty reverse
proxy, and exercised three direct-link-capable drivers via signed SigV4
presigned GET requests. Each was tested both with and without
response-content-disposition.

Bucket Driver Cloud GET (plain) GET (with response-content-disposition)
cu 139Yun China Mobile 302 direct 302 direct
ct 189CloudPC China Telecom 302 direct 302 direct
cm WoPan China Unicom 302 direct 302 direct

Before the fix, the response-content-disposition variant returned 304/200
(server-side proxy); after the fix it returns 302 to the upstream direct link.

Verified the new response headers are present on every 302:

HTTP/1.1 302 Found
Cache-Control: max-age=0, no-cache, no-store, must-revalidate
Referrer-Policy: no-referrer
Location: https://<upstream-direct-link>...

Verified X-Forwarded-For handling: requests carrying
X-Forwarded-For: 1.2.3.4 still redirect correctly, confirming the client-IP
change does not break the link flow.

Verified the block-list against gofakes3 routing.go: sub-resource requests
(?uploads, ?uploadId=, ?versioning, ?versions, ?location,
?versionId=) still fall back to the gofakes3 server-side path; plain object
downloads (including ?x-id=GetObject and ?response-content-disposition=...)
are redirected.

Checklist / 检查清单

  • I have read CONTRIBUTING.
    / 我已阅读 CONTRIBUTING
  • I confirm this contribution follows the repository license, contribution policy, and code of conduct.
    / 我确认此贡献符合仓库许可证、贡献规范和行为准则。
  • I have formatted the changed code with gofmt, go fmt, or prettier where applicable.
    / 我已按适用情况使用 gofmtgo fmtprettier 格式化变更代码。
  • I have requested review from relevant maintainers or code owners where applicable.
    / 我已在适用情况下请求相关维护者或代码所有者审查。

AI Disclosure / AI 使用声明

  • This PR includes AI-assisted content.
    / 此 PR 包含 AI 辅助内容。

Tools used / 使用工具:

  • ChatGPT
  • Codex
  • GitHub Copilot
  • Claude
  • Gemini
  • Other (please specify) / 其他(请注明):

Usage scope / 使用范围:

  • Code generation / 代码生成

  • Refactoring / 重构

  • Documentation / 文档

  • Tests / 测试

  • Translation / 翻译

  • Review assistance / 审查辅助

  • I have reviewed and validated all AI-assisted content included in this PR.
    / 我已审核并验证此 PR 中的所有 AI 辅助内容。

  • I have ensured that all AI-assisted commits include Co-Authored-By attribution.
    / 我已确保所有 AI 辅助提交都包含 Co-Authored-By 归属信息。

  • I can reproduce all AI-assisted content included in this PR without any AI tools.
    / 我可以在没有任何 AI 工具的情况下重现此 PR 中包含的所有 AI 辅助内容。

The direct-redirect path treated any non-allowlisted query parameter as a
sub-resource request and fell back to server-side proxying. S3 clients that
append `response-content-disposition` (and other `response-*` overrides) to a
presigned GET were therefore always proxied instead of redirected to the
upstream direct link.

Replace the allowlist with a blocklist of the query keys that actually route a
GET to a gofakes3 sub-resource handler (uploadId, uploads, versioning,
versions, location, versionId), so plain object downloads carrying response-*
overrides are redirected as 302 again.

Also align the redirect responses with the native download handler: set
Referrer-Policy: no-referrer on both the download and upload redirects to keep
signed upstream URLs out of the Referer header, add the no-store Cache-Control
to the download redirect, and resolve the client IP via utils.ClientIP so
X-Forwarded-For is honored behind a reverse proxy.

Relates to OpenListTeam#2598

Co-Authored-By: Claude <noreply@anthropic.com>
jyxjjj
jyxjjj previously approved these changes Jun 12, 2026
@jyxjjj

jyxjjj commented Jun 12, 2026

Copy link
Copy Markdown
Member

LGTM overall.

Before merge, I’d prefer adding a small table test for hasNonObjectQuery covering response-content-disposition, x-id=GetObject, uploads/uploadId/versioning/versions/location, and versionId null/non-null cases.

One thing to double-check: switching from RemoteAddr to utils.ClientIP means this path now trusts forwarded IP headers. If utils.ClientIP is not limited to trusted proxies, clients may spoof IP-bound direct-link generation in non-proxied deployments.

Add table cases for response-content-disposition/type, x-id, the
uploads/uploadId/versioning/versions/location sub-resources, and
versionId null/non-null/empty, per review on OpenListTeam#2604. Correct the stale
list-type expectation: with an object segment present gofakes3 routes
to getObject, so list-type must not block a direct redirect.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@syscc

syscc commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review!

1. Table test — Done in f002a6d. Extended TestHasNonObjectQuery to cover response-content-disposition, response-content-type, x-id=GetObject, all sub-resource keys (uploads/uploadId/versioning/versions/location), and versionId null/empty/non-null.

One note: I changed the existing list-type=2 case from want: true to want: false. In gofakes3's routeBase, list-type is not a routing key — a path with an object segment always dispatches to getObject, so it must not block a direct redirect. The old true was an artifact of the previous allowlist behavior this PR replaces.

2. IP trust — Good catch, and you're right about the mechanism. A couple of points for context:

  • This isn't new to this path: server/webdav/webdav.go:257 already uses utils.ClientIP(r) to generate direct links the same way.
  • The S3 server is mounted via gin.WrapH on a bare http.Handler, so c.ClientIP() isn't reachable here — utils.ClientIP(r) is the only option.
  • The repo never calls SetTrustedProxies anywhere, so gin already trusts X-Forwarded-For globally by default. The spoofing surface you describe exists project-wide regardless of this change, not just on this path.
  • For reverse-proxied deployments (the common case for the S3 endpoint), reading XFF is required to pass the real client IP to IP-bound drivers — RemoteAddr would hand them the proxy IP.

If you'd prefer a trusted-proxy gate, I think that's better addressed globally (a shared utils.ClientIP that respects a trusted-proxy config) rather than only on this path, so behavior stays consistent with the WebDAV redirect. Happy to open a follow-up for that if you agree.

@jyxjjj jyxjjj merged commit d7c332c into OpenListTeam:main Jun 12, 2026
8 checks passed
EnterTang pushed a commit to EnterTang/OpenList that referenced this pull request Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants