Skip to content

fix: ignore unsafe hydrated release assets - #470

Open
ThunderTr77 wants to merge 4 commits into
TouchAI-org:mainfrom
ThunderTr77:fix/nonfrontend-release-feed-bug-2
Open

fix: ignore unsafe hydrated release assets#470
ThunderTr77 wants to merge 4 commits into
TouchAI-org:mainfrom
ThunderTr77:fix/nonfrontend-release-feed-bug-2

Conversation

@ThunderTr77

Copy link
Copy Markdown
Contributor

Summary

  • Ignore unsafe Velopack history asset file names that contain path separators or empty path segments.
  • Write the hydrated feed with only safe package entries so the release directory cannot be populated outside its intended folder.
  • Add a regression test covering a traversal-style package name in the existing feed.

Related issue or RFC

Related to https://github.com/TouchAI-org/TouchAI

AI assistance disclosure

  • Tool(s) used: AI assistant
  • Scope of assistance: bug identification, patch drafting, and local verification
  • Human review or rewrite performed: reviewed the diff and test output before publishing
  • Architecture or boundary impact: none

Testing evidence

Vitest targeted: tests/ci/hydrate-velopack-history.test.ts --pool=vmThreads --fileParallelism=false passed (1 test)
Prettier check passed for the changed files
ESLint passed for the changed files
git diff --check passed

pnpm test:pr did not run locally because the Windows environment only exposes pnpm through Corepack while the hook expects a direct pnpm shim. CI should provide full-suite proof before merge.

Risk notes

  • AgentService, runtime, MCP, or schema impact: none
  • database baseline or migration impact: none
  • release or packaging impact: Velopack history hydration now drops unsafe package names from hydrated feeds and downloaded package files

Screenshots or recordings

Not applicable; this changes release history hydration logic covered by unit tests.

Checklist

  • The PR title follows Conventional Commits and is valid for squash merge.
  • This PR is either ready for review or explicitly marked as a Draft PR.
  • I did not use [WIP] or similar title prefixes.
  • If AI materially assisted this PR, I disclosed the tools and scope and I personally reviewed every affected change.
  • I can explain the why, what, and how of this change without relying on an AI tool.
  • If this touches AgentService, runtime, MCP, or schema boundaries, there is an accepted RFC.
  • If this changes architecture or adds a new cross-boundary abstraction, there is an accepted RFC.
  • I ran pnpm test:pr for this code PR, or this is a docs-only change.
  • If I changed Rust behavior or tests, I reviewed pnpm test:coverage:rust or relied on CI coverage evidence.
  • If I changed desktop startup/window/search/popup/settings/E2E paths, I ran pnpm test:e2e locally or documented why CI is the first valid proof.
  • I added tests or explained why tests are not appropriate.
  • I updated docs when behavior changed.

@ThunderTr77
ThunderTr77 requested a review from hiqiancheng as a code owner June 15, 2026 14:24
@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: cb97581a-4ded-4c06-9992-170035bff547

📥 Commits

Reviewing files that changed from the base of the PR and between ff56a27 and f292aee.

📒 Files selected for processing (2)
  • apps/desktop/scripts/ci/hydrate-velopack-history.mjs
  • apps/desktop/tests/ci/hydrate-velopack-history.test.ts
📜 Recent review details
⏰ Context from checks skipped due to timeout. (5)
  • GitHub Check: Frontend Tests
  • GitHub Check: Rust Checks
  • GitHub Check: Frontend Quality
  • GitHub Check: CodeQL (rust)
  • GitHub Check: Desktop E2E Smoke (Windows)
🔇 Additional comments (4)
apps/desktop/scripts/ci/hydrate-velopack-history.mjs (2)

167-190: LGTM!


209-215: LGTM!

apps/desktop/tests/ci/hydrate-velopack-history.test.ts (2)

1-1: LGTM!


136-139: 🔒 Security & Privacy

No change needed.

fetchPublicAsset calls fetch(url, options), so the two-argument mock assertion matches the actual call shape; this is not a fetch(url) / undefined second-argument issue.

			> Likely an incorrect or invalid review comment.

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Improved desktop update handling by accepting only safe, Windows-compatible package filenames.
    • Prevented invalid or potentially unsafe package paths from being downloaded, written, or retained in release feeds.
    • Ensured hydrated update feeds include only valid package assets.

Walkthrough

The Velopack history hydrator now rejects unsafe asset filenames, filters hydrated feeds to safe .nupkg assets, and tests download, feed, filesystem, and cleanup behavior for safe and unsafe filenames.

Changes

Velopack filename safety

Layer / File(s) Summary
Filename validation and filtered feed output
apps/desktop/scripts/ci/hydrate-velopack-history.mjs
Adds Windows-compatible filename validation, requires safe .nupkg assets, and writes feeds containing only accepted assets.
Hydrator safety coverage
apps/desktop/tests/ci/hydrate-velopack-history.test.ts
Tests safe asset hydration, rejection of unsafe names, download filtering, feed contents, directory contents, and cleanup.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Feed as Velopack feed
  participant Hydrator as hydrateVelopackHistory
  participant Fetch as globalThis.fetch
  participant Filesystem as Release directory
  Feed->>Hydrator: Provide asset filenames
  Hydrator->>Hydrator: Validate safe .nupkg names
  Hydrator->>Fetch: Fetch accepted package
  Fetch-->>Hydrator: Return package contents
  Hydrator->>Filesystem: Write accepted packages and filtered feed
Loading

Suggested labels: area:ci

Suggested reviewers: hiqiancheng

Poem

I’m a rabbit with packages snug,
Safe names tucked beneath every plug.
Unsafe paths hop away,
Clean feeds brighten the day—
Thump, test, and ship with a hug! 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is Conventional Commit-style, concise, and accurately reflects the unsafe asset filtering change.
Description check ✅ Passed The description matches the template well with summary, testing, risk, AI disclosure, and checklist sections filled in.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install timed out. The project may have too many dependencies for the sandbox.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ThunderTr77
ThunderTr77 force-pushed the fix/nonfrontend-release-feed-bug-2 branch from c1c7186 to 14756d3 Compare June 15, 2026 14:29
@hiqiancheng

Copy link
Copy Markdown
Collaborator

建议覆盖更多 unsafe filename 测试。

Please add coverage for more unsafe filename cases.

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been inactive for 21 days. Please update it or leave a note if it is still in progress.

@github-actions github-actions Bot added the Stale label Jul 28, 2026
…se-feed-bug-2

# Conflicts:
#	apps/desktop/scripts/ci/hydrate-velopack-history.mjs
#	apps/desktop/tests/ci/hydrate-velopack-history.test.ts
@coderabbitai coderabbitai Bot added the area:ci CI, automation, or repository workflow changes label Jul 28, 2026
@github-actions github-actions Bot removed the Stale label Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ci CI, automation, or repository workflow changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants