fix: validate patch source before saving to prevent invalid sources #2884
Conversation
|
I have resolved the merge conflicts and fixed the build errors. (Note: The remaining CI failure regarding the 'GPG key' seems expected for a fork.) Could you please review the changes again? |
|
Can you link the respective issue this RP closes? |
|
Done. I have added the issue number in the PR description/body. |
| // Reload to update metadata | ||
| reload() |
| // Validate URL format first | ||
| val source = try { | ||
| SourceInfo.from(url) | ||
| } catch (e: Exception) { | ||
| throw InvalidBundleSourceException( | ||
| app.getString(R.string.source_invalid_url), | ||
| e | ||
| ) | ||
| } | ||
|
|
||
| // Ensure it's a remote source | ||
| if (source !is SourceInfo.Remote) { | ||
| throw InvalidBundleSourceException( | ||
| app.getString(R.string.source_invalid_url) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Move this to the caller and make the function accept a SourceInfo.Remote instead.
| // Clean up any partial downloads | ||
| dir.deleteRecursively() | ||
| if (e is CancellationException) throw e | ||
| Log.e(tag, "Failed to validate patch source: $url", e) | ||
| throw InvalidBundleSourceException( | ||
| app.getString(R.string.source_add_fail, e.simpleMessage()), | ||
| e | ||
| ) |
There was a problem hiding this comment.
This catch block definitely needs to be tested. dispatchAction does not block and exceptions thrown in it will not be propagated to the caller (they have their own handler).
| versionHash = with(src) { downloadLatest() } | ||
| } catch (e: Exception) { | ||
| // Clean up any partial downloads | ||
| dir.deleteRecursively() |
There was a problem hiding this comment.
I don't think this cleanup logic is sufficient. The app process might crash or be suspended during the download if it takes a long time, and that would leave those files on disk. This wasn't a problem before, since the user could just delete the bad bundle or reattempt the download.
| suspend fun createRemote(url: String, autoUpdate: Boolean) { | ||
| // Validate URL format first | ||
| val source = try { | ||
| SourceInfo.from(url) |
There was a problem hiding this comment.
Why not create a remote bundle directly? Since the input has to be a url anyway and isn't supposed to create other bundle types.
4 participants
Summary
Test plan
https://github.com/non_existent_user/repo)Closes #2792