Warn and block outdated Dolphin launches - #314
Open
patchzyy wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughChangesDolphin version and launch update
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LaunchDolphin
participant DolphinVersionService
participant YesNoWindow
participant ILinuxDolphinInstaller
LaunchDolphin->>DolphinVersionService: CheckConfiguredDolphin()
DolphinVersionService-->>LaunchDolphin: return status and version
LaunchDolphin->>YesNoWindow: prompt for outdated version
YesNoWindow-->>LaunchDolphin: return update or play-anyway choice
LaunchDolphin->>ILinuxDolphinInstaller: UpdateFlatpakDolphin(appId, progress)
ILinuxDolphinInstaller-->>LaunchDolphin: return update result
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@WheelWizard/Features/DolphinInstaller/DolphinVersion.cs`:
- Around line 89-90: Update the development-build handling in GetStatus to use
int.TryParse instead of int.Parse; return DolphinVersionStatus.Unknown when
parsing fails, while preserving the minimum-build comparison for valid values.
Add a DolphinVersionTests case covering an oversized development build number
and the expected Unknown status.
In `@WheelWizard/Features/DolphinInstaller/LinuxDolphinInstaller.cs`:
- Around line 108-123: Add direct tests for
LinuxDolphinInstaller.UpdateFlatpakDolphin covering unavailable Flatpak, process
failure, nonzero exit code, and successful execution. Verify the process service
receives “flatpak” with “update -y <appId>” and the same progress instance,
while asserting each expected OperationResult outcome.
In `@WheelWizard/Services/Launcher/Helpers/DolphinLaunchHelper.cs`:
- Around line 198-203: Expose an explicit outcome from
DolphinLaunchHelper.LaunchDolphin and add a reusable version preflight that
callers can run before destructive preparation; return a non-success outcome
when ConfirmDolphinVersionAsync blocks launch. In
WheelWizard/Services/Launcher/Helpers/MiiChannelLaunchHelper.cs:38, run the
preflight before enabling the virtual Wii Remote and avoid retaining Mii Channel
launch state when blocked. In
WheelWizard/Services/Launcher/RrBetaLauncher.cs:58-60 and
WheelWizard/Services/Launcher/RrLauncher.cs:59-61, run the preflight before
killing Dolphin or preparing mods/patches, and return a non-success result when
launch does not proceed.
In `@WheelWizard/Services/PathManager.cs`:
- Around line 690-696: Update ExtractDolphinFlatpakAppId to parse the flatpak
run arguments and return the application-ID operand immediately following run,
rather than selecting the last app-ID-shaped match. Ignore app-ID-shaped values
belonging to options such as --command, preserve the default fallback for
missing or invalid input, and add coverage for commands containing app-ID-shaped
option values.
In `@WheelWizard/Views/Pages/HomePage.axaml.cs`:
- Around line 102-105: Update DolphinButton_OnClick to disable CompleteGrid
before awaiting DolphinLaunchHelper.LaunchDolphin, preventing concurrent launch
flows during prompts or updates. Keep the existing DisableAllButtonsTemporarily
cooldown, and re-enable CompleteGrid in a finally block after that cooldown
completes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ed8310fa-d91b-4995-84ed-10ad47c58053
📒 Files selected for processing (16)
WheelWizard.Test/Features/DolphinVersionTests.csWheelWizard.Test/Features/LinuxDolphinInstallerTests.csWheelWizard/Features/DolphinInstaller/DolphinInstallerExtensions.csWheelWizard/Features/DolphinInstaller/DolphinVersion.csWheelWizard/Features/DolphinInstaller/DolphinVersionService.csWheelWizard/Features/DolphinInstaller/LinuxDolphinInstaller.csWheelWizard/Features/Settings/SettingsManager.csWheelWizard/Resources/Languages/en.ymlWheelWizard/Services/Launcher/Helpers/DolphinLaunchHelper.csWheelWizard/Services/Launcher/Helpers/MiiChannelLaunchHelper.csWheelWizard/Services/Launcher/RrBetaLauncher.csWheelWizard/Services/Launcher/RrLauncher.csWheelWizard/Services/PathManager.csWheelWizard/Views/Pages/HomePage.axaml.csWheelWizard/Views/Popups/DevToolWindow.axaml.csWheelWizard/Views/Popups/Generic/YesNoWindow.axaml.cs
Comment on lines
+89
to
+90
| if (devBuildGroup.Success) | ||
| return int.Parse(devBuildGroup.Value) >= MinimumDevBuild; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target file outline ---'
ast-grep outline WheelWizard/Features/DolphinInstaller/DolphinVersion.cs 2>/dev/null || true
printf '%s\n' '--- target implementation ---'
sed -n '1,180p' WheelWizard/Features/DolphinInstaller/DolphinVersion.cs
printf '%s\n' '--- related call sites and tests ---'
rg -n -C 5 'GetStatus|CheckConfiguredDolphin|DolphinVersionTests|MinimumDevBuild|devBuildGroup' . \
-g '*.cs' -g '*.csproj' -g '*.sln' || trueRepository: TeamWheelWizard/WheelWizard
Length of output: 27609
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
import re
pattern = re.compile(r'(?<![\w.])(?P<yearMonth>\d{4})(?P<revision>[a-z]?)(?:-(?P<devBuild>\d+))?(?![\w.])')
text = '2606-999999999999999999999'
match = pattern.search(text)
value = match.group('devBuild') if match else None
print(f'input={text!r}')
print(f'match={match.group(0)!r}')
print(f'devBuild={value!r}')
print(f'digits={len(value)}')
print(f'Int32.MaxValue={2**31 - 1}')
print(f'fits_Int32={value is not None and int(value) <= 2**31 - 1}')
PY
printf '%s\n' '--- service contract and call path ---'
cat -n WheelWizard/Features/DolphinInstaller/DolphinVersionService.cs | sed -n '1,52p'
cat -n WheelWizard/Services/Launcher/Helpers/DolphinLaunchHelper.cs | sed -n '125,145p'
printf '%s\n' '--- relevant test file ---'
cat -n WheelWizard.Test/Features/DolphinVersionTests.cs | sed -n '1,90p'Repository: TeamWheelWizard/WheelWizard
Length of output: 6970
Handle oversized development build numbers without throwing.
VersionPattern accepts unbounded digits, but int.Parse throws OverflowException for values such as 2606-999999999999999999999. CheckConfiguredDolphin does not catch exceptions from GetStatus, despite its never-throws contract. Use int.TryParse and return DolphinVersionStatus.Unknown when parsing fails. Add this case to DolphinVersionTests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@WheelWizard/Features/DolphinInstaller/DolphinVersion.cs` around lines 89 -
90, Update the development-build handling in GetStatus to use int.TryParse
instead of int.Parse; return DolphinVersionStatus.Unknown when parsing fails,
while preserving the minimum-build comparison for valid values. Add a
DolphinVersionTests case covering an oversized development build number and the
expected Unknown status.
Comment on lines
+108
to
+123
| public async Task<OperationResult> UpdateFlatpakDolphin(string appId, IProgress<int>? progress = null) | ||
| { | ||
| if (!IsFlatpakInstalled()) | ||
| return Fail("Flatpak is not available, so Dolphin cannot be updated automatically."); | ||
|
|
||
| // No installation scope flag on purpose: Flatpak resolves whichever installation actually | ||
| // holds the app. A system-wide install may still refuse without elevation, which surfaces | ||
| // as a non-zero exit code and lets the caller fall back to the download page. | ||
| var updateResult = await processService.RunWithProgressAsync("flatpak", $"update -y {appId}", progress); | ||
| if (updateResult.IsFailure) | ||
| return updateResult.Error; | ||
|
|
||
| if (updateResult.Value != 0) | ||
| return Fail($"Dolphin update failed with exit code {updateResult.Value}."); | ||
|
|
||
| return Ok(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add direct tests for UpdateFlatpakDolphin.
The supplied LinuxDolphinInstallerTests class has no coverage for this new operation. Add tests for unavailable Flatpak, process failure, nonzero exit code, and successful execution. Assert that the command is flatpak update -y <appId> and that the progress instance is forwarded.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@WheelWizard/Features/DolphinInstaller/LinuxDolphinInstaller.cs` around lines
108 - 123, Add direct tests for LinuxDolphinInstaller.UpdateFlatpakDolphin
covering unavailable Flatpak, process failure, nonzero exit code, and successful
execution. Verify the process service receives “flatpak” with “update -y
<appId>” and the same progress instance, while asserting each expected
OperationResult outcome.
Comment on lines
198
to
+203
| // Make sure all file arguments are absolute paths | ||
| public static void LaunchDolphin(string arguments = "", bool shellExecute = false) | ||
| public static async Task LaunchDolphin(string arguments = "", bool shellExecute = false) | ||
| { | ||
| if (!await ConfirmDolphinVersionAsync()) | ||
| return; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Expose the version-gate outcome before launcher preparation.
LaunchDolphin can return without starting Dolphin when the user dismisses the outdated-version prompt or starts an update. It returns Task, so callers cannot distinguish that case from a successful launch. The Retro Rewind launchers then return Ok() after they already killed Dolphin and modified launch state.
WheelWizard/Services/Launcher/Helpers/DolphinLaunchHelper.cs#L198-L203: return an explicit launch outcome and provide a version preflight that callers can run before destructive preparation.WheelWizard/Services/Launcher/Helpers/MiiChannelLaunchHelper.cs#L38-L38: run the preflight before enabling the virtual Wii Remote. Do not retain Mii Channel launch state when the launch is blocked.WheelWizard/Services/Launcher/RrBetaLauncher.cs#L58-L60: run the preflight before killing Dolphin or preparing mods. Return a non-success result when launch does not proceed.WheelWizard/Services/Launcher/RrLauncher.cs#L59-L61: run the preflight before killing Dolphin or preparing patches. Return a non-success result when launch does not proceed.
📍 Affects 4 files
WheelWizard/Services/Launcher/Helpers/DolphinLaunchHelper.cs#L198-L203(this comment)WheelWizard/Services/Launcher/Helpers/MiiChannelLaunchHelper.cs#L38-L38WheelWizard/Services/Launcher/RrBetaLauncher.cs#L58-L60WheelWizard/Services/Launcher/RrLauncher.cs#L59-L61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@WheelWizard/Services/Launcher/Helpers/DolphinLaunchHelper.cs` around lines
198 - 203, Expose an explicit outcome from DolphinLaunchHelper.LaunchDolphin and
add a reusable version preflight that callers can run before destructive
preparation; return a non-success outcome when ConfirmDolphinVersionAsync blocks
launch. In WheelWizard/Services/Launcher/Helpers/MiiChannelLaunchHelper.cs:38,
run the preflight before enabling the virtual Wii Remote and avoid retaining Mii
Channel launch state when blocked. In
WheelWizard/Services/Launcher/RrBetaLauncher.cs:58-60 and
WheelWizard/Services/Launcher/RrLauncher.cs:59-61, run the preflight before
killing Dolphin or preparing mods/patches, and return a non-success result when
launch does not proceed.
Comment on lines
+690
to
+696
| public static string ExtractDolphinFlatpakAppId(string flatpakDolphinLocation) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(flatpakDolphinLocation)) | ||
| return DefaultDolphinFlatpakAppId; | ||
|
|
||
| var matches = Regex.Matches(flatpakDolphinLocation, @"(?i)\b[a-z][a-z0-9]*(?:\.[a-z_][a-z0-9_]*){1,}\.[a-z_][a-z0-9_-]*\b"); | ||
| return matches.Count == 0 ? DefaultDolphinFlatpakAppId : matches[^1].Value; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Extract the application ID from the flatpak run argument position.
Line 696 selects the last app-ID-shaped value in the complete command. For example, flatpak run org.DolphinEmu.dolphin-emu --command=org.example.Tool returns org.example.Tool, not the Dolphin application ID.
This causes flatpak info and flatpak update to target the wrong application. Version detection then returns Unknown and allows a vulnerable Dolphin launch. Parse the flatpak run arguments and select its application-ID operand. Add cases with app-ID-shaped option values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@WheelWizard/Services/PathManager.cs` around lines 690 - 696, Update
ExtractDolphinFlatpakAppId to parse the flatpak run arguments and return the
application-ID operand immediately following run, rather than selecting the last
app-ID-shaped match. Ignore app-ID-shaped values belonging to options such as
--command, preserve the default fallback for missing or invalid input, and add
coverage for commands containing app-ID-shaped option values.
Comment on lines
+102
to
105
| private async void DolphinButton_OnClick(object? sender, RoutedEventArgs e) | ||
| { | ||
| DolphinLaunchHelper.LaunchDolphin(); | ||
| await DolphinLaunchHelper.LaunchDolphin(); | ||
| DisableAllButtonsTemporarily(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Disable launch controls before awaiting the launch flow.
The button remains enabled during the version prompt and Flatpak update. Multiple clicks can start duplicate launch flows.
Disable CompleteGrid before await DolphinLaunchHelper.LaunchDolphin(). Re-enable it in finally after the existing cooldown.
Proposed fix
private async void DolphinButton_OnClick(object? sender, RoutedEventArgs e)
{
- await DolphinLaunchHelper.LaunchDolphin();
- DisableAllButtonsTemporarily();
+ CompleteGrid.IsEnabled = false;
+ try
+ {
+ await DolphinLaunchHelper.LaunchDolphin();
+ }
+ finally
+ {
+ DisableAllButtonsTemporarily();
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private async void DolphinButton_OnClick(object? sender, RoutedEventArgs e) | |
| { | |
| DolphinLaunchHelper.LaunchDolphin(); | |
| await DolphinLaunchHelper.LaunchDolphin(); | |
| DisableAllButtonsTemporarily(); | |
| private async void DolphinButton_OnClick(object? sender, RoutedEventArgs e) | |
| { | |
| CompleteGrid.IsEnabled = false; | |
| try | |
| { | |
| await DolphinLaunchHelper.LaunchDolphin(); | |
| } | |
| finally | |
| { | |
| DisableAllButtonsTemporarily(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@WheelWizard/Views/Pages/HomePage.axaml.cs` around lines 102 - 105, Update
DolphinButton_OnClick to disable CompleteGrid before awaiting
DolphinLaunchHelper.LaunchDolphin, preventing concurrent launch flows during
prompts or updates. Keep the existing DisableAllButtonsTemporarily cooldown, and
re-enable CompleteGrid in a finally block after that cooldown completes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose of this PR:
Force users to use newest dolphin version to avoid the Dolphin DSP-HLE Guest-to-Host RCE
How to Test:
Dolphin dolphin version 2606, link it to wheelwizard and try to launch your game, you should get a popup warning that this version is vulnerable, updating to 2606a should not give this popup
What Has Been Changed:
A new DolphinVersionService has been created
Related Issue Link:
none
Checklist before merging
Summary by CodeRabbit