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

steipete · 2026-07-04T11:45:03Z

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 not exported through the Slack plugin API.

Supersedes #98495 after its fork branch could not be rebased through GitHub's maintainer-edit API. Contributor credit is preserved in the commit trailer.

Why This Change Was Made

The original PR added another manual redirect loop. Full review showed the safer, leaner fix is deletion: remove the unused helper and helper-only tests while keeping active Slack downloads on the canonical createSlackMediaRequest → createSlackMediaFetch → saveRemoteMedia / shared guarded-fetch path.

This branch is based on current main, avoiding the merge-critical drift that blocked #98495.

User Impact

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

Evidence

Final diff: 2 files, 219 deletions, 0 additions.
Byte-for-byte comparison: both touched files match the reviewed/Testbox-proven final tree from fix(slack): remove unused unsafe auth fetch helper #98495.
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.
Original rewritten head c593263aaa747bc6bf05529c54a8421645388b4c: hosted CI run 28704833527 completed 105 checks with zero failures.
Fresh current-main Codex autoreview: no accepted/actionable findings, correctness confidence 0.99.
git diff --check passed.

Known gap: no live Slack workspace run. The removed helper was unreachable; active Slack behavior is unchanged and covered by retained tests.

No changelog entry: internal dead-code removal; release automation owns CHANGELOG.md.

Co-authored-by: liyuanbin <[email protected]>

steipete · 2026-07-04T11:54:53Z

Land-ready proof for current-main replacement head 33f897bda46588cfdcec0a3551f0f18724cb4a80.

One co-authored maintainer commit; 2 files, 219 deletions, 0 additions.
Both touched files are byte-for-byte identical to the fully reviewed/Testbox-proven final tree from fix(slack): remove unused unsafe auth fetch helper #98495.
Blacksmith Testbox tbx_01kwpdqh65849veg0j35wzcjmx (Actions 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 28705160714 — 117 checks completed, zero failures.
Fresh current-main autoreview — no accepted/actionable findings, correctness confidence 0.99.
scripts/pr review-validate-artifacts 99944 and OPENCLAW_TESTBOX=1 scripts/pr prepare-run 99944 passed.

Known proof gap: no live Slack workspace run. The removed helper had no runtime caller and no public API export; active Slack downloads are unchanged and retain their existing guarded-fetch tests.

After landing, #98495 will be closed as superseded with the canonical commit and contributor thanks.

clawsweeper · 2026-07-04T11:55:18Z

Codex review: needs real behavior proof before merge. Reviewed July 4, 2026, 8:05 AM ET / 12:05 UTC.

Summary
The PR deletes the unused Slack fetchWithSlackAuth helper and its helper-only tests; its head also contains a memory FD repro config cleanup that is already on current main.

PR surface: Source -52, Tests -170, Other -2. Total -224 across 4 files.

Reproducibility: not applicable. this is a dead-code cleanup PR rather than a reported runtime bug. Source search verifies no production callers, and retained tests exercise the active Slack media path.

Review metrics: 1 noteworthy metric.

Effective merge delta: 2 Slack files changed after current-main merge. The potential merge commit shows the memory repro config diff is already on main, so maintainers should review the Slack deletion as the remaining change.

Stored data model
Persistent data-model change detected: vector/embedding metadata: test/scripts/check-memory-fd-repro.test.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: canonical
Canonical: #99944
Summary: This PR is the current-main replacement for the closed predecessor branch that attempted the same Slack helper removal.

Members:

superseded: fix(slack): remove unused unsafe auth fetch helper #98495 - The related PR is closed unmerged and is described by the current PR body and timeline as superseded by this replacement branch.

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

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
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:

none.

Risk before merge

[P1] No live Slack workspace run is attached; this is low residual risk because the removed helper has no production caller and the retained Slack media redirect path is covered by source and tests.

Maintainer options:

Decide the mitigation before merge
Land the Slack-only merge result once required checks finish, then treat fix(slack): remove unused unsafe auth fetch helper #98495 as the superseded predecessor rather than keeping two branches open.
Pause or close
Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

[P1] No repair lane is needed because the patch has no blocking review findings; remaining action is normal protected maintainer landing after required checks.

Security
Cleared: The diff removes an unused auth/redirect helper and does not add dependencies, workflow execution, secret handling, or public API surface.

Review details

Best possible solution:

Land the Slack-only merge result once required checks finish, then treat #98495 as the superseded predecessor rather than keeping two branches open.

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

Not applicable: this is a dead-code cleanup PR rather than a reported runtime bug. Source search verifies no production callers, and retained tests exercise the active Slack media path.

Is this the best way to solve the issue?

Yes: deleting the unreachable private helper is the best fix because it avoids a duplicate redirect/auth implementation while keeping active downloads on the shared guarded-fetch path.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

add P3: This is a low-risk cleanup/security-hardening PR that removes unreachable private Slack helper code without changing active runtime behavior.
add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The maintainer-labeled PR removes unreachable private code, so the external contributor real-behavior proof gate does not apply; Testbox and hosted CI are still useful supplemental proof.

Label justifications:

P3: This is a low-risk cleanup/security-hardening PR that removes unreachable private Slack helper code without changing active runtime behavior.
rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The maintainer-labeled PR removes unreachable private code, so the external contributor real-behavior proof gate does not apply; Testbox and hosted CI are still useful supplemental proof.

Evidence reviewed

PR surface:

Source -52, Tests -170, Other -2. Total -224 across 4 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	0	52	-52
Tests	2	1	171	-170
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	1	0	2	-2
Total	4	1	225	-224

What I checked:

Live PR state: Live GitHub state shows head 4af0954fe76a7dba287f1c56bbcb24fa7a9f52ba, mergeable with merge state UNSTABLE, labels including maintainer, and a PR body/comment with Testbox and hosted CI evidence plus a known no-live-Slack gap. (4af0954fe76a)
Effective merge result: The potential merge commit da66d78f9383e16d9a9fd8b618c69513ea267f46 compares against current main with only the two Slack media files changed, because the memory FD repro config cleanup is already on main. (da66d78f9383)
Helper is private dead code: Repository search on current main found fetchWithSlackAuth only in extensions/slack/src/monitor/media.ts and its colocated tests, while extensions/slack/api.ts does not export it. (extensions/slack/src/monitor/media.ts:117, d6ed2c392c1e)
Active Slack media path remains guarded: The retained runtime path builds a Slack-authenticated request, uses createSlackMediaFetch, and passes it through saveSlackMedia/saveRemoteMedia with the Slack SSRF policy. (extensions/slack/src/monitor/media.ts:322, d6ed2c392c1e)
Retained tests cover redirect auth behavior: The remaining resolveSlackMedia tests cover preserving Authorization on same-origin Slack redirects and stripping it on cross-origin Slack CDN redirects. (extensions/slack/src/monitor/media.test.ts:452, d6ed2c392c1e)
Shared guarded fetch contract: fetchWithSsrFGuard follows redirects manually, strips unsafe cross-origin headers, and only restores Authorization for HTTPS redirect targets matching an explicit allowlist. (src/infra/net/fetch-guard.ts:620, d6ed2c392c1e)

Likely related people:

vincentkoc: Auth-preserving Slack media redirect behavior was introduced in 55cbcd829d, and the current-main memory FD repro config cleanup is d6ed2c392c. (role: introduced behavior and recent adjacent owner; confidence: high; commits: 55cbcd829d38, d6ed2c392c1e; files: extensions/slack/src/monitor/media.ts, extensions/slack/src/monitor/media.test.ts, scripts/check-memory-fd-repro.mjs)
openperf: e8fb140642 moved Slack media downloads onto dispatcher-aware guarded transport, which is the retained path this PR relies on. (role: introduced guarded Slack media transport; confidence: medium; commits: e8fb14064207; files: extensions/slack/src/monitor/media.ts, extensions/slack/src/monitor/media.test.ts)
steipete: Recent current-main history includes Slack media/network test isolation and Slack thread context work, and this PR is authored from the repository branch. (role: recent area contributor; confidence: medium; commits: f3abc0c076ff, 6b525023d4d2; files: extensions/slack/src/monitor/media.ts, extensions/slack/src/monitor/media.test.ts, extensions/slack/src/monitor/message-handler/prepare-thread-context.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.

steipete · 2026-07-04T12:21:35Z

Merged via squash.

Prepared head SHA: 4af0954fe76a7dba287f1c56bbcb24fa7a9f52ba
Landed commit: 706443c79b1cb2f53aceaee08c3f3bedeb9ce8b5

* fix(slack): remove unused unsafe auth fetch helper Co-authored-by: liyuanbin <[email protected]> * fix(slack): remove unused unsafe auth fetch helper --------- Co-authored-by: liyuanbin <[email protected]>

fix(slack): remove unused unsafe auth fetch helper

33f897b

Co-authored-by: liyuanbin <[email protected]>

openclaw-barnacle Bot added channel: slack Channel integration: slack size: M maintainer Maintainer-authored PR labels Jul 4, 2026

steipete self-assigned this Jul 4, 2026

fix(slack): remove unused unsafe auth fetch helper

4af0954

openclaw-barnacle Bot added the scripts Repository scripts label Jul 4, 2026

clawsweeper Bot mentioned this pull request Jul 4, 2026

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

Closed

steipete merged commit 706443c into main Jul 4, 2026
127 of 132 checks passed

steipete deleted the codex/slack-remove-unused-auth-fetch branch July 4, 2026 12:21

github-actions Bot mentioned this pull request Jul 4, 2026

📡 Upstream Digest — 2026-07-04 13:04 UTC curtismercier/openclaw-mods#1204

Open

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#99944

fix(slack): remove unused unsafe auth fetch helper#99944
steipete merged 2 commits into
mainfrom
codex/slack-remove-unused-auth-fetch

steipete commented Jul 4, 2026

Uh oh!

steipete commented Jul 4, 2026

Uh oh!

clawsweeper Bot commented Jul 4, 2026 •

edited

Loading

Uh oh!

Uh oh!

steipete commented Jul 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

steipete commented Jul 4, 2026

What Problem This Solves

Why This Change Was Made

User Impact

Evidence

Uh oh!

steipete commented Jul 4, 2026

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

steipete commented Jul 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

clawsweeper Bot commented Jul 4, 2026 •

edited

Loading