Major Overhaul – Improved Bypass Methods, File Structure, and Robustness#4
Merged
Conversation
Bug: If the Minecraft client exits the game, the launcher.targets sees this as a fallback and launches the next available script.
More info:
param : The term 'param' is not recognized as the name of a cmdlet, function, script file, or operable program. Check
the spelling of the name, or if a path was included, verify that the path is correct and try again.
At \portablemc-5.0.2-windows-x86_64-msvc\Scripts\Launcher.ps1:3 char:1
+ param(
+ ~~~~~
+ CategoryInfo : ObjectNotFound: (param:String) [], CommandNotFoundException
+ FullyQualifiedErrorId : CommandNotFoundException
Actual Proper README.md for this repository!
Contributor
Reviewer's GuideOverhauls the Windows-focused launcher into a %LOCALAPPDATA%\PortableMC-based runtime with a menu-driven bootstrapper, robust MSBuild/C#/PowerShell/VBScript fallback chain, improved SSL handling, reliable junction creation, and CI/docs updates for Ruff-based automation and the new architecture. Sequence diagram for web launcher bootstrap and PortableMC startsequenceDiagram
actor User
participant main_py as main_py
participant EmbeddedPython as embedded_python
participant SystemPython as system_python
participant portablemc_py as portablemc_py
participant PortableMC as portablemc_runtime
User->>main_py: run main.py
main_py->>main_py: initialize_runtime_configuration
User->>main_py: select option 1 (web launcher)
main_py->>main_py: prepare_user_data
main_py->>EmbeddedPython: setup_embedded_python
alt embedded Python usable
EmbeddedPython-->>main_py: ok
main_py->>EmbeddedPython: install_base_packages
main_py->>EmbeddedPython: ensure_portablemc
EmbeddedPython-->>main_py: method(binary|uvx|module)
main_py->>portablemc_py: launch_launcher(method, EMBEDDED_PYTHON)
main_py->>portablemc_py: env LAUNCHER_ROOT, BASE_DIR, __COMPAT_LAYER
portablemc_py->>PortableMC: start via method
PortableMC-->>portablemc_py: game output
portablemc_py-->>User: web console via Flask_SocketIO
else embedded Python blocked or fails
EmbeddedPython-->>main_py: failure
main_py->>SystemPython: get_system_python
SystemPython-->>main_py: path or None
alt system Python found
main_py->>SystemPython: install BASE_PACKAGES and portablemc
main_py->>portablemc_py: launch_launcher(method, SystemPython)
portablemc_py->>PortableMC: start via method
PortableMC-->>portablemc_py: game output
portablemc_py-->>User: web console
else no system Python
main_py-->>User: error and failure dump
end
end
Sequence diagram for MSBuild launcher and C#/PowerShell/VBScript fallback chainsequenceDiagram
actor User
participant main_py as main_py
participant MSBuild as MSBuild_exe
participant Targets as Launcher_targets
participant CSharp as PortableMCLoader_exe
participant PS as Launcher_ps1
participant VBS as Launcher_vbs
participant PortableMC as portablemc_exe
User->>main_py: run main.py
main_py->>main_py: initialize_runtime_configuration
User->>main_py: select option 2 (MSBuild launcher)
main_py->>main_py: prepare_user_data
main_py->>MSBuild: run Launcher.targets
MSBuild->>Targets: load targets and properties
alt C# loader enabled and succeeds
Targets->>CSharp: compile and run PortableMCLoader.exe
CSharp->>PortableMC: download and extract portablemc.exe
CSharp->>PortableMC: launch with JVM options
PortableMC-->>CSharp: exit code 0 or detach
CSharp-->>Targets: success
Targets-->>MSBuild: stop further fallbacks
MSBuild-->>main_py: success
else C# loader fails or disabled
Targets-->>MSBuild: C# failed
alt VBScript succeeds
Targets->>VBS: run Launcher.vbs
VBS->>PortableMC: download and launch portablemc.exe
PortableMC-->>VBS: exit code
VBS-->>Targets: success
Targets-->>MSBuild: stop after VBScript
MSBuild-->>main_py: success
else VBScript fails
Targets-->>MSBuild: VBScript failed
alt PowerShell succeeds
Targets->>PS: run Launcher.ps1
PS->>PortableMC: download and launch portablemc.exe or python module
PortableMC-->>PS: exit code
PS-->>Targets: success
Targets-->>MSBuild: stop after PowerShell
MSBuild-->>main_py: success
else all methods fail
Targets-->>MSBuild: failure
MSBuild-->>main_py: nonzero exit code
main_py->>main_py: write_failure_dump(kind msbuild)
main_py-->>User: error and log path
end
end
end
Flow diagram for runtime trusted directory selection and config syncflowchart TD
A["start_main_py"] --> B["load_launcher_config_json_if_exists"]
B --> C["merge_with_DEFAULT_CONFIG"]
C --> D["compute_builtin_trusted_candidates"]
D --> E["merge_allowlist_trusted_dirs_from_config"]
E --> F["for_each_candidate_probe_directory_access"]
F --> G{candidate_has_create_write_read_execute}
G -->|yes| H["select_candidate_as_ACTIVE_TRUSTED_DIR"]
G -->|no| F
H --> I["set_BASE_DIR_to_ACTIVE_TRUSTED_DIR"]
I --> J["update_launcher_state_with_trusted_dir_and_probes"]
J --> K["sync_config_to_trusted_dir"]
K --> L{try_create_symlink_for_launcher_config_json}
L -->|symlink_ok| M["config_sync_mode_symlink"]
L -->|symlink_failed_copy_ok| N["config_sync_mode_copy"]
L -->|both_failed| O["config_sync_mode_disabled"]
M --> P["write_final_launcher_state_to_config"]
N --> P
O --> P
P --> Q["continue_to_launcher_menu"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 23 security issues, 6 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'. (link)
- Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'. (link)
- Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'. (link)
- Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- Several places deliberately disable SSL certificate verification (Python
ssl._create_unverified_context, C#ServerCertificateValidationCallback = ... => true, VBS/PowerShell fallbacks); consider centralizing this behind an explicit configuration flag or environment variable and logging when it is used so you can audit or disable the insecure path more easily in production. - The junction creation and PortableMC bootstrap logic is duplicated across Python, PowerShell, VBScript, and C# loaders with slightly different behaviors; it may be worth defining a single reference behavior (e.g., in Python) and aligning the scripts to that contract to avoid subtle drift (for example, around when deletion of existing targets is allowed and what happens on failure).
- The new
get_system_pythonlogic hard‑codes a large set of Windows paths and versions; consider extracting these constants and using a clearer priority order (e.g., current interpreter, PATH, registry, then common locations) to make future maintenance or version bumps less error‑prone.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several places deliberately disable SSL certificate verification (Python `ssl._create_unverified_context`, C# `ServerCertificateValidationCallback = ... => true`, VBS/PowerShell fallbacks); consider centralizing this behind an explicit configuration flag or environment variable and logging when it is used so you can audit or disable the insecure path more easily in production.
- The junction creation and PortableMC bootstrap logic is duplicated across Python, PowerShell, VBScript, and C# loaders with slightly different behaviors; it may be worth defining a single reference behavior (e.g., in Python) and aligning the scripts to that contract to avoid subtle drift (for example, around when deletion of existing targets is allowed and what happens on failure).
- The new `get_system_python` logic hard‑codes a large set of Windows paths and versions; consider extracting these constants and using a clearer priority order (e.g., current interpreter, PATH, registry, then common locations) to make future maintenance or version bumps less error‑prone.
## Individual Comments
### Comment 1
<location path="main.py" line_range="130-139" />
<code_context>
+def create_junction(source, target):
</code_context>
<issue_to_address>
**issue (bug_risk):** Junction detection is unreliable and may delete the contents of the source directory.
On Windows, directory junctions are usually reported as `is_dir()` but not `is_symlink()`. With the current logic, a junction at `target_path` will go through the `is_dir()` branch, and `shutil.rmtree(target_path)` will delete the *target directory’s contents*—which, for a junction, are actually the contents of `source_path`.
Since you call `create_junction(ROOT_DIR / "mods", base_dir / "mods")`, this could erase the original `mods` contents if that path is already a junction.
To avoid this:
- Detect junctions explicitly (e.g. via `os.lstat()` and the reparse tag, or `ctypes`/`win32file`), or
- Treat an existing directory at `target_path` as a link to unlink with `os.rmdir(target_path)` instead of recursively deleting it.
In particular, `shutil.rmtree` should not be used on a path that might be a junction.
</issue_to_address>
### Comment 2
<location path=".github/workflows/main.yml" line_range="16-17" />
<code_context>
jobs:
ruff:
runs-on: ubuntu-latest
+ # Skip if the commit already contains [skip ci]
+ if: "!contains(github.event.head_commit.message, '[skip ci]')"
steps:
- name: Checkout code
</code_context>
<issue_to_address>
**issue (bug_risk):** Workflow `if` condition may fail on non-push events due to `github.event.head_commit` being undefined.
`github.event.head_commit.message` is only set for some events (e.g. `push`), and is `null` for `pull_request`, which can break the `contains(...)` evaluation.
To make this safe:
- Either scope this condition to push events only, e.g.
```yaml
if: |
github.event_name != 'push' ||
!contains(github.event.head_commit.message, '[skip ci]')
```
- Or, for push events, use a commits-based expression like `join(github.event.commits.*.message, ' ')` and gate it with `github.event_name == 'push'`.
</issue_to_address>
### Comment 3
<location path="main.py" line_range="262" />
<code_context>
-def download_get_pip():
+
+def download_get_pip(python_exe=None):
+ """Download get-pip.py into the embedded Python directory."""
+ if python_exe is None:
</code_context>
<issue_to_address>
**suggestion:** The `python_exe` parameter is unused in `download_get_pip`.
The new signature implies interpreter-specific behavior, but the parameter is never used, which is misleading now that `install_pip` passes it through. Either make `python_exe` affect the behavior (for example, derive the target directory from it instead of `EMBEDDED_DIR`) or remove the parameter and keep this function interpreter-agnostic.
Suggested implementation:
```python
def download_get_pip():
"""Download get-pip.py into the embedded Python directory."""
pip_script = EMBEDDED_DIR / "get-pip.py"
```
Since `download_get_pip` is no longer parameterized, all call sites that pass `python_exe` (e.g. `download_get_pip(python_exe)`) must be updated to call `download_get_pip()` with no arguments to keep the function interpreter-agnostic as per the review comment.
</issue_to_address>
### Comment 4
<location path="scripts/Launcher.ps1" line_range="9-10" />
<code_context>
+ [string]$JvmOpts
+)
+
+# Fallback to positional arguments if needed
+if (-not $JvmOpts -and $args.Count -ge 0) {
+ $JvmOpts = $args[0]
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Positional JvmOpts fallback condition is always true for `$args.Count -ge 0` and can overwrite a valid value.
Because `$args.Count -ge 0` is always true, this condition is effectively just `if (-not $JvmOpts) { ... }` and will access `$args[0]` even when no arguments are passed, potentially throwing or setting `$JvmOpts` to `$null`. To only use a positional fallback when at least one argument exists, use:
```powershell
if (-not $JvmOpts -and $args.Count -gt 0) {
$JvmOpts = $args[0]
}
```
</issue_to_address>
### Comment 5
<location path="README.md" line_range="3" />
<code_context>
-* EXE Code compilation
-* Powershell Script Code as fallback
-* VBS Script as fallback
+A Python‑based Minecraft launcher that runs in a browser using [Flask](https://github.com/pallets/flask) and [PortableMC](https://github.com/mindstorm38/portablemc). Designed for restricted Windows environments where arbitrary `.exe` files are blocked. It uses multiple fallback mechanisms (C# compilation, PowerShell, VBScript) to bypass group policy restrictions.
</code_context>
<issue_to_address>
**suggestion (typo):** Consider capitalizing "Group Policy" as it is a proper Microsoft term.
This matches Microsoft’s official naming of the Windows feature: “Group Policy.”
```suggestion
A Python‑based Minecraft launcher that runs in a browser using [Flask](https://github.com/pallets/flask) and [PortableMC](https://github.com/mindstorm38/portablemc). Designed for restricted Windows environments where arbitrary `.exe` files are blocked. It uses multiple fallback mechanisms (C# compilation, PowerShell, VBScript) to bypass Group Policy restrictions.
```
</issue_to_address>
### Comment 6
<location path="README.md" line_range="91" />
<code_context>
+* [PortableMC](https://github.com/mindstorm38/portablemc) – The Heart of The Launcher
+* [Flask](https://flask.palletsprojects.com/) & [Flask‑SocketIO](https://flask-socketio.readthedocs.io/) – Web Interface
+* [ansi2html](https://github.com/ralphbean/ansi2html) & [ansi_up](https://github.com/drudru/ansi_up) – ANSI colour conversion
+* [Socket.io](https://socket.io) - Communication across Web and Flask Interface
+* [Ruff Linter](https://github.com/astral-sh/ruff) - An extremely fast Python linter
</code_context>
<issue_to_address>
**nitpick (typo):** Tighten the phrasing to improve grammar and clarity in this description.
Consider rephrasing this to "Communication between the web and Flask interfaces" to add the missing articles and correct pluralization while preserving the original meaning.
```suggestion
* [Socket.io](https://socket.io) - Communication between the web and Flask interfaces
```
</issue_to_address>
### Comment 7
<location path="main.py" line_range="242-247" />
<code_context>
result = subprocess.run(
[str(EMBEDDED_PYTHON), "--version"],
capture_output=True,
text=True,
timeout=5,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 8
<location path="main.py" line_range="243" />
<code_context>
[str(EMBEDDED_PYTHON), "--version"],
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-tainted-env-args):** Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'.
*Source: opengrep*
</issue_to_address>
### Comment 9
<location path="main.py" line_range="293" />
<code_context>
result = subprocess.run(cmd, env=env, capture_output=True, text=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-tainted-env-args):** Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'.
*Source: opengrep*
</issue_to_address>
### Comment 10
<location path="main.py" line_range="318" />
<code_context>
result = subprocess.run(cmd, env=env, capture_output=True, text=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-tainted-env-args):** Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'.
*Source: opengrep*
</issue_to_address>
### Comment 11
<location path="main.py" line_range="353-359" />
<code_context>
result = subprocess.run(
[str(python_exe), "-c", "import certifi; print(certifi.where())"],
capture_output=True,
text=True,
check=True,
env={"PYTHONNOUSERSITE": "1"},
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 12
<location path="main.py" line_range="354" />
<code_context>
[str(python_exe), "-c", "import certifi; print(certifi.where())"],
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-tainted-env-args):** Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'.
*Source: opengrep*
</issue_to_address>
### Comment 13
<location path="main.py" line_range="424-430" />
<code_context>
result = subprocess.run(
[str(binary_path), "--help"],
env=env,
capture_output=True,
text=True,
timeout=5,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 14
<location path="main.py" line_range="425" />
<code_context>
[str(binary_path), "--help"],
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-tainted-env-args):** Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'.
*Source: opengrep*
</issue_to_address>
### Comment 15
<location path="main.py" line_range="445" />
<code_context>
result = subprocess.run(cmd, env=env, capture_output=True, text=True, timeout=5)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-tainted-env-args):** Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'.
*Source: opengrep*
</issue_to_address>
### Comment 16
<location path="main.py" line_range="485-487" />
<code_context>
result = subprocess.run(
[str(current), "--version"], capture_output=True, text=True, timeout=2
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 17
<location path="main.py" line_range="542-544" />
<code_context>
result = subprocess.run(
[str(p), "--version"], capture_output=True, text=True, timeout=2
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 18
<location path="main.py" line_range="594" />
<code_context>
subprocess.run(cmd, env=env, check=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-tainted-env-args):** Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'.
*Source: opengrep*
</issue_to_address>
### Comment 19
<location path="main.py" line_range="620-625" />
<code_context>
pip_check = subprocess.run(
[str(EMBEDDED_PYTHON), "-m", "pip", "--version"],
env=env_check,
capture_output=True,
text=True,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 20
<location path="main.py" line_range="621" />
<code_context>
[str(EMBEDDED_PYTHON), "-m", "pip", "--version"],
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-tainted-env-args):** Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'.
*Source: opengrep*
</issue_to_address>
### Comment 21
<location path="main.py" line_range="665" />
<code_context>
result = subprocess.run(cmd, env=env, capture_output=True, text=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 22
<location path="main.py" line_range="705" />
<code_context>
subprocess.run(cmd, env=env, check=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 23
<location path="main.py" line_range="730-735" />
<code_context>
pip_check = subprocess.run(
[str(EMBEDDED_PYTHON), "-m", "pip", "--version"],
env=env_check,
capture_output=True,
text=True,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 24
<location path="main.py" line_range="731" />
<code_context>
[str(EMBEDDED_PYTHON), "-m", "pip", "--version"],
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-tainted-env-args):** Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'.
*Source: opengrep*
</issue_to_address>
### Comment 25
<location path="main.py" line_range="782" />
<code_context>
subprocess.run(cmd, env=env, cwd=BASE_DIR, check=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 26
<location path="main.py" line_range="782" />
<code_context>
subprocess.run(cmd, env=env, cwd=BASE_DIR, check=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-tainted-env-args):** Detected subprocess function 'run' with user controlled data. A malicious actor could leverage this to perform command injection. You may consider using 'shlex.quote()'.
*Source: opengrep*
</issue_to_address>
### Comment 27
<location path="main.py" line_range="808" />
<code_context>
result = subprocess.run(cmd, env=env, capture_output=True, text=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 28
<location path="main.py" line_range="841" />
<code_context>
subprocess.run(cmd, env=env, cwd=BASE_DIR, check=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 29
<location path="scripts/portablemc.py" line_range="1148-1152" />
<code_context>
process = subprocess.Popen(
cmd,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
shell=False,
text=True,
encoding="utf-8",
errors="replace",
bufsize=1,
creationflags=subprocess.CREATE_NO_WINDOW if os.name == "nt" else 0,
startupinfo=startupinfo,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Updated the Launcher.targets file to include conditional execution for PowerShell and VBScript fallbacks, and added properties to control their usage.
See pull request for more details
Contributor
|
Linting and formatting changes were found. Please run |
Added a Debug Menu option and updated folder structure details.
Owner
Author
|
What I changed Added runtime trusted-dir probing in main.py with built-in candidates + config allowlist merge. Added root config: launcher_config.json in project root. Dumps now write to ./logs (project folder), not %LOCALAPPDATA%\PortableMC\logs. Added run_uv_install(...) and install_packages_with_fallback(...). Added hidden menu option 4 in main.py (accepted even though not printed in main menu text). Rewrote README.md with: |
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
run_debug_menu, the conditionelif choice == "b" or "q":is always truthy due to the bare string, so the menu will exit on any non‑handled input; this should beelif choice in ("b", "q"):(or equivalent) to behave as intended. - The
ensure_junctions()helper recomputesbase_dirfrom%LOCALAPPDATA%\PortableMCinstead of using the dynamically resolvedBASE_DIR/trusted directory, which can desynchronize mods/resourcepacks from the actual runtime location; consider wiring it to the globalBASE_DIRso all launch modes share the same target. - The diagnostic dumps in
write_failure_dump()include identifiers likeUSERNAME,USERDOMAIN, andCOMPUTERNAME, which may be sensitive in some environments; if these logs are meant to be shared externally, consider redacting or making inclusion of these fields configurable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `run_debug_menu`, the condition `elif choice == "b" or "q":` is always truthy due to the bare string, so the menu will exit on any non‑handled input; this should be `elif choice in ("b", "q"):` (or equivalent) to behave as intended.
- The `ensure_junctions()` helper recomputes `base_dir` from `%LOCALAPPDATA%\PortableMC` instead of using the dynamically resolved `BASE_DIR`/trusted directory, which can desynchronize mods/resourcepacks from the actual runtime location; consider wiring it to the global `BASE_DIR` so all launch modes share the same target.
- The diagnostic dumps in `write_failure_dump()` include identifiers like `USERNAME`, `USERDOMAIN`, and `COMPUTERNAME`, which may be sensitive in some environments; if these logs are meant to be shared externally, consider redacting or making inclusion of these fields configurable.
## Individual Comments
### Comment 1
<location path="main.py" line_range="474-478" />
<code_context>
+ print("ℹ️ No trusted directory selected.")
+ elif choice == "5":
+ print(json.dumps(LAUNCHER_STATE, indent=2, sort_keys=True))
+ elif choice == "6":
+ selected, candidates, probes = _resolve_trusted_dir(LAUNCHER_STATE)
+ print(f"Selected trusted dir: {selected}")
+ print(json.dumps({str(k): v for k, v in probes.items()}, indent=2))
+ elif choice == "b" or "q":
+ return
+ else:
</code_context>
<issue_to_address>
**issue (bug_risk):** The `elif choice == "b" or "q"` condition is always truthy, so the debug menu will always exit after one iteration.
In Python this is evaluated as `(choice == "b") or "q"`, and any non-empty string is truthy. As a result, this branch runs for all inputs not matched above, so the `else` block never executes and the menu always exits. Use `elif choice in ("b", "q"):` or `elif choice == "b" or choice == "q":` instead.
</issue_to_address>
### Comment 2
<location path="main.py" line_range="403-410" />
<code_context>
+ return False
+
+
+def ensure_junctions():
+ r"""Ensure mods and resourcepacks directories exist in %LOCALAPPDATA%\PortableMC."""
+ appdata = Path(os.environ.get("LOCALAPPDATA", Path.home() / "AppData/Local"))
+ base_dir = appdata / "PortableMC"
+ base_dir.mkdir(parents=True, exist_ok=True)
+
+ create_junction(ROOT_DIR / "mods", base_dir / "mods")
+ create_junction(ROOT_DIR / "resourcepacks", base_dir / "resourcepacks")
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Junctions are created under `%LOCALAPPDATA%\PortableMC` even when the active base/trusted directory has been switched.
`initialize_runtime_configuration()` may change `BASE_DIR` via `_set_runtime_base_dir()`, but `ensure_junctions()` recomputes `base_dir` from `LOCALAPPDATA` instead of using the active base/trusted directory. This can split resources across two locations. Consider deriving the junction targets from the current `BASE_DIR`/`ACTIVE_TRUSTED_DIR`, or, if this separation is intentional, make it explicit (e.g., via naming or comments).
```suggestion
def ensure_junctions():
r"""Ensure mods and resourcepacks directories exist in the active base/trusted directory.
The junction targets are derived from the current runtime base directory so that
resources are not split across multiple locations when the base/trusted directory
is changed at runtime.
"""
# Prefer the active trusted directory if set, otherwise fall back to BASE_DIR.
base_dir = ACTIVE_TRUSTED_DIR or BASE_DIR
base_dir.mkdir(parents=True, exist_ok=True)
create_junction(ROOT_DIR / "mods", base_dir / "mods")
create_junction(ROOT_DIR / "resourcepacks", base_dir / "resourcepacks")
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
PythonChicken123
added
bug
Owner
Author
|
@sourcery-ai dismiss |
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- main.py has grown into a ~1600‑line monolith that mixes configuration, environment probing, package installation, launcher orchestration, and debug UI; consider extracting cohesive pieces (e.g. trusted-dir resolution, Python/portablemc setup, MSBuild runner, CLI/Web launchers) into separate modules or helper classes for maintainability.
- The logic for installing and invoking portablemc (embedded vs system Python, uv/pip fallback, certifi setup, junction management) is partially duplicated between run_web_launcher, run_cli_launcher, and install_portablemc_via_embedded; factoring this into shared helper functions would reduce drift and make future changes to the bootstrap flow less error‑prone.
- Insecure SSL handling is implemented separately in Python, C#, PowerShell, and VBScript with slightly different environment checks (e.g. variations of ALLOW_INSECURE_SSL values); it may be worth standardizing the accepted values and documenting them in a single place to avoid subtle inconsistencies between launch paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- main.py has grown into a ~1600‑line monolith that mixes configuration, environment probing, package installation, launcher orchestration, and debug UI; consider extracting cohesive pieces (e.g. trusted-dir resolution, Python/portablemc setup, MSBuild runner, CLI/Web launchers) into separate modules or helper classes for maintainability.
- The logic for installing and invoking portablemc (embedded vs system Python, uv/pip fallback, certifi setup, junction management) is partially duplicated between run_web_launcher, run_cli_launcher, and install_portablemc_via_embedded; factoring this into shared helper functions would reduce drift and make future changes to the bootstrap flow less error‑prone.
- Insecure SSL handling is implemented separately in Python, C#, PowerShell, and VBScript with slightly different environment checks (e.g. variations of ALLOW_INSECURE_SSL values); it may be worth standardizing the accepted values and documenting them in a single place to avoid subtle inconsistencies between launch paths.
## Individual Comments
### Comment 1
<location path="README.md" line_range="74-75" />
<code_context>
+4. **VBScript** (via `cscript`).
+
+If a method is blocked, it tries the next. All launchers set __COMPAT_LAYER=RUNASINVOKER to avoid UAC prompts.
+If all fails, the script will leave a log dump which you can use to [open an issue](https://github.com/PythonChicken123/Minecraft-Portable-Web/issues)
+
+All downloaded files (embedded Python, portablemc binary) are stored in `%LOCALAPPDATA%\PortableMC`. Junctions are created for `mods` and `resourcepacks` so that files placed in the project folder appear inside the game.
</code_context>
<issue_to_address>
**suggestion (typo):** Minor grammatical issue in the phrase "If all fails".
Please rephrase "If all fails" to "If all methods fail" or "If all else fails" to fix the grammar and clarify the reference to the listed methods.
```suggestion
If a method is blocked, it tries the next. All launchers set __COMPAT_LAYER=RUNASINVOKER to avoid UAC prompts.
If all methods fail, the script will leave a log dump which you can use to [open an issue](https://github.com/PythonChicken123/Minecraft-Portable-Web/issues)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
run_cli_launcher, the exception handlers reference acmdvariable that is never defined, which will mask the original error; capture and pass the actual command you invoked (e.g.used_cmd) intowrite_failure_dumpinstead. - The new
main.pyhas grown to ~1500 lines and mixes configuration probing, package installation, diagnostics, and launch modes in a single file; consider extracting cohesive pieces (e.g. MSBuild discovery, trusted-dir probing, Python/portablemc install helpers) into separate modules or functions to keep it maintainable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `run_cli_launcher`, the exception handlers reference a `cmd` variable that is never defined, which will mask the original error; capture and pass the actual command you invoked (e.g. `used_cmd`) into `write_failure_dump` instead.
- The new `main.py` has grown to ~1500 lines and mixes configuration probing, package installation, diagnostics, and launch modes in a single file; consider extracting cohesive pieces (e.g. MSBuild discovery, trusted-dir probing, Python/portablemc install helpers) into separate modules or functions to keep it maintainable.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
Linting and formatting changes were found. Please run |
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- main.py has grown into a ~1800‑line monolith combining configuration, environment probing, install logic, and launchers; consider splitting this into smaller modules (e.g., config/state, environment probing, Python/portablemc install, launch modes) to keep responsibilities isolated and easier to maintain.
- The installation and portablemc setup logic for embedded vs system Python is duplicated across web and CLI launch paths; it would be more maintainable to centralize this into shared helper functions that return a configured interpreter + environment.
- Defaults like DEFAULT_USERNAME, DEFAULT_SERVER_IP, and DEFAULT_JVM_OPTS are hard-coded in main.py; consider moving them into a user-editable config file or environment variables so they can be changed without modifying the launcher code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- main.py has grown into a ~1800‑line monolith combining configuration, environment probing, install logic, and launchers; consider splitting this into smaller modules (e.g., config/state, environment probing, Python/portablemc install, launch modes) to keep responsibilities isolated and easier to maintain.
- The installation and portablemc setup logic for embedded vs system Python is duplicated across web and CLI launch paths; it would be more maintainable to centralize this into shared helper functions that return a configured interpreter + environment.
- Defaults like DEFAULT_USERNAME, DEFAULT_SERVER_IP, and DEFAULT_JVM_OPTS are hard-coded in main.py; consider moving them into a user-editable config file or environment variables so they can be changed without modifying the launcher code.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Pull Request: Major Overhaul – Improved Bypass Methods, File Structure, and Robustness
This PR consolidates weeks of development to make the launcher work reliably in restricted Windows environments (AppLocker/Group Policy). It introduces multiple fallback mechanisms, moves all data to
%LOCALAPPDATA%\PortableMC, and adds proper error handling and logging.Summary of Changes
PortableMCLoader.cscompiled on‑the‑fly bycsc.exe.Launcher.vbs), then PowerShell (Launcher.ps1). The MSBuild.targetsfile now correctly stops after a successful method.ssl._create_unverified_context()when downloading.create_junction()functions in bothmain.pyandportablemc.pythat safely remove existing targets and fall back to regular directories if junctions are blocked.%LOCALAPPDATA%\PortableMC. The project folder only contains source code and user folders (mods,resourcepacks).Launcher.targets,Launcher.vbs,Launcher.ps1,PortableMCLoader.cs) into aScripts/subfolder for a cleaner root.prepare_user_data()– Copiesstatic/,options.txt, andservers.datto the AppData folder on first use.exceptstatements with specific exception classes. Added logging throughout..gitignore, a comprehensiveREADME.md, a GitHub Actions workflow (Ruff auto‑fix, triggered only on.pychanges), and issue/discussion templates.Detailed Changes
1. C# Loader (
Scripts/PortableMCLoader.cs)main.py.CreateNoWindow = true).2. MSBuild Target (
Scripts/Launcher.targets)PortableMCLoader.csexists..batfiles – uses directExectasks withEnvironmentVariables.3. Python Entry Point (
main.py)BASE_DIR = %LOCALAPPDATA%\PortableMCandROOT_DIR = script folder.prepare_user_data()copiesstatic/and game files toBASE_DIR.get_system_python()now includes the current interpreter and scans all common locations (Python 3.9–3.14).prepare_user_data()and set__COMPAT_LAYER=RUNASINVOKER.create_junction()removes existing target safely (handles junctions vs. regular folders) and falls back to a regular directory.f'/p:JvmOpts="{DEFAULT_JVM_OPTS}"'.4. Web Launcher (
portablemc.py)LAUNCHER_ROOTfrom environment to find the user's root folder.BASE_DIR/static.BASE_DIR(Minecraft working directory set toBASE_DIR).5. VBScript (
Scripts/Launcher.vbs)__COMPAT_LAYERenvironment variable.rootDirfor junctions.6. PowerShell (
Scripts/Launcher.ps1)$rootDircomputed,$env:__COMPAT_LAYERset.paramerrors).7. GitHub Workflow (
.github/workflows/main.yml).pyfile changes.[skip ci].Testing
How to Test This PR
python main.py.%LOCALAPPDATA%\PortableMC(no clutter in the root folder).mods/and a resource pack inresourcepacks/; they should be visible in game.%LOCALAPPDATA%\PortableMC\portablemc_binand restart – it should re‑download.Breaking Changes
None – this PR replaces the previous launcher entirely. Users should delete their old
python/andportablemc_bin/folders and let the script re‑create them in AppData.Next Steps
v2.0.0for the major overhaul.README.mdwith the new structure.Summary by Sourcery
Overhaul the Windows-focused Minecraft launcher to bootstrap and run PortableMC reliably in restricted environments using a unified AppData-based runtime, multiple fallback launch methods, and improved robustness.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Documentation:
Chores:
Summary by Sourcery
Overhaul the Windows-focused Minecraft launcher to bootstrap and run PortableMC reliably in restricted environments using a unified AppData-based runtime, multiple fallback launch methods, and improved robustness.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Documentation:
Chores: