Skip to content

fix(qqbot): allow RFC2544 benchmark range for token fetch (#88984)#89015

Merged
sliverp merged 2 commits into
mainfrom
fix/issue-88984-qqbot-token-ssrf-rfc2544
Jun 2, 2026
Merged

fix(qqbot): allow RFC2544 benchmark range for token fetch (#88984)#89015
sliverp merged 2 commits into
mainfrom
fix/issue-88984-qqbot-token-ssrf-rfc2544

Conversation

@sliverp

@sliverp sliverp commented Jun 1, 2026

Copy link
Copy Markdown
Member

Summary

QQ Bot bots.qq.com token-fetch path was failing for users whose DNS resolver maps the hostname into the RFC 2544 benchmark range 198.18.0.0/15. This is the default behavior of fake-IP proxy stacks (sing-box, Clash, Surge) and some WSL2 DNS configurations. The default SSRF guard treats 198.18.0.0/15 as private/special-use and blocks the request, surfacing as:

Network error getting access_token: Blocked: resolves to private/internal/special-use IP address

The QQ Bot channel cannot come up at all in such environments.

Fix

Pass a host-scoped SsrFPolicy (allowRfc2544BenchmarkRange: true) to the single hard-coded TOKEN_URL request in TokenManager.doFetchToken(). Because TOKEN_URL is a const (https://bots.qq.com/app/getAppAccessToken) and not user-controlled, the relaxation cannot widen the attack surface to any other host.

This mirrors the existing QQBOT_MEDIA_SSRF_POLICY pattern used by extensions/qqbot/src/engine/utils/file-utils.ts, which already addresses the same fake-IP scenario for the media-download path. The change is intentionally narrow / host-scoped — the global SSRF default stays strict.

Test plan

  • pnpm exec vitest run extensions/qqbot/src/engine/api/token.test.ts — 5 / 5 passed
  • pnpm check — typecheck + oxlint (core/extensions/scripts) + policy guards all clean

Updated the existing equality assertion in token.test.ts to include the new policy field, and added a dedicated regression test that asserts policy: { allowRfc2544BenchmarkRange: true } is forwarded into fetchWithSsrFGuard.

Risk / rollback

  • Single-file behavior change limited to the QQ Bot token endpoint.
  • No new network paths introduced; URL constant is immutable.
  • Trivially reverted (single commit, single logical change).

Fixes #88984

@clawsweeper

clawsweeper Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 1, 2026, 10:58 PM ET / 02:58 UTC.

Summary
The PR adds a bots.qq.com-scoped RFC2544 SSRF allowance to QQ Bot token fetching and updates token manager tests to assert the policy is forwarded.

PR surface: Source +19, Tests +23. Total +42 across 2 files.

Reproducibility: yes. from source, not from a live QQ credential run: current main calls fetchWithSsrFGuard for the hard-coded token URL without a policy, and the SSRF contract blocks 198.18.0.0/15 unless allowRfc2544BenchmarkRange is true. The linked issue supplies the observed 198.18.0.70 DNS/error log.

Review metrics: 1 noteworthy metric.

  • SSRF Policy Exceptions: 1 added. The PR adds one host-scoped exception to a credential-bearing network request, which is the maintainer-visible decision before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Record maintainer/security acceptance of the bots.qq.com-scoped RFC2544 SSRF exception.
  • [P2] Optionally add redacted fake-IP token-fetch proof if maintainers want runtime confirmation before merge.

Risk before merge

  • [P1] Merging intentionally permits RFC2544 benchmark-range DNS answers for a credential-bearing QQ Bot token request; the hostname allowlist limits the exception to bots.qq.com, but it is still a security-boundary decision.
  • [P1] The PR body reports unit/check proof, not a redacted real fake-IP token-fetch run; this is acceptable for a member PR if maintainers choose to rely on source and tests, but it leaves runtime confirmation to maintainer judgment.

Maintainer options:

  1. Accept The Host-Scoped Exception (recommended)
    A maintainer/security owner can explicitly accept that bots.qq.com token fetches may use RFC2544 fake-IP DNS answers while all other hosts remain under the strict default policy.
  2. Require Runtime Fake-IP Proof First
    Maintainers can ask for a redacted run showing the token fetch succeeds when bots.qq.com resolves into 198.18.0.0/15 before accepting the exception.
  3. Pause If The Boundary Is Not Acceptable
    If OpenClaw should not allow RFC2544 answers for credential-bearing plugin token requests, pause or close this PR and keep the default fail-closed behavior.

Next step before merge

  • [P2] The remaining action is maintainer/security acceptance of an intentional SSRF exception; there is no narrow automated repair blocker in the current patch.

Security
Needs attention: The diff is narrowly scoped, but it deliberately relaxes an SSRF security boundary and needs maintainer/security acceptance before merge.

Review details

Best possible solution:

Land the narrow host-scoped exception only after a maintainer/security owner accepts the SSRF tradeoff; otherwise keep the default strict token-fetch policy.

Do we have a high-confidence way to reproduce the issue?

Yes from source, not from a live QQ credential run: current main calls fetchWithSsrFGuard for the hard-coded token URL without a policy, and the SSRF contract blocks 198.18.0.0/15 unless allowRfc2544BenchmarkRange is true. The linked issue supplies the observed 198.18.0.70 DNS/error log.

Is this the best way to solve the issue?

Yes, if maintainers accept the security tradeoff: adding a hostnameAllowlist-limited policy at the hard-coded token fetch is narrower than a global default or config change and mirrors the existing QQ Bot media fake-IP pattern.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 9d9a6140a3e4.

Label changes

Label changes:

  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The author association is MEMBER, so the external contributor proof gate does not apply; the PR body reports focused test and check commands but not live fake-IP token-fetch proof.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: ⏳ waiting on author.

Label justifications:

  • P1: The linked bug prevents QQ Bot from obtaining access tokens in fake-IP proxy or WSL2 DNS environments, breaking a real channel workflow.
  • merge-risk: 🚨 security-boundary: The diff relaxes the SSRF resolved-address block for bots.qq.com token fetching, so green tests do not settle the security-boundary acceptance decision.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The author association is MEMBER, so the external contributor proof gate does not apply; the PR body reports focused test and check commands but not live fake-IP token-fetch proof.
Evidence reviewed

PR surface:

Source +19, Tests +23. Total +42 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 20 1 +19
Tests 1 23 0 +23
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 43 1 +42

Security concerns:

  • [medium] Approve the host-scoped RFC2544 token exception — extensions/qqbot/src/engine/api/token.ts:35
    The patch permits RFC2544 resolved IPs for the QQ Bot token endpoint; hostnameAllowlist ["bots.qq.com"] and the hard-coded TOKEN_URL keep the blast radius narrow, but this is still a credential-bearing network request.
    Confidence: 0.86

What I checked:

  • Current main still blocks the reported token path: On current main, TokenManager.doFetchToken calls fetchWithSsrFGuard for https://bots.qq.com/app/getAppAccessToken with auditContext qqbot-token and capture false, but no policy is passed, so the default SSRF policy applies. (extensions/qqbot/src/engine/api/token.ts:233, 9d9a6140a3e4)
  • PR head scopes the exception to bots.qq.com: The PR head defines QQBOT_TOKEN_SSRF_POLICY with hostnameAllowlist ["bots.qq.com"] and allowRfc2544BenchmarkRange true, then passes that policy to the single hard-coded token URL fetch. (extensions/qqbot/src/engine/api/token.ts:35, 8c5c3e3dc5d4)
  • SSRF dependency contract supports this exact opt-in shape: The network policy blocks RFC2544 IPv4 answers by default and only exempts them when allowRfc2544BenchmarkRange is true; hostnameAllowlist is evaluated before DNS and non-matching hosts are rejected. (src/infra/net/ssrf.ts:380, 9d9a6140a3e4)
  • Existing QQ Bot media path already uses the same fake-IP pattern: QQBOT_MEDIA_SSRF_POLICY already combines a QQ media hostname allowlist with allowRfc2544BenchmarkRange true, and downloadFile forwards that policy through the platform adapter. (extensions/qqbot/src/engine/utils/file-utils.ts:66, 9d9a6140a3e4)
  • Regression coverage is present in the PR branch: The PR branch updates the existing fetch assertion and adds a regression test requiring the token fetch to forward hostnameAllowlist ["bots.qq.com"] plus allowRfc2544BenchmarkRange true. (extensions/qqbot/src/engine/api/token.test.ts:41, 8c5c3e3dc5d4)
  • History and ownership signal: Current shallow history/blame attributes the token manager and QQ Bot media policy files to recent work by Vincent Koc and the latest-release baseline commit by Peter Steinberger; this is useful routing signal, not blame. (extensions/qqbot/src/engine/api/token.ts:19, af44fb9b6cdd)

Likely related people:

  • Vincent Koc: Current blame and follow-log for the QQ Bot token manager attribute the current main implementation to commit af44fb9, and the live timeline also shows Vincent was mentioned on this PR. (role: recent area contributor; confidence: medium; commits: af44fb9b6cdd; files: extensions/qqbot/src/engine/api/token.ts, extensions/qqbot/src/engine/api/token.test.ts)
  • Peter Steinberger: The latest release baseline commit e932160 added the QQ Bot token and media utility files in this checkout history, making Peter a useful release/provenance routing candidate. (role: shipped baseline contributor; confidence: medium; commits: e93216080aa1; files: extensions/qqbot/src/engine/api/token.ts, extensions/qqbot/src/engine/utils/file-utils.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels Jun 1, 2026
@sliverp sliverp assigned steipete and unassigned steipete Jun 1, 2026
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 1, 2026
sliverp added 2 commits June 2, 2026 10:51
QQ Bot `bots.qq.com` token-fetch path was failing for users whose DNS resolver maps the hostname into the RFC 2544 benchmark range `198.18.0.0/15` (commonly seen with fake-IP proxy stacks: sing-box, Clash, Surge, WSL2 DNS). The default SSRF guard treats that range as private and blocks the request, surfacing as "Network error getting access_token: Blocked: resolves to private/internal/special-use IP address".

Pass a host-scoped `SsrFPolicy` (`allowRfc2544BenchmarkRange: true`) to the single hard-coded `TOKEN_URL` request, mirroring the existing `QQBOT_MEDIA_SSRF_POLICY` pattern used by the media path. Because `TOKEN_URL` is a const and not user-controlled, the relaxation cannot widen attack surface to other hosts.

Adds a regression test asserting `policy: { allowRfc2544BenchmarkRange: true }` is forwarded into `fetchWithSsrFGuard`, and updates the existing equality assertion accordingly.

Fixes #88984
@sliverp sliverp force-pushed the fix/issue-88984-qqbot-token-ssrf-rfc2544 branch from f337c29 to 8c5c3e3 Compare June 2, 2026 02:51
@clawsweeper clawsweeper Bot added status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 2, 2026
@sliverp sliverp merged commit 0552ec8 into main Jun 2, 2026
163 of 165 checks passed
@sliverp sliverp deleted the fix/issue-88984-qqbot-token-ssrf-rfc2544 branch June 2, 2026 07:00
@sliverp

sliverp commented Jun 2, 2026

Copy link
Copy Markdown
Member Author

Landed via temp rebase onto main.

  • Gate: pnpm lint && pnpm build && pnpm test
  • Land commit: 8c5c3e3
  • Merge commit: 0552ec8

Thanks @sliverp!

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jun 3, 2026
…8984) (openclaw#89015)

* fix(qqbot): allow RFC2544 benchmark range for token fetch (openclaw#88984)

QQ Bot `bots.qq.com` token-fetch path was failing for users whose DNS resolver maps the hostname into the RFC 2544 benchmark range `198.18.0.0/15` (commonly seen with fake-IP proxy stacks: sing-box, Clash, Surge, WSL2 DNS). The default SSRF guard treats that range as private and blocks the request, surfacing as "Network error getting access_token: Blocked: resolves to private/internal/special-use IP address".

Pass a host-scoped `SsrFPolicy` (`allowRfc2544BenchmarkRange: true`) to the single hard-coded `TOKEN_URL` request, mirroring the existing `QQBOT_MEDIA_SSRF_POLICY` pattern used by the media path. Because `TOKEN_URL` is a const and not user-controlled, the relaxation cannot widen attack surface to other hosts.

Adds a regression test asserting `policy: { allowRfc2544BenchmarkRange: true }` is forwarded into `fetchWithSsrFGuard`, and updates the existing equality assertion accordingly.

Fixes openclaw#88984

* fix(qqbot): scope token ssrf policy
SYU8384 pushed a commit to SYU8384/openclaw that referenced this pull request Jun 3, 2026
…8984) (openclaw#89015)

* fix(qqbot): allow RFC2544 benchmark range for token fetch (openclaw#88984)

QQ Bot `bots.qq.com` token-fetch path was failing for users whose DNS resolver maps the hostname into the RFC 2544 benchmark range `198.18.0.0/15` (commonly seen with fake-IP proxy stacks: sing-box, Clash, Surge, WSL2 DNS). The default SSRF guard treats that range as private and blocks the request, surfacing as "Network error getting access_token: Blocked: resolves to private/internal/special-use IP address".

Pass a host-scoped `SsrFPolicy` (`allowRfc2544BenchmarkRange: true`) to the single hard-coded `TOKEN_URL` request, mirroring the existing `QQBOT_MEDIA_SSRF_POLICY` pattern used by the media path. Because `TOKEN_URL` is a const and not user-controlled, the relaxation cannot widen attack surface to other hosts.

Adds a regression test asserting `policy: { allowRfc2544BenchmarkRange: true }` is forwarded into `fetchWithSsrFGuard`, and updates the existing equality assertion accordingly.

Fixes openclaw#88984

* fix(qqbot): scope token ssrf policy
sablehead pushed a commit to sablehead/openclaw that referenced this pull request Jun 10, 2026
…8984) (openclaw#89015)

* fix(qqbot): allow RFC2544 benchmark range for token fetch (openclaw#88984)

QQ Bot `bots.qq.com` token-fetch path was failing for users whose DNS resolver maps the hostname into the RFC 2544 benchmark range `198.18.0.0/15` (commonly seen with fake-IP proxy stacks: sing-box, Clash, Surge, WSL2 DNS). The default SSRF guard treats that range as private and blocks the request, surfacing as "Network error getting access_token: Blocked: resolves to private/internal/special-use IP address".

Pass a host-scoped `SsrFPolicy` (`allowRfc2544BenchmarkRange: true`) to the single hard-coded `TOKEN_URL` request, mirroring the existing `QQBOT_MEDIA_SSRF_POLICY` pattern used by the media path. Because `TOKEN_URL` is a const and not user-controlled, the relaxation cannot widen attack surface to other hosts.

Adds a regression test asserting `policy: { allowRfc2544BenchmarkRange: true }` is forwarded into `fetchWithSsrFGuard`, and updates the existing equality assertion accordingly.

Fixes openclaw#88984

* fix(qqbot): scope token ssrf policy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

channel: qqbot maintainer Maintainer-authored PR merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XS status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] QQ bot token fetch still blocked by SSRF — RFC2544 benchmark range not allowed

2 participants