Skip to content

fix(download): validate HTTP 206 and Content-Range in segmented download - #2463

Merged
debpalash merged 1 commit into
debpalash:mainfrom
ege-arhan:fix/segmented-download-byte-range-validation
Oct 1, 2026
Merged

debpalash merged 1 commit into
debpalash:mainfrom
ege-arhan:fix/segmented-download-byte-range-validation

Conversation

@ege-arhan

@ege-arhan ege-arhan commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #2451

Segmented download worker accepts response bodies without verifying status code or matching Content-Range byte offsets against requested segment boundaries and total size. When an origin returns 200 or an unexpected range, corrupted or offset bytes can be written into preallocated chunks.

Changes:

  • Require HTTP 206 Partial Content in parallel stream chunks
  • Parse and validate Content-Range header offsets and file size
  • Add unit tests verifying non-206 rejection and range mismatch handling

Segmented downloads now require HTTP 206 and a Content-Range that matches the requested byte interval and known file size; a total of * is allowed. This prevents incorrect response bodies from being written as valid segments. Invalid responses raise ValueError, so confirm the download flow handles this error as intended.

@greptile-apps

greptile-apps Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Retrigger

[Medium risk] Adds validation to segmented download HTTP responses.

The PR appears safe to merge.

Summary

The PR validates partial responses before writing segmented downloads and adds rejection tests.

  • Range response status, offsets, and known total size are checked.
  • Test origins now provide Content-Range headers.

Reviews (1) · Last reviewed commit: "fix(download): enforce HTTP 206 and Cont..."

@coderabbitai

coderabbitai Bot commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: debpalash/VoiceStudio/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 262f8773-6465-4ddd-a8cc-b3a125091ddf

📥 Commits

Reviewing files that changed from the base of the PR and between 0834c8b and f42bdc9.

📒 Files selected for processing (2)
  • backend/services/segmented_download.py
  • tests/test_fdl_segmented_download.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Segmented downloads now require ranged responses to use HTTP 206 and provide a valid Content-Range that matches the requested interval and known file size. Tests update ranged-response mocks and cover invalid status and range boundaries.

Changes

Segmented download validation

Layer / File(s) Summary
Validate ranged responses
backend/services/segmented_download.py, tests/test_fdl_segmented_download.py
The downloader parses and checks Content-Range and rejects responses with an incorrect status, malformed or mismatched boundaries, or an unexpected numeric total. Tests update mock headers and cover invalid status and boundaries.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: marreiradigital

Merge Risk: ⚪ Minimal · up to f42bd

The change prevents invalid ranged responses from being accepted, and the updated tests support the intended behavior. No actionable merge-blocking issue remains.

Security Architecture Review

Security architecture risk: 🔵 Low · up to f42bd

The change rejects inconsistent ranged responses before writing their bytes, without adding permissions or access paths. No introduced or worsened security concern was established. Existing resume behavior and alternate download paths limit the assurance this validation alone provides.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — A malicious or faulty origin can influence response metadata and bytes for an existing model download. The directly affected state is that file’s partial download, completion manifest, and eventual cache blob. The inspected change does not expand network reachability, credentials, or downstream authority; wider tenant or environment exposure was not established.

Security Findings and Attack Paths

  • inferred — The gate blocks incorrect status and inconsistent range metadata from reaching ranged writes. It does not address the pre-existing possibility of an overlong body writing beyond its assigned interval before the received-length check rejects it. This residual condition was not shown to be introduced or worsened by the PR and is not retained as an active PR concern.

Trust Boundaries and Controls

  • observed — Remote response geometry is now validated before accepting ranged bytes into local state. Token forwarding remains host-gated to the configured Hugging Face hosts and subdomains; the patch does not alter that credential boundary.

Resilience and Maintainability Implications

  • inferred — A worker validation failure prevents destination publication, but asyncio.gather does not itself cancel all siblings. The inspected production caller uses a separate asyncio.run invocation, which cancels and awaits pending tasks during shutdown before retry or fallback proceeds. Equivalent containment was not established for other shared-event-loop callers.

Hardening Proposals

  • proposed — As separate hardening of existing behavior, bound writes to each segment’s owned interval and make sibling cancellation and draining explicit within the service. These would strengthen partial-file integrity and failure containment independently of caller event-loop ownership.
🚥 Pre-merge checks | ✅ 7 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description explains the problem, changes, tests, and linked issue, but it does not follow the required template. It omits the required Type, Testing, Checklist, and Release cadence sections. Reformat the description to include all template sections. Select the applicable Type checkboxes, describe the testing performed, complete the Checklist, and retain the required Release cadence text.
Docstring Coverage ⚠️ Warning Docstring coverage is 30.77% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (7 passed)
Check name Status Explanation
Title check ✅ Passed The title uses conventional-commit style with the scope download and clearly describes the validation change. The description includes the issue reference #2451.
Linked Issues check ✅ Passed Issue #2451 requires segmented responses to use HTTP 206 and a matching Content-Range before writing. The downloader validates status, range start and end, and the reported total against the resolved …
Out of Scope Changes check ✅ Passed The changes stay within issue #2451. They modify segmented response validation and update or add focused tests for the required HTTP range contract; no unrelated product area or external-service scope…
Cross-Platform Default Parity ✅ Passed PASS: The PR changes the default-on segmented download path, but the new behavior is platform-neutral. The added validation only checks HTTP status and Content-Range values in `backend/services/segm…
I18n Completeness (21 Locales) ✅ Passed The pull request changes only backend/services/segmented_download.py and tests/test_fdl_segmented_download.py. It adds no frontend code, t('...') keys, or frontend user-facing strings, so the 21-local…
Local-First Guarantee ✅ Passed The PR adds only local Content-Range parsing and response validation, plus mock tests. The diff adds no cloud endpoint, account flow, API key, telemetry, or reporting call. Existing HTTP traffic rem…
Backward Compatibility ✅ Passed No backward-compatibility failure is introduced. The PR changes only segmented HTTP response validation and tests; it adds no database, migration, voice, project, settings, or engine-state changes. Ex…
  • Fix all pre-merge checks with AI

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.

@debpalash
debpalash merged commit ef42fd2 into debpalash:main Oct 1, 2026
2 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.

Segmented model downloads accept incorrect HTTP byte ranges

2 participants