fix(slack): remove unused unsafe auth fetch helper by miorbnli · Pull Request #98495 · openclaw/openclaw

miorbnli · 2026-07-01T08:16:12Z

What Problem This Solves

fetchWithSlackAuth duplicated Slack redirect policy in extensions/slack/src/monitor/media.ts, contained an unsafe automatic-redirect path, and had no production callers. It was also not exported through the Slack plugin API, so retaining and expanding it would create a second network policy implementation without protecting any runtime behavior.

Why This Change Was Made

The maintainer rewrite removes the unused helper and its helper-only tests instead of adding another redirect loop. Active Slack downloads already use createSlackMediaRequest + createSlackMediaFetch + saveRemoteMedia, which delegates redirect handling, DNS pinning, authorization stripping, and SSRF policy enforcement to the shared guarded-fetch path.

This keeps one canonical implementation and removes the unsafe future footgun entirely.

User Impact

No current runtime behavior changes: the removed helper had no production caller and was not part of extensions/slack/api.ts. Slack private-file and forwarded-image downloads continue through the existing guarded media path.

Evidence

Repository-wide caller search: no fetchWithSlackAuth references remain; before removal, its dedicated tests were the only callers.
Public-surface check: extensions/slack/api.ts does not export the helper.
Active-path review: resolveSlackMedia and forwarded attachment downloads continue through saveRemoteMedia with Slack hostname policy and manual redirect handling.
Blacksmith Testbox tbx_01kwpdqh65849veg0j35wzcjmx (Actions run):
- extensions/slack/src/monitor/media.test.ts: 35/35 passed.
- pnpm check:changed: extension production/test typechecks, lint, guards, and import-cycle checks passed.
Fresh Codex autoreview: no accepted/actionable findings; 0.99 confidence.
Final diff: 2 files, 219 deletions, 0 additions.

Risk checklist

User-visible behavior: No; dead private helper only.
Config/env/migration behavior: No.
Security/auth/secrets/network/tool execution: Improvement; removes an unused unsafe redirect implementation.
Plugins/providers/channels/SDK: No public API change.
Changelog: Not required for an internal dead-code removal.

Closes: N/A (no existing issue)

clawsweeper · 2026-07-01T08:55:06Z

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

This PR is useful and the final deletion shape looks correct, but it is no longer the best landing candidate because an open current-main-based replacement carries the same Slack helper/test removal, preserves contributor credit, is mergeable, and has passing proof checks.

Root-cause cluster
Relationship: superseded
Canonical: #99944
Summary: The current PR is superseded by an open maintainer-labeled replacement PR that carries the same end-state helper/test deletion on a current-main branch and preserves contributor credit.

Members:

canonical: fix(slack): remove unused unsafe auth fetch helper #99944 - Open, mergeable current-main replacement with the same cleanup diff, passing checks, and co-author credit for the contributor.
same_root_cause: [Security] Slack Media Fetch: SSRF via Unvalidated Redirect Following #13145 - Closed security report described the same unused helper redirect SSRF concern, but it is not a live canonical landing path.
adjacent_distinct: fix(slack): preserve auth on same-origin media redirects #62996 - Merged same-origin Slack media redirect auth preservation for the active media path; related area but not the same dead-helper cleanup.
partial_overlap: fix(security): cap Slack media downloads and validate Slack file URLs #6639 - Earlier merged Slack media URL validation and download cap hardening overlaps security context but not the same remaining cleanup.

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Canonical path: Close this source PR and use #99944 as the canonical current-main landing path for the same cleanup, with contributor credit preserved there.

So I’m closing this here and keeping the remaining discussion on #99944.

Review details

Best possible solution:

Close this source PR and use #99944 as the canonical current-main landing path for the same cleanup, with contributor credit preserved there.

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

Yes. Source inspection shows current main's private helper validates the first Slack URL, then follows redirected HTTPS URLs with redirect: "follow"; caller search shows only helper tests use it.

Is this the best way to solve the issue?

Yes for the end state: deleting the private unused helper is narrower than maintaining a second redirect loop, and the same end state is now better handled by the current-main replacement PR.

Security review:

Security review cleared: The diff removes an unused token-bearing network helper and helper-only tests, with no dependency, workflow, package, or secret-handling expansion.

AGENTS.md: found and applied where relevant.

What I checked:

Root repository policy read: Read the full root policy and applied the OpenClaw PR review requirements for source, scoped policy, callers, tests, replacement PR viability, and history. (AGENTS.md:1, d6ed2c392c1e)
Scoped extension policy read: The extension policy treats bundled plugin internals as private unless promoted through api.ts, which matters because this helper is not a public Slack plugin API surface. (extensions/AGENTS.md:1, d6ed2c392c1e)
Current helper source: Current main still has fetchWithSlackAuth; after one manual redirect it delegates to redirect: "follow", which is the unused unsafe path the PR removes. (extensions/slack/src/monitor/media.ts:117, d6ed2c392c1e)
Active Slack media path: Production Slack file and forwarded-image downloads go through createSlackMediaRequest, createSlackMediaFetch, saveSlackMedia, and saveRemoteMedia with SLACK_MEDIA_SSRF_POLICY, not through the removed helper. (extensions/slack/src/monitor/media.ts:322, d6ed2c392c1e)
No public Slack API export: extensions/slack/api.ts exports Slack plugin APIs and action helpers but does not export fetchWithSlackAuth, supporting the private-helper cleanup boundary. (extensions/slack/api.ts:1, d6ed2c392c1e)
Caller search: Repository search found fetchWithSlackAuth only in the helper definition and helper-only tests on current main; production call sites use resolveSlackMedia instead. (extensions/slack/src/monitor/media.ts:117, d6ed2c392c1e)

Likely related people:

steipete: Authored the current-main replacement commit, appears in history for the original Slack file redirect helper and the shared SSRF guarded-fetch path. (role: canonical replacement author and adjacent fetch-guard contributor; confidence: high; commits: 33f897bda465, 287ab8406033, 81c68f582d4a; files: extensions/slack/src/monitor/media.ts, extensions/slack/src/monitor/media.test.ts, src/media/fetch.ts)
vincentkoc: Merged the nearby same-origin Slack media redirect auth preservation fix, which shaped the current media helper and tests. (role: recent Slack media redirect contributor; confidence: high; commits: 55cbcd829d38; files: extensions/slack/src/monitor/media.ts, extensions/slack/src/monitor/media.test.ts)
scoootscooob: Moved Slack channel code into extensions/slack, which is important for following current file history and owner boundary. (role: file-move and extension-boundary contributor; confidence: medium; commits: 8746362f5ebf; files: extensions/slack/src/monitor/media.ts, src/slack/monitor/media.ts)
davidiach: Authored the earlier merged Slack media URL validation and download-cap hardening that is adjacent to this helper cleanup. (role: adjacent Slack media security contributor; confidence: medium; commits: 4e4ed2ea1793; files: src/slack/monitor/media.ts)

Codex review notes: model internal, reasoning high; reviewed against d6ed2c392c1e.

miorbnli · 2026-07-02T01:26:19Z

@clawsweeper re-review

Updated the Real behavior proof section with non-mocked evidence: a real local HTTPS server + production undici dispatcher (globalThis.fetch is not replaced), with the server logging each hop it actually receives.

clawsweeper · 2026-07-02T01:26:22Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

…t (SSRF) fetchWithSlackAuth only validated the initial URL via assertSlackFileUrl but then handed redirect handling to fetch redirect-follow, which follows up to 20 hops to any HTTPS URL. A redirect from a Slack host to an arbitrary HTTPS endpoint (internal service, attacker host) bypassed the SSRF guard entirely. Rewrite the redirect handling as a manual-redirect loop that re-validates every hop against isSlackHostname (Slack allowlist) and requires HTTPS, throws on a non-Slack or non-HTTPS target, and strips Authorization on cross-origin hops. Bounded to MAX_REDIRECTS=5. Non-redirect (2xx/4xx/5xx) and missing/malformed Location responses are returned unchanged, preserving the existing contract. Add SSRF regression tests: redirect to a non-Slack host throws, redirect to non-HTTPS throws. Update existing redirect tests to expect manual redirect on the second hop (the loop no longer hands off to follow). Co-Authored-By: Claude <[email protected]>

steipete · 2026-07-04T11:40:22Z

Land-ready proof for head c593263aaa747bc6bf05529c54a8421645388b4c.

Maintainer work:

Re-reviewed the full Slack media module, active callers, shared guarded-fetch callee, sibling redirect path, plugin API exports, tests, shipped history, and Undici redirect contract.
Rewrote the PR from a second redirect implementation (+57 net lines) to removal of the unused private helper and helper-only tests (219 deletions, 0 additions).
Updated the PR title/body to match the final implementation.

Verification:

Blacksmith Testbox tbx_01kwpdqh65849veg0j35wzcjmx (Actions run 28704570262):
- corepack pnpm test extensions/slack/src/monitor/media.test.ts — 35/35 passed.
- env OPENCLAW_CHECK_CHANGED_REMOTE_CHILD=1 OPENCLAW_CHANGED_LANES_RAW_SYNC=1 corepack pnpm check:changed — extension prod/test typechecks, lint, guards, and import-cycle checks passed.
Hosted exact-head CI run 28704833527 — 105 checks completed, zero failures.
.agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main --stream-engine-output — no accepted/actionable findings, correctness confidence 0.99.
scripts/pr review-validate-artifacts 98495 and OPENCLAW_TESTBOX=1 scripts/pr prepare-run 98495 passed.

Known proof gap: no live Slack workspace run. The removed helper had no production callers and was not exported through extensions/slack/api.ts; active Slack downloads are unchanged and remain covered by the retained media tests and shared guarded-fetch checks.

clawsweeper · 2026-07-04T11:57:37Z

🦞✅
ClawSweeper autoclose is complete.

Reason: structured ClawSweeper close marker: close-required (sha=c593263aaa747bc6bf05529c54a8421645388b4c)

Closed:

Closed fix(slack): remove unused unsafe auth fetch helper #98495: fix(slack): remove unused unsafe auth fetch helper

steipete · 2026-07-04T12:22:08Z

Landed the reviewed fix as #99944 in commit 706443c: 706443c.

The original redirect gap was real. Full review showed fetchWithSlackAuth had no production callers and was not public API, so the optimal fix was to delete the unsafe helper and its helper-only tests rather than maintain a second redirect stack. Your original report and real-network proof were valuable, and contributor credit is preserved via Co-authored-by: liyuanbin <[email protected]> in the landed commit.

We first updated this fork branch, but GitHub's maintainer-edit API can append a verified commit without rewriting branch ancestry. The repository merge guard required a current-main lineage, so #99944 recreated the same reviewed two-file tree as one current-main maintainer commit.

Thank you @miorbnli for finding and proving the issue. Closing this PR as superseded by the landed canonical fix.

openclaw-barnacle Bot added channel: slack Channel integration: slack size: S labels Jul 1, 2026

miorbnli closed this Jul 1, 2026

miorbnli reopened this Jul 1, 2026

miorbnli closed this Jul 1, 2026

miorbnli reopened this Jul 1, 2026

miorbnli force-pushed the fix/slack-fetch-auth-ssrf-redirect-guard branch from 94316d8 to 38b2182 Compare July 2, 2026 00:44

miorbnli force-pushed the fix/slack-fetch-auth-ssrf-redirect-guard branch from 38b2182 to a0eb4de Compare July 3, 2026 00:49

miorbnli closed this Jul 3, 2026

miorbnli reopened this Jul 3, 2026

steipete self-assigned this Jul 4, 2026

refactor(slack): remove unused auth fetch helper

c593263

openclaw-barnacle Bot added size: M and removed size: S labels Jul 4, 2026

steipete changed the title ~~fix(slack): validate redirect targets against Slack hostname allowlist (SSRF)~~ fix(slack): remove unused unsafe auth fetch helper Jul 4, 2026

steipete mentioned this pull request Jul 4, 2026

fix(slack): remove unused unsafe auth fetch helper #99944

Merged

clawsweeper Bot closed this Jul 4, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(slack): remove unused unsafe auth fetch helper#98495

fix(slack): remove unused unsafe auth fetch helper#98495
miorbnli wants to merge 2 commits into
openclaw:mainfrom
miorbnli:fix/slack-fetch-auth-ssrf-redirect-guard

miorbnli commented Jul 1, 2026 •

edited by steipete

Loading

Uh oh!

clawsweeper Bot commented Jul 1, 2026 •

edited

Loading

Uh oh!

miorbnli commented Jul 2, 2026

Uh oh!

clawsweeper Bot commented Jul 2, 2026 •

edited

Loading

Uh oh!

steipete commented Jul 4, 2026

Uh oh!

clawsweeper Bot commented Jul 4, 2026

Uh oh!

steipete commented Jul 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

miorbnli commented Jul 1, 2026 • edited by steipete Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Problem This Solves

Why This Change Was Made

User Impact

Evidence

Risk checklist

Uh oh!

clawsweeper Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miorbnli commented Jul 2, 2026

Uh oh!

clawsweeper Bot commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steipete commented Jul 4, 2026

Uh oh!

clawsweeper Bot commented Jul 4, 2026

Uh oh!

steipete commented Jul 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

miorbnli commented Jul 1, 2026 •

edited by steipete

Loading

clawsweeper Bot commented Jul 1, 2026 •

edited

Loading

clawsweeper Bot commented Jul 2, 2026 •

edited

Loading