Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@
import org.json.JSONArray
import org.json.JSONObject
import java.io.File
import java.net.HttpURLConnection
import java.net.URL
import java.util.concurrent.Executors

class DownloadManagerModule(reactContext: ReactApplicationContext) :
ReactContextBaseJavaModule(reactContext) {
Expand Down Expand Up @@ -90,6 +93,14 @@
}
}

private val executor = Executors.newSingleThreadExecutor()

Choose a reason for hiding this comment

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

high

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.


private val allowedDownloadHosts = setOf(
"huggingface.co",
"cdn-lfs.huggingface.co",
"cas-bridge.xethub.hf.co",
)
Comment on lines +98 to +102

Choose a reason for hiding this comment

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

critical

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.


private val downloadManager: DownloadManager by lazy {
reactApplicationContext.getSystemService(Context.DOWNLOAD_SERVICE) as DownloadManager
}
Expand All @@ -113,67 +124,89 @@

@ReactMethod
fun startDownload(params: ReadableMap, promise: Promise) {
try {
val url = params.getString("url") ?: throw IllegalArgumentException("URL is required")
val fileName = params.getString("fileName") ?: throw IllegalArgumentException("fileName is required")
val title = params.getString("title") ?: fileName
val description = params.getString("description") ?: "Downloading model..."
val modelId = params.getString("modelId") ?: ""
val totalBytes = if (params.hasKey("totalBytes")) params.getDouble("totalBytes").toLong() else 0L
val hideNotification = params.hasKey("hideNotification") && params.getBoolean("hideNotification")

// Clean up any existing file with the same name to prevent DownloadManager
// from auto-renaming (e.g., file.gguf → file-1.gguf)
val existingFile = File(
reactApplicationContext.getExternalFilesDir(Environment.DIRECTORY_DOWNLOADS),
fileName
)
if (existingFile.exists()) {
android.util.Log.d("DownloadManager", "Deleting existing file before download: ${existingFile.absolutePath}")
existingFile.delete()
}

// Also clean up any stale entries from previous sessions
cleanupStaleDownloads()
val url = params.getString("url") ?: run {
promise.reject("DOWNLOAD_ERROR", "URL is required")
return
}
val fileName = params.getString("fileName")?.let { File(it).name } ?: run {
promise.reject("DOWNLOAD_ERROR", "fileName is required")
return
}
val title = params.getString("title") ?: fileName
val description = params.getString("description") ?: "Downloading model..."
val modelId = params.getString("modelId") ?: ""
val totalBytes = if (params.hasKey("totalBytes")) params.getDouble("totalBytes").toLong() else 0L
val hideNotification = params.hasKey("hideNotification") && params.getBoolean("hideNotification")

// Validate URL against allowed download hosts to prevent SSRF
val parsedHost = try { URL(url).host } catch (_: Exception) { null }

Check warning on line 142 in android/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.kt

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Deprecated code should not be used.

See more on https://sonarcloud.io/project/issues?id=alichherawalla_off-grid-mobile&issues=AZy-IvWG5gPz8w2klKOK&open=AZy-IvWG5gPz8w2klKOK&pullRequest=110
if (parsedHost == null || !allowedDownloadHosts.any { parsedHost == it || parsedHost.endsWith(".$it") }) {
promise.reject("DOWNLOAD_ERROR", "Download URL host not allowed: $parsedHost")
return
Comment on lines +142 to +145

Choose a reason for hiding this comment

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

critical

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.

}

val request = DownloadManager.Request(Uri.parse(url))
.setTitle(title)
.setDescription(description)
.setNotificationVisibility(
if (hideNotification) DownloadManager.Request.VISIBILITY_HIDDEN
else DownloadManager.Request.VISIBILITY_VISIBLE_NOTIFY_COMPLETED
)
.setDestinationInExternalFilesDir(
reactApplicationContext,
Environment.DIRECTORY_DOWNLOADS,
// Resolve redirects on a background thread (network I/O)
executor.execute {
try {
Comment on lines +149 to +150

Choose a reason for hiding this comment

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

high

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.

// Clean up any existing file with the same name to prevent DownloadManager
// from auto-renaming (e.g., file.gguf → file-1.gguf)
val existingFile = File(
reactApplicationContext.getExternalFilesDir(Environment.DIRECTORY_DOWNLOADS),
fileName
)
.setAllowedOverMetered(true)
.setAllowedOverRoaming(true)

val downloadId = downloadManager.enqueue(request)

// Persist download info
val downloadInfo = JSONObject().apply {
put("downloadId", downloadId)
put("url", url)
put("fileName", fileName)
put("modelId", modelId)
put("title", title)
put("totalBytes", totalBytes)
put("status", "pending")
put("startedAt", System.currentTimeMillis())
}
persistDownload(downloadId, downloadInfo)
if (existingFile.exists()) {
android.util.Log.d("DownloadManager", "Deleting existing file before download: ${existingFile.absolutePath}")
existingFile.delete()
}

val result = Arguments.createMap().apply {
putDouble("downloadId", downloadId.toDouble())
putString("fileName", fileName)
putString("modelId", modelId)
// Also clean up any stale entries from previous sessions
cleanupStaleDownloads()

// 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)

Choose a reason for hiding this comment

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

security-medium medium

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

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.

android.util.Log.d("DownloadManager", "Resolved URL: ${resolvedUrl.take(120)}...")

val request = DownloadManager.Request(Uri.parse(resolvedUrl))
.setTitle(title)
.setDescription(description)
.setNotificationVisibility(
if (hideNotification) DownloadManager.Request.VISIBILITY_HIDDEN
else DownloadManager.Request.VISIBILITY_VISIBLE_NOTIFY_COMPLETED
)
.setDestinationInExternalFilesDir(
reactApplicationContext,
Environment.DIRECTORY_DOWNLOADS,
fileName
)
.setAllowedOverMetered(true)
.setAllowedOverRoaming(true)

val downloadId = downloadManager.enqueue(request)

// Persist download info
val downloadInfo = JSONObject().apply {
put("downloadId", downloadId)
put("url", url)
put("fileName", fileName)
put("modelId", modelId)
put("title", title)
put("totalBytes", totalBytes)
put("status", "pending")
put("startedAt", System.currentTimeMillis())
}
persistDownload(downloadId, downloadInfo)

val result = Arguments.createMap().apply {
putDouble("downloadId", downloadId.toDouble())
putString("fileName", fileName)
putString("modelId", modelId)
}
promise.resolve(result)
} catch (e: Exception) {
promise.reject("DOWNLOAD_ERROR", "Failed to start download: ${e.message}", e)
}
promise.resolve(result)
} catch (e: Exception) {
promise.reject("DOWNLOAD_ERROR", "Failed to start download: ${e.message}", e)
}
}

Expand Down Expand Up @@ -344,6 +377,52 @@
// Required for RN event emitter
}

/**
* Follow HTTP redirects manually and return the final URL.
* Some OEM DownloadManager implementations silently fail on 302 redirects
* to long signed CDN URLs (e.g. HuggingFace → xethub.hf.co).
* By pre-resolving, DownloadManager gets the direct URL with no redirects.
* Falls back to the original URL on any error so downloads aren't blocked.
*/
internal fun resolveRedirects(originalUrl: String, maxRedirects: Int = 5): String {

Check failure on line 387 in android/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.kt

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this method to reduce its Cognitive Complexity from 21 to the 15 allowed.

See more on https://sonarcloud.io/project/issues?id=alichherawalla_off-grid-mobile&issues=AZy-LI_NrCdcK0ItIvEQ&open=AZy-LI_NrCdcK0ItIvEQ&pullRequest=110
var currentUrl = originalUrl
for (i in 0 until maxRedirects) {
val connection = URL(currentUrl).openConnection() as HttpURLConnection

Check warning on line 390 in android/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.kt

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Deprecated code should not be used.

See more on https://sonarcloud.io/project/issues?id=alichherawalla_off-grid-mobile&issues=AZy-HCBpRgeTSH_c7Lja&open=AZy-HCBpRgeTSH_c7Lja&pullRequest=110
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()

Check warning on line 403 in android/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.kt

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Deprecated code should not be used.

See more on https://sonarcloud.io/project/issues?id=alichherawalla_off-grid-mobile&issues=AZy-HCBpRgeTSH_c7Ljb&open=AZy-HCBpRgeTSH_c7Ljb&pullRequest=110

Check warning on line 403 in android/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.kt

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Deprecated code should not be used.

See more on https://sonarcloud.io/project/issues?id=alichherawalla_off-grid-mobile&issues=AZy-HCBpRgeTSH_c7Ljc&open=AZy-HCBpRgeTSH_c7Ljc&pullRequest=110
}
Comment on lines +398 to +404

Choose a reason for hiding this comment

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

medium

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.

// Re-validate redirected host against allowlist to prevent SSRF bypass
val nextHost = try { URL(nextUrl).host } catch (_: Exception) { null }

Check warning on line 406 in android/app/src/main/java/ai/offgridmobile/download/DownloadManagerModule.kt

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Deprecated code should not be used.

See more on https://sonarcloud.io/project/issues?id=alichherawalla_off-grid-mobile&issues=AZy-LI_NrCdcK0ItIvEP&open=AZy-LI_NrCdcK0ItIvEP&pullRequest=110
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
Comment on lines +415 to +417

Choose a reason for hiding this comment

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

medium

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.

} finally {
connection.disconnect()
}
}
android.util.Log.w("DownloadManager", "Redirect resolution exceeded max redirects ($maxRedirects), using original URL")
return originalUrl
}
Comment on lines +387 to +424

Choose a reason for hiding this comment

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

security-medium medium

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
    }


private fun pollAllDownloads() {
val downloads = getAllPersistedDownloads()

Expand All @@ -361,6 +440,7 @@
putDouble("totalBytes", statusInfo.getDouble("totalBytes").takeIf { it > 0 }
?: download.optDouble("totalBytes", 0.0))
putString("status", status)
putString("reason", statusInfo.getString("reason") ?: "")

Choose a reason for hiding this comment

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

medium

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.

}

val previousStatus = download.optString("status", "pending")
Expand Down
8 changes: 6 additions & 2 deletions src/screens/DownloadManagerScreen/items.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,11 @@ export type DownloadItem = {
filePath?: string;
isVisionModel?: boolean;
mmProjPath?: string;
reason?: string;
};

export interface DownloadItemsData {
downloadProgress: Record<string, { progress: number; bytesDownloaded: number; totalBytes: number }>;
downloadProgress: Record<string, { progress: number; bytesDownloaded: number; totalBytes: number; reason?: string }>;
activeDownloads: BackgroundDownloadInfo[];
activeBackgroundDownloads: Record<number, { modelId: string; fileName: string; author: string; quantization: string; totalBytes: number } | null>;
downloadedModels: DownloadedModel[];
Expand Down Expand Up @@ -87,6 +88,7 @@ export function buildDownloadItems(data: DownloadItemsData): DownloadItem[] {
bytesDownloaded: progress.bytesDownloaded,
progress: progress.progress,
status: 'downloading',
reason: progress.reason,
});
});

Expand Down Expand Up @@ -191,7 +193,9 @@ export const ActiveDownloadCard: React.FC<ActiveDownloadCardProps> = ({ item, on
<View style={styles.quantBadge}>
<Text style={styles.quantText}>{item.quantization}</Text>
</View>
<Text style={styles.statusText}>{getStatusText(item.status)}</Text>
<Text style={styles.statusText}>
{getStatusText(item.status)}{item.reason ? ` · ${item.reason}` : ''}
</Text>
</View>
</Card>
);
Expand Down
4 changes: 2 additions & 2 deletions src/screens/DownloadManagerScreen/useDownloadManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,12 @@ export function useDownloadManager(): UseDownloadManagerResult {
if (!metadata) return;
const key = `${metadata.modelId}/${metadata.fileName}`;
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;

Choose a reason for hiding this comment

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

medium

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.

setDownloadProgress(key, {
progress: event.totalBytes > 0 ? event.bytesDownloaded / event.totalBytes : 0,
bytesDownloaded: event.bytesDownloaded,
totalBytes: event.totalBytes,
reason: event.reason || undefined,
});
});

Expand Down
1 change: 1 addition & 0 deletions src/services/backgroundDownloadService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ interface DownloadProgressEvent {
bytesDownloaded: number;
totalBytes: number;
status: BackgroundDownloadStatus;
reason?: string;
}

interface DownloadCompleteEvent {
Expand Down
2 changes: 1 addition & 1 deletion src/stores/appStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { Platform } from 'react-native';
import AsyncStorage from '@react-native-async-storage/async-storage';
import { DeviceInfo, DownloadedModel, ModelRecommendation, ONNXImageModel, ImageGenerationMode, AutoDetectMethod, ModelLoadingStrategy, CacheType, GeneratedImage, PersistedDownloadInfo } from '../types';

type DownloadProgressInfo = { progress: number; bytesDownloaded: number; totalBytes: number };
type DownloadProgressInfo = { progress: number; bytesDownloaded: number; totalBytes: number; reason?: string };

type OnboardingChecklist = {
downloadedModel: boolean; loadedModel: boolean; sentMessage: boolean;
Expand Down
Loading