Skip to content

fix(download): scope cookies to target domain#385

Merged
jackwener merged 3 commits intojackwener:mainfrom
Astro-Han:fix/scope-download-cookies
Mar 24, 2026
Merged

fix(download): scope cookies to target domain#385
jackwener merged 3 commits intojackwener:mainfrom
Astro-Han:fix/scope-download-cookies

Conversation

@Astro-Han
Copy link
Contributor

Description

Scope browser cookies used by download to the target domain and prevent cookie leakage across cross-domain redirects.

This PR fixes the direct HTTP download path so it no longer forwards unrelated browser cookies, and adds regression coverage for mixed-domain batches and CDP cookie filtering.

Related issue: #9

Type of Change

  • 🐛 Bug fix
  • ✨ New feature
  • 🌐 New site adapter
  • 📝 Documentation
  • ♻️ Refactor
  • 🔧 CI / build / tooling

Checklist

  • I ran the checks relevant to this PR
  • I updated tests or docs if needed
  • I included output or screenshots when useful

Documentation (if adding/modifying an adapter)

  • Added doc page under docs/adapters/ (if new adapter)
  • Updated docs/adapters/index.md table (if new adapter)
  • Updated sidebar in docs/.vitepress/config.mts (if new adapter)
  • Updated README.md / README.zh-CN.md when command discoverability changed
  • Used positional args for the command's primary subject unless a named flag is clearly better
  • Normalized expected adapter failures to CliError subclasses instead of raw Error

Screenshots / Output

Verification:

  • npx vitest run src/download/index.test.ts src/pipeline/steps/download.test.ts src/browser/cdp.test.ts
  • npx tsc --noEmit

Summary:

  • Scope direct-download browser cookies per target host instead of reusing the first URL's cookies
  • Strip both cookies and headers.Cookie/headers.cookie on cross-domain redirects
  • Fix CDP domain filtering to avoid substring matches like notexample.com matching example.com
  • Add regression tests for redirect cookie stripping, mixed-domain batch downloads, and CDP cookie filtering

@Astro-Han Astro-Han changed the title fix(security): scope download cookies to target domain fix(download): scope cookies to target domain Mar 24, 2026
@jackwener jackwener force-pushed the fix/scope-download-cookies branch from b793d9d to 56f8ab7 Compare March 24, 2026 15:26
Copy link
Owner

@jackwener jackwener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

结论:方向正确,我已经直接在当前分支补了一个关键 follow-up,可以合入。

Review 摘要:

  • ✅ direct HTTP path 的 cookie scoping / cross-domain redirect stripping 是对的
  • ✅ CDP cookie domain filtering 从 substring match 改成 actual domain match 也是必要修复
  • P1(已修):stepDownload()yt-dlp 仍然只拿第一条 URL 的 cookies 给整批任务复用,mixed-domain batch 还会遗漏或串用 cookies

我已补到当前分支:

  • stepDownload() 现在会收集整批 yt-dlp 目标域名,按域名提取 cookies,再做 dedupe 后生成 cookiesFile
  • 补了 regression test,覆盖 mixed-domain yt-dlp batch 场景

本地验证已过:

  • npx vitest run src/download/index.test.ts src/pipeline/steps/download.test.ts src/browser/cdp.test.ts
  • npm run typecheck
  • npm test
  • npm run build

follow-up commit: 56f8ab7

@jackwener jackwener merged commit 106ab3a into jackwener:main Mar 24, 2026
11 checks passed
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