fix(slack): preserve auth on same-origin media redirects#62996
Conversation
openclaw-barnacle
Bot
added
channel: slack
b32a96c to
9af257e
Compare
|
Landed via squash after rebasing onto the latest
Note: repo-wide Thanks @vincentkoc! |
) (thanks @vincentkoc) - Verified: pnpm build\n- Verified: pnpm test extensions/slack/src/monitor/media.test.ts\n- Verified: pnpm exec oxlint extensions/slack/src/monitor/media.ts extensions/slack/src/monitor/media.test.ts\n- Verified: pnpm exec oxfmt --check extensions/slack/src/monitor/media.ts extensions/slack/src/monitor/media.test.ts CHANGELOG.md\n\nRepo-wide pnpm lint and pnpm test were not clean on current main outside this fix, and the first full-suite test attempt from the default core sparse profile was additionally contaminated by missing ui/packages/OpenClawKit paths until they were materialized.
Greptile SummaryThis PR fixes Slack Confidence Score: 5/5Safe to merge — the core fix is correct and well-tested; only a P2 style concern remains. The bug fix (moving auth into No files require special attention beyond the P2 note on
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export async function fetchWithSlackAuth(url: string, token: string): Promise<Response> { | ||
| const parsed = assertSlackFileUrl(url); | ||
| const authHeaders = createSlackAuthHeaders(token); | ||
|
|
||
| // Initial request with auth and manual redirect handling | ||
| const initialRes = await fetch(parsed.href, { | ||
| headers: { Authorization: `Bearer ${token}` }, | ||
| headers: authHeaders, | ||
| redirect: "manual", | ||
| }); | ||
|
|
||
| // If not a redirect, return the response directly | ||
| if (initialRes.status < 300 || initialRes.status >= 400) { | ||
| return initialRes; | ||
| } | ||
|
|
||
| // Handle redirect - the redirected URL should be pre-signed and not need auth | ||
| const redirectUrl = initialRes.headers.get("location"); | ||
| if (!redirectUrl) { | ||
| return initialRes; | ||
| } | ||
|
|
||
| // Resolve relative URLs against the original | ||
| const resolvedUrl = new URL(redirectUrl, parsed.href); | ||
|
|
||
| // Only follow safe protocols (we do NOT include Authorization on redirects). | ||
| if (resolvedUrl.protocol !== "https:") { | ||
| return initialRes; | ||
| } | ||
|
|
||
| // Follow the redirect without the Authorization header | ||
| // (Slack's CDN URLs are pre-signed and don't need it) | ||
| if (resolvedUrl.origin === parsed.origin) { | ||
| return fetch(resolvedUrl.toString(), { | ||
| headers: authHeaders, | ||
| redirect: "follow", | ||
| }); | ||
| } | ||
| return fetch(resolvedUrl.toString(), { redirect: "follow" }); | ||
| } |
There was a problem hiding this comment.
fetchWithSlackAuth is exported but has no production callers
This function is exported and thoroughly tested in isolation, but it is not called by any production code path — not by resolveSlackMedia, resolveSlackAttachmentContent, actions.ts, or any other file in the extension (confirmed via a full extension-tree search). The actual fix routes auth through requestInit into fetchRemoteMedia's built-in redirect handling. If fetchWithSlackAuth is intended as a utility for future Slack actions, a brief doc-comment noting its purpose and a re-export via api.ts would clarify intent; otherwise it's dead exported surface that could mislead future readers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/slack/src/monitor/media.ts
Line: 92-121
Comment:
**`fetchWithSlackAuth` is exported but has no production callers**
This function is exported and thoroughly tested in isolation, but it is not called by any production code path — not by `resolveSlackMedia`, `resolveSlackAttachmentContent`, `actions.ts`, or any other file in the extension (confirmed via a full extension-tree search). The actual fix routes auth through `requestInit` into `fetchRemoteMedia`'s built-in redirect handling. If `fetchWithSlackAuth` is intended as a utility for future Slack actions, a brief doc-comment noting its purpose and a re-export via `api.ts` would clarify intent; otherwise it's dead exported surface that could mislead future readers.
How can I resolve this? If you propose a fix, please make it concise.
…anks @vincentkoc) - Verified: pnpm build\n- Verified: pnpm test extensions/slack/src/monitor/media.test.ts\n- Verified: pnpm exec oxlint extensions/slack/src/monitor/media.ts extensions/slack/src/monitor/media.test.ts\n- Verified: pnpm exec oxfmt --check extensions/slack/src/monitor/media.ts extensions/slack/src/monitor/media.test.ts CHANGELOG.md\n\nRepo-wide pnpm lint and pnpm test were not clean on current main outside this fix, and the first full-suite test attempt from the default core sparse profile was additionally contaminated by missing ui/packages/OpenClawKit paths until they were materialized.
) (thanks @vincentkoc) - Verified: pnpm build\n- Verified: pnpm test extensions/slack/src/monitor/media.test.ts\n- Verified: pnpm exec oxlint extensions/slack/src/monitor/media.ts extensions/slack/src/monitor/media.test.ts\n- Verified: pnpm exec oxfmt --check extensions/slack/src/monitor/media.ts extensions/slack/src/monitor/media.test.ts CHANGELOG.md\n\nRepo-wide pnpm lint and pnpm test were not clean on current main outside this fix, and the first full-suite test attempt from the default core sparse profile was additionally contaminated by missing ui/packages/OpenClawKit paths until they were materialized.
) (thanks @vincentkoc) - Verified: pnpm build\n- Verified: pnpm test extensions/slack/src/monitor/media.test.ts\n- Verified: pnpm exec oxlint extensions/slack/src/monitor/media.ts extensions/slack/src/monitor/media.test.ts\n- Verified: pnpm exec oxfmt --check extensions/slack/src/monitor/media.ts extensions/slack/src/monitor/media.test.ts CHANGELOG.md\n\nRepo-wide pnpm lint and pnpm test were not clean on current main outside this fix, and the first full-suite test attempt from the default core sparse profile was additionally contaminated by missing ui/packages/OpenClawKit paths until they were materialized.
) (thanks @vincentkoc) - Verified: pnpm build\n- Verified: pnpm test extensions/slack/src/monitor/media.test.ts\n- Verified: pnpm exec oxlint extensions/slack/src/monitor/media.ts extensions/slack/src/monitor/media.test.ts\n- Verified: pnpm exec oxfmt --check extensions/slack/src/monitor/media.ts extensions/slack/src/monitor/media.test.ts CHANGELOG.md\n\nRepo-wide pnpm lint and pnpm test were not clean on current main outside this fix, and the first full-suite test attempt from the default core sparse profile was additionally contaminated by missing ui/packages/OpenClawKit paths until they were materialized.
) (thanks @vincentkoc) - Verified: pnpm build\n- Verified: pnpm test extensions/slack/src/monitor/media.test.ts\n- Verified: pnpm exec oxlint extensions/slack/src/monitor/media.ts extensions/slack/src/monitor/media.test.ts\n- Verified: pnpm exec oxfmt --check extensions/slack/src/monitor/media.ts extensions/slack/src/monitor/media.test.ts CHANGELOG.md\n\nRepo-wide pnpm lint and pnpm test were not clean on current main outside this fix, and the first full-suite test attempt from the default core sparse profile was additionally contaminated by missing ui/packages/OpenClawKit paths until they were materialized.
1 participant
Summary
url_private_downloadfetches lostAuthorizationon same-originfiles.slack.comredirects, so image attachments degraded into HTML login pages and were dropped.requestInit, kept the guarded fetch wrapper for dispatcher-backed requests, and added regression coverage for same-origin auth preservation plus cross-origin auth stripping.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
extensions/slack/src/monitor/media.tsmanaged the bearer token outside the shared guarded-fetch redirect flow. It sent auth on the first hop, then unconditionally deletedAuthorizationon every subsequent hop.resolveSlackMediapath.Regression Test Plan (if applicable)
extensions/slack/src/monitor/media.test.tsfiles.slack.comredirects keepAuthorization; cross-origin Slack CDN redirects strip it.User-visible / Behavior Changes
Slack image attachments served through
url_private_downloadload again instead of being silently dropped after a login-page redirect.Diagram (if applicable)
Security Impact (required)
NoYesNoNoNoYes, explain risk + mitigation: the bearer token now stays attached only on same-origin Slack redirects. Cross-origin redirects still pass through the shared guarded-fetch stripping logic, so we do not widen token exposure beyond Slack's original host.Repro + Verification
Environment
Steps
https://files.slack.com/...to a same-origin path.Authorization.https://downloads.slack-edge.com/...and verify auth is stripped there.Expected
resolveSlackMediareturns the saved attachment instead ofnull.Actual
Evidence
Human Verification (required)
pnpm test extensions/slack/src/monitor/media.test.ts;pnpm build; targetedoxlintandoxfmt --checkon the touched files.pnpm lintorpnpm testgate because current main has unrelated failures, and the defaultcoresparse profile initially hidui/,packages/, and OpenClawKit resource paths until I materialized them.Review Conversations
Compatibility / Migration
YesNoNoRisks and Mitigations