Use base filename when saving Orbit signed-URL installer downloads

[lukeheath]

Related issue: Resolves fleetdm/confidential#16722

FileResponse.Handle (client/base_client.go) only reduced the filename to its base name
inside the Content-Disposition (!SkipMediaType) branch. On the Orbit signed-URL installer
download path, SkipMediaType is set and the server-supplied installer name comes in through
DestFile, which was then joined into the destination path as-is. This makes the download
destination depend on the exact server-supplied string rather than always staying inside the
configured download directory.

This applies filepath.Base to the DestFile fallback as well, so only the base name is used,
and re-guards the degenerate ""/"."/".." cases before filepath.Join.

Checklist for submitter

• Changes file added for user-visible changes in changes/.
• Input data is properly validated, SELECT * is avoided, SQL injection is prevented, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.

Testing

• Added/updated automated tests
• QA'd all new/changed functionality manually

fleetd/orbit/Fleet Desktop

• If the change applies to only one platform, confirmed that runtime.GOOS is used as needed to isolate changes (N/A — filepath.Base treats / and \ as separators on both platforms)
• Verified that fleetd runs on macOS, Linux and Windows

Summary by CodeRabbit

• Bug Fixes

  • Downloaded installers are now saved using only the filename provided, preventing unexpected directory components from affecting the destination.
  • Download paths remain safely contained within the selected destination folder, including when media-type information is unavailable or invalid.

• Tests

  • Added coverage to verify that downloads cannot overwrite files outside the selected destination folder.
  • Expanded Windows test coverage for client functionality.

[lukeheath]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: f1334e07-5d76-4e5d-8d69-e921034ba522

📥 Commits

Reviewing files that changed from the base of the PR and between 403cc44 and ba2efad.

📒 Files selected for processing (4)

• .github/workflows/test-go-windows.yml
• client/base_client.go
• client/base_client_test.go
• orbit/changes/16722-orbit-installer-filename

🚧 Files skipped from review as they are similar to previous changes (3)

• client/base_client.go
• client/base_client_test.go
• .github/workflows/test-go-windows.yml

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

Walkthrough

FileResponse.Handle now reduces fallback DestFile values to base filenames before constructing download paths. Client tests cover traversal attempts and backslash-separated filenames across platforms. The Windows Go workflow now triggers for client Go changes and runs client package tests. A changelog entry documents the installer filename behavior.

Merge Risk: ⚪ Minimal · up to ba2ef

The change keeps server-supplied installer downloads within the configured directory by using only the base filename and handling degenerate names safely. No actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (2 skipped: 2 …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: saving Orbit installer downloads with only the base filename.
Description check	✅ Passed	The description explains the vulnerability, the code fix, the test coverage, the related issue, and the platform scope. It omits or leaves incomplete several template items, including manual QA, timeo…
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.

Full details: Description check

Explanation

The description explains the vulnerability, the code fix, the test coverage, the related issue, and the platform scope. It omits or leaves incomplete several template items, including manual QA, timeout and retry review, release compatibility, and auto-update verification. These omissions are not critical to understanding this focused change.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (2 skipped: 2 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix-orbit-installer-path-traversal

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[Copilot]

Warning

• Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.

Pull request overview

Updates the client-side installer download handling to ensure server-influenced filenames cannot affect the destination directory, particularly for Orbit signed-URL downloads where SkipMediaType is used.

Changes:

• Apply filepath.Base to the DestFile fallback used when SkipMediaType is set, and re-guard invalid basename edge cases.
• Add tests covering SkipMediaType + DestFile handling (including Windows backslash-separated input behavior).
• Expand Windows Go test workflow triggers and coverage to include client/**.go.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

File	Description
client/base_client.go	Sanitizes fallback filename selection for signed-URL downloads by basing DestFile and re-checking invalid names.
client/base_client_test.go	Adds regression tests ensuring SkipMediaType downloads stay within DestPath (including Windows path separator cases).
.github/workflows/test-go-windows.yml	Ensures Windows CI runs when client code changes and includes ./client/... in the test matrix.
changes/16722-orbit-installer-filename	Release note entry (content excluded from review by policy).

Files excluded by content exclusion policy (1)

• changes/16722-orbit-installer-filename

Suppressed comments (1)

client/base_client.go:255

• filepath.Base can still return an absolute root path (e.g. "/" on Unix, "\" on Windows, and potentially a drive root/volume-ish value), which would cause filepath.Join(f.DestPath, filename) to ignore DestPath and write outside the intended download directory. Since filename is ultimately server-influenced (via Content-Disposition or DestFile when SkipMediaType is true), it should also be rejected if it is absolute or contains a volume name.

	if filename == "" || filename == "." || filename == ".." {
		filename = uuid.NewString()
	}

	f.DestFilePath = filepath.Join(f.DestPath, filename)

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.70%. Comparing base (403cc44) to head (ba2efad).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #52121      +/-   ##
==========================================
- Coverage   69.79%   68.70%   -1.09%     
==========================================
  Files        4072     3843     -229     
  Lines      265453   261605    -3848     
  Branches    14237    12306    -1931     
==========================================
- Hits       185285   179748    -5537     
- Misses      63979    65655    +1676     
- Partials    16189    16202      +13     

Flag	Coverage Δ
backend	70.26% <ø> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[lukeheath]

@sharon-fdm @lucasmrod This seemed like a straightforward change so I opened a PR. Then I realized I couldn't validate locally because I don't have a local environment with cloudfront configured. Do you think the tests are sufficient to merge, given it will go through normal QA?

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[lucasmrod]

LGTM!

(I moved the changes file to orbit/changes/).
Setting milestone of the issue to fleetd-v1.62.0.

[lucasmrod]

This should be moved to orbit/changes/.