fix: pre-resolve HuggingFace redirects to fix 0-byte downloads on Android#110
Conversation
…roid HuggingFace download URLs return a 302 redirect to a ~1350-char signed CDN URL (cas-bridge.xethub.hf.co). Some OEM DownloadManager implementations silently fail to follow this redirect, causing downloads to stay at 0 bytes. Reported on Samsung S25 Ultra, OnePlus 11, and Xiaomi devices. - Pre-resolve redirects via HEAD request before calling DownloadManager.enqueue() so it receives the final CDN URL directly with no redirects needed - Falls back to original URL on any resolution error - Move startDownload work to background thread (resolveRedirects does network I/O) - Surface DownloadManager reason in progress events and download UI for diagnostics Closes #107 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug on specific Android devices where downloads from HuggingFace would get stuck at 0 bytes. The issue stemmed from the Android DownloadManager's inability to properly handle 302 redirects to lengthy CDN URLs. The solution involves proactively resolving these redirects to provide the DownloadManager with the direct content URL. Additionally, the changes enhance the user experience by exposing the underlying reason for download stalls or failures directly within the application's download interface. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a solid fix for an issue where downloads fail on certain Android devices due to improper handling of HTTP redirects. The approach of pre-resolving the final URL before enqueuing the download is well-implemented, including performing network operations on a background thread and safely falling back to the original URL on errors. Additionally, surfacing the DownloadManager's reason string in the UI is a nice improvement for diagnostics. However, it introduces two security vulnerabilities: a high-severity Path Traversal vulnerability via the fileName parameter and a medium-severity SSRF vulnerability via the url parameter. Both issues stem from insufficient validation of user-supplied input before sensitive operations. I also have a couple of suggestions for the Android native module to further improve robustness and resource management.
| val fileName = params.getString("fileName") ?: run { | ||
| promise.reject("DOWNLOAD_ERROR", "fileName is required") | ||
| return | ||
| } |
There was a problem hiding this comment.
The fileName parameter is used to construct file paths without sanitization. An attacker can provide a fileName containing path traversal sequences (e.g., ../) to manipulate file paths, potentially leading to arbitrary file deletion (line 143) or arbitrary file write (line 165) within the app's accessible directories.
| val fileName = params.getString("fileName") ?: run { | |
| promise.reject("DOWNLOAD_ERROR", "fileName is required") | |
| return | |
| } | |
| val fileName = params.getString("fileName")?.let { java.io.File(it).name } ?: run { | |
| promise.reject("DOWNLOAD_ERROR", "fileName is required") | |
| return | |
| } |
There was a problem hiding this comment.
Fixed — fileName is now sanitized with File(it).name to strip any path traversal sequences. Good catch.
| connection.disconnect() | ||
| } | ||
| } | ||
| return currentUrl |
There was a problem hiding this comment.
When the for loop completes, it means the maxRedirects limit has been reached. Returning currentUrl here might still pass a redirect URL to DownloadManager, which could cause the same issue this change aims to fix if that URL is also a redirect. It would be safer to treat this as a resolution failure, log a warning, and fall back to the originalUrl as you do in the catch block. This aligns with the goal of providing a safe fallback.
android.util.Log.w("DownloadManager", "Redirect resolution failed: Exceeded max redirects ($maxRedirects). Using original URL.")
return originalUrlThere was a problem hiding this comment.
Fixed — now logs a warning and returns originalUrl when max redirects is exceeded, consistent with the error fallback behavior.
| // Pre-resolve redirects so DownloadManager gets the final CDN URL directly. | ||
| // HuggingFace returns a 302 redirect to a long signed CDN URL (~1350 chars) | ||
| // that some OEM DownloadManager implementations fail to follow silently. | ||
| val resolvedUrl = resolveRedirects(url) |
There was a problem hiding this comment.
The url parameter is passed to resolveRedirects, which performs a network request using HttpURLConnection. Since the URL is not validated against an allow-list of trusted domains, an attacker can use this to perform SSRF attacks, probing internal network resources or local services on the device.
There was a problem hiding this comment.
Fixed — added an allowlist of trusted HuggingFace download hosts (huggingface.co, cdn-lfs.huggingface.co, cas-bridge.xethub.hf.co). URLs with unrecognized hosts are now rejected before any network request.
| val hideNotification = params.hasKey("hideNotification") && params.getBoolean("hideNotification") | ||
|
|
||
| // Resolve redirects on a background thread (network I/O) | ||
| Thread { |
There was a problem hiding this comment.
While creating a new Thread for each download is functional, it's generally better practice to use an ExecutorService to manage background threads. This provides better control over thread lifecycle and resource usage, preventing potential issues from creating an unbounded number of threads if downloads are initiated in quick succession.
Consider defining a class-level executor, for example:
private val executor = java.util.concurrent.Executors.newSingleThreadExecutor()
And then using it here:
executor.execute { ... }
This would serialize download initiations, which is often desirable.
There was a problem hiding this comment.
Fixed — replaced raw Thread with a class-level Executors.newSingleThreadExecutor() to serialize download initiations and manage thread lifecycle properly.
…e executor - Sanitize fileName with File(it).name to prevent path traversal (high) - Validate download URL against allowlist of HuggingFace hosts to prevent SSRF (medium) - Replace raw Thread with single-thread ExecutorService for better resource management (medium) - Fall back to originalUrl when max redirects exceeded instead of returning last redirect (high) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses the 0-byte download issue on certain Android devices by pre-resolving HuggingFace redirects. While it correctly adds an initial whitelist for allowed download hosts to prevent Server-Side Request Forgery (SSRF), a critical bypass is introduced in the manual redirect resolution logic by failing to re-validate the host after following a redirect. Additionally, the PR improves security by sanitizing the fileName parameter to prevent path traversal, moves network operations to a background thread to prevent ANRs, and enhances diagnostic capabilities by surfacing DownloadManager reason strings in the UI.
| private val allowedDownloadHosts = setOf( | ||
| "huggingface.co", | ||
| "cdn-lfs.huggingface.co", | ||
| "cas-bridge.xethub.hf.co", | ||
| ) |
There was a problem hiding this comment.
The introduction of allowedDownloadHosts is a critical security enhancement. It helps prevent Server-Side Request Forgery (SSRF) attacks by ensuring that the application only attempts to download from trusted domains. This is a highly recommended security practice.
| val parsedHost = try { URL(url).host } catch (_: Exception) { null } | ||
| if (parsedHost == null || !allowedDownloadHosts.any { parsedHost == it || parsedHost.endsWith(".$it") }) { | ||
| promise.reject("DOWNLOAD_ERROR", "Download URL host not allowed: $parsedHost") | ||
| return |
There was a problem hiding this comment.
This URL validation against allowedDownloadHosts is a crucial security check. It directly leverages the allowedDownloadHosts set to prevent the application from making requests to arbitrary external URLs, which could be exploited for SSRF. This is an excellent addition.
| executor.execute { | ||
| try { |
There was a problem hiding this comment.
Moving the network-intensive resolveRedirects call and subsequent DownloadManager.enqueue operation to a background thread using executor.execute is a significant improvement. This prevents potential Application Not Responding (ANR) errors and keeps the UI responsive, adhering to Android's best practices for network operations.
| internal fun resolveRedirects(originalUrl: String, maxRedirects: Int = 5): String { | ||
| var currentUrl = originalUrl | ||
| for (i in 0 until maxRedirects) { | ||
| val connection = URL(currentUrl).openConnection() as HttpURLConnection | ||
| try { | ||
| connection.instanceFollowRedirects = false | ||
| connection.requestMethod = "HEAD" | ||
| connection.connectTimeout = 10_000 | ||
| connection.readTimeout = 10_000 | ||
| val responseCode = connection.responseCode | ||
| if (responseCode in 300..399) { | ||
| val location = connection.getHeaderField("Location") | ||
| if (location.isNullOrEmpty()) return currentUrl | ||
| currentUrl = if (location.startsWith("http")) { | ||
| location | ||
| } else { | ||
| URL(URL(currentUrl), location).toString() | ||
| } | ||
| } else { | ||
| return currentUrl | ||
| } | ||
| } catch (e: Exception) { | ||
| android.util.Log.w("DownloadManager", "Redirect resolution failed, using original URL", e) | ||
| return originalUrl | ||
| } finally { | ||
| connection.disconnect() | ||
| } | ||
| } | ||
| android.util.Log.w("DownloadManager", "Redirect resolution exceeded max redirects ($maxRedirects), using original URL") | ||
| return originalUrl | ||
| } |
There was a problem hiding this comment.
The resolveRedirects function, while efficiently using HEAD requests and reasonable connectTimeout/readTimeout values (10 seconds), introduces a critical Server-Side Request Forgery (SSRF) vulnerability. It manually follows HTTP redirects without re-validating the host of the redirected URLs against the allowedDownloadHosts whitelist. This allows an attacker to bypass the initial SSRF protection by redirecting to internal or restricted resources. To remediate this, the host of the redirected URL should be validated against the allowedDownloadHosts whitelist inside the resolveRedirects loop before making the next request.
internal fun resolveRedirects(originalUrl: String, maxRedirects: Int = 5): String {
var currentUrl = originalUrl
for (i in 0 until maxRedirects) {
val connection = URL(currentUrl).openConnection() as HttpURLConnection
try {
connection.instanceFollowRedirects = false
connection.requestMethod = "HEAD"
connection.connectTimeout = 10_000
connection.readTimeout = 10_000
val responseCode = connection.responseCode
if (responseCode in 300..399) {
val location = connection.getHeaderField("Location")
if (location.isNullOrEmpty()) return currentUrl
val nextUrl = if (location.startsWith("http")) {
location
} else {
URL(URL(currentUrl), location).toString()
}
// Validate the redirected host against the whitelist
val nextHost = try { URL(nextUrl).host } catch (_: Exception) { null }
if (nextHost == null || !allowedDownloadHosts.any { nextHost == it || nextHost.endsWith(".$it") }) {
android.util.Log.w("DownloadManager", "Redirect to unauthorized host blocked: $nextHost")
return currentUrl
}
currentUrl = nextUrl
} else {
return currentUrl
}
} catch (e: Exception) {
android.util.Log.w("DownloadManager", "Redirect resolution failed, using original URL", e)
return originalUrl
} finally {
connection.disconnect()
}
}
android.util.Log.w("DownloadManager", "Redirect resolution exceeded max redirects ($maxRedirects), using original URL")
return originalUrl
}| val location = connection.getHeaderField("Location") | ||
| if (location.isNullOrEmpty()) return currentUrl | ||
| currentUrl = if (location.startsWith("http")) { | ||
| location | ||
| } else { | ||
| URL(URL(currentUrl), location).toString() | ||
| } |
There was a problem hiding this comment.
The logic for handling the Location header, specifically distinguishing between absolute and relative URLs, is well-implemented. This ensures that redirects are followed correctly regardless of the format of the Location header, which is a common source of bugs in manual redirect handling.
| } catch (e: Exception) { | ||
| android.util.Log.w("DownloadManager", "Redirect resolution failed, using original URL", e) | ||
| return originalUrl |
There was a problem hiding this comment.
The try-catch block around the network operations in resolveRedirects with a fallback to the originalUrl ensures resilience. If any error occurs during redirect resolution, the download can still proceed with the initial URL, preventing complete failure and improving user experience.
| putDouble("totalBytes", statusInfo.getDouble("totalBytes").takeIf { it > 0 } | ||
| ?: download.optDouble("totalBytes", 0.0)) | ||
| putString("status", status) | ||
| putString("reason", statusInfo.getString("reason") ?: "") |
There was a problem hiding this comment.
Adding the reason string to the download progress event is a valuable diagnostic improvement. This allows the UI to display more specific information about why a download might be paused or failed, enhancing the user's understanding of the download status.
| if (cancelledKeysRef.current.has(key)) return; | ||
| const existing = useAppStore.getState().downloadProgress[key]; | ||
| if (existing && existing.bytesDownloaded >= event.bytesDownloaded) return; | ||
| if ((useAppStore.getState().downloadProgress[key]?.bytesDownloaded ?? -1) >= event.bytesDownloaded) return; |
There was a problem hiding this comment.
The change to if ((useAppStore.getState().downloadProgress[key]?.bytesDownloaded ?? -1) >= event.bytesDownloaded) return; makes the progress update logic more robust. By using the null-safe operator ?. and providing a default value of -1, it correctly handles cases where downloadProgress[key] might be undefined or null, preventing potential issues with missed or incorrect progress updates.
Re-validate each redirected URL's host against allowedDownloadHosts inside the resolveRedirects loop, preventing an attacker from using a trusted initial URL that redirects to an internal/restricted resource. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/gemini review |
|
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a bug with HuggingFace downloads on certain Android devices by pre-resolving redirects. The implementation is robust, using HttpURLConnection to manually follow redirects and re-validating hosts at each step to prevent security vulnerabilities like SSRF. The networking and file operations are correctly moved to a background thread using an ExecutorService, preventing ANRs. Additionally, the changes improve diagnostics by surfacing the DownloadManager reason string in the UI. My review includes one high-severity suggestion to prevent a potential thread leak by ensuring the ExecutorService is properly shut down.
| } | ||
| } | ||
|
|
||
| private val executor = Executors.newSingleThreadExecutor() |
There was a problem hiding this comment.
The ExecutorService created here is not shut down when the module is destroyed. This can lead to a thread leak, as the executor's thread may keep the application process alive unnecessarily. It's important to manage the lifecycle of the executor to ensure resources are released correctly.
You should shut down the executor in the onCatalystInstanceDestroy method, which is called when the React Native bridge is torn down.
Example:
override fun onCatalystInstanceDestroy() {
super.onCatalystInstanceDestroy()
if (!executor.isShutdown) {
executor.shutdown()
}
}This method needs to be added to the DownloadManagerModule class.
|
Addressed the executor lifecycle comment — added |
1 participant
Summary
Fixes downloads stuck at 0 bytes on certain Android devices (Samsung S25 Ultra, OnePlus 11, Xiaomi). HuggingFace download URLs return a 302 redirect to a ~1350-char signed CDN URL (
cas-bridge.xethub.hf.co). Some OEM DownloadManager implementations silently fail to follow this redirect.Fix: Pre-resolve the redirect via a HEAD request before calling
DownloadManager.enqueue(), so the system DownloadManager receives the final CDN URL directly with no redirects needed. Falls back to the original URL on any resolution error, so this change is safe for devices where downloads already work.Also surfaces the DownloadManager reason string in the download UI for future diagnostics.
Type of Change
Screenshots / Screen Recordings
N/A — this is a backend/native fix. The only visible UI change is that the download status line now shows the DownloadManager reason when a download is stuck (e.g. "Queued · Waiting for WiFi").
Checklist
General
Testing
npm test)React Native Specific
Security
Related Issues
Closes #107
Additional Notes
resolveRedirectsfunction usesHttpURLConnection(available on all Android versions) rather than OkHttp to avoid adding dependenciesenqueue(), this is not a concern for starting downloadsllama.rnlint failure in CI is unrelated to this change