Download SITL binaries from GitHub #2438

Scavanger · 2025-11-27T14:39:09Z

User description

Get rid of SITL binaries floating that around in the Configurator repro and no more forgotten or outdated SITLs in release ;)

Download and install the appropriate SITL binaries.
Automatic update check (can be disabled)
Automatic download when demo mode is started for the first time
Minor adjustments and improvements to SITL handling

Windows:
Windows:
As long as iNavFlight/inav#11133 has not been merged and the new main release is out, installation will fail. User has to copy a own version of cygwin1.dll to the appData directory. Corresponding note in the GUI log.

PR Type

Enhancement, Bug fix

Description

Implement SITL binary download from GitHub with automatic version management
- Fetch releases from iNavFlight repository with version filtering
- Download and extract binaries for Windows, Linux, and macOS platforms
- Store version information and handle platform-specific executable paths
Add automatic SITL update check on startup with user confirmation
- Check for latest stable release and prompt user to download
- Support disabling update checks via settings option
- Auto-download SITL when demo mode started without installed binary
Refactor SITL process management and file handling
- Move SITL binaries to userData directory instead of repository
- Simplify permission handling by removing chmod IPC calls
- Fix serial receiver default settings in standard profiles
Hide SITL tab and demo mode for 32-bit Windows systems
Add UI controls for SITL version selection and unstable releases toggle

Diagram Walkthrough

flowchart LR
  A["GitHub API"] -->|"Fetch releases"| B["sitl_tools.js"]
  B -->|"Download & extract"| C["userData/sitl/"]
  C -->|"Store version"| D["version file"]
  E["Configurator startup"] -->|"Check version"| F["configurator_main.js"]
  F -->|"Prompt update"| G["User confirmation"]
  G -->|"Download"| B
  H["SITL Tab UI"] -->|"Select version"| I["sitlBinaries dropdown"]
  I -->|"Download"| B
  J["Demo mode"] -->|"Check SITL"| K["serial_backend.js"]
  K -->|"Auto-download if missing"| B

File Walkthrough

Relevant files

Enhancement

10 files

sitl.js `Add SITL binary download UI and version management`	+127/-12
sitl_tools.js `New module for SITL binary download and extraction`	+157/-0
configurator_main.js `Add automatic SITL update check on startup`	+46/-2
serial_backend.js `Auto-download SITL for demo mode if missing`	+46/-11
main.js `Add IPC handlers for SITL download and version queries`	+45/-13
preload.js `Expose new SITL download and version APIs to renderer`	+10/-1
globalSettings.js `Add setting to disable SITL update check`	+1/-0
sitl.html `Add UI section for SITL binary selection and version display`	+35/-0
options.html `Add checkbox to disable SITL update check`	+4/-1
sitl.css `Add styling for SITL binary selection UI elements`	+25/-0

Bug fix

4 files

sitl.js `Refactor SITL paths and remove chmod IPC calls`	+9/-24
port_handler.js `Hide demo mode for 32-bit Windows systems`	+4/-1
child_process.js `Handle file permissions locally instead of IPC`	+17/-2
groundstation.js `Comment out problematic path reference`	+1/-1

Formatting

1 files

ports.js `Minor formatting adjustment`	+1/-0

Configuration changes

1 files

forge.config.js `Enable ASAR packaging for production builds`	+1/-1

Documentation

2 files

messages.json `Add localization strings for SITL download and update features`	+53/-0
README.md `Update build and debug commands to use yarn`	+3/-3

Dependencies

1 files

package.json `Add extract-zip dependency and remove unused fs package`	+2/-2

Additional files

2 files

inav_SITL	[link]
inav_SITL	[link]

qodo-code-review · 2025-11-27T14:39:50Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🔴	Insecure binary download Description: Downloads and writes a binary DLL (`cygwin1.dll`) directly from a personal GitHub repository without integrity verification (no checksum/signature), enabling a supply‑chain attack if the remote content is tampered with. sitl_tools.js [63-73] Referred Code // Until then, we at least have working nightly builds. downloadCygwinDll: async function (path) { const responseDll = await fetch('https://raw.githubusercontent.com/Scavanger/cygwin-test/main/cygwin1.dll'); if (!responseDll.ok) { throw new Error(`Failed to download cygwin1.dll: ${reponse.statusText}`); } const buffer = await responseDll.arrayBuffer(); await writeFile(path, Buffer.from(buffer)); },
	Path handling bug Description: Constructs a filesystem path for an executable using a variable that shadows the imported `path` module and then uses undeclared `commandPath` when checking/chmodding permissions, which can lead to runtime errors and potential spawning of an unintended binary if fallback paths are used. child_process.js [10-24] Referred Code start: function (command, args, opts, window) { var process; try { const path = path.join(app.getPath('userData'), 'sitl', command); if (os.platform() != 'win32') { const stats = fs.statSync(commandPath); const permission = stats.mode & 0o777; if (permission != 0o755) { fs.chmodSync(commandPath, 0o755); } } process = spawn(path, args, opts);
⚪	Unsigned update mechanism Description: Fetches release metadata from GitHub over HTTPS but then downloads ZIP assets and executes contained binaries without any integrity or signature validation, allowing malicious or MITM-substituted artifacts to be installed and run. sitl_tools.js [12-51] Referred Code getSitlReleases: async function name(devRelease, latest) { const response = await fetch(`https://api.github.com/repos/iNavFlight/inav${devRelease ? '-nightly' : ''}/releases?per_page=${latest ? '1' : '50'}`); if (!response.ok) { const message = (await response.json()).message; throw new Error(message \|\| `GitHub Api Error: ${response.statusText}`); } const release = await response.json(); const releases = release.filter(release => semver.gte(release.tag_name, CONFIGURATOR.minfirmwareVersionAccepted) && semver.lt(release.tag_name, CONFIGURATOR.maxFirmwareVersionAccepted)); if (!release \|\| releases.length == 0) { throw new Error("No SITL releases found") } var assets = []; releases.forEach(release => { if (!release.assets) { throw new Error("No assets found in releases for this version.") ... (clipped 19 lines)
	Blocking sync IPC Description: Exposes synchronous IPC calls (`getPlatform`, `getArch`) to renderer via `ipcRenderer.sendSync`, which can block the main process and be abused for minor DoS; should use async `invoke` handlers to avoid main-thread blocking. preload.js [49-50] Referred Code getPlatform: ()=> ipcRenderer.sendSync('getPlatform'), getArch: ()=> ipcRenderer.sendSync('getArch'),
	Misplaced security assumption Description: Enables ASAR packing, which can break dynamic binary path expectations; combined with executable extraction/spawn logic may encourage unpacking to userData but also risks developers assuming ASAR protects assets—ASAR is not a security boundary. forge.config.js [10-11] Referred Code asar: true, icon: 'images/inav',
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🔴	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Broken variable use: Uses undefined `commandPath` when checking permissions and shadows `path` import, causing potential runtime errors and missing chmod on non-Windows. Referred Code const path = path.join(app.getPath('userData'), 'sitl', command); if (os.platform() != 'win32') { const stats = fs.statSync(commandPath); const permission = stats.mode & 0o777; if (permission != 0o755) { fs.chmodSync(commandPath, 0o755); } } Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Insecure download source: `downloadCygwinDll` downloads a DLL from a personal GitHub repository over HTTPS without signature or checksum verification, creating a supply-chain risk. Referred Code // Until then, we at least have working nightly builds. downloadCygwinDll: async function (path) { const responseDll = await fetch('https://raw.githubusercontent.com/Scavanger/cygwin-test/main/cygwin1.dll'); if (!responseDll.ok) { throw new Error(`Failed to download cygwin1.dll: ${reponse.statusText}`); } const buffer = await responseDll.arrayBuffer(); await writeFile(path, Buffer.from(buffer)); }, Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing audit logs: Critical actions like downloading, extracting, and installing executables are performed without structured audit logs that include user, timestamp, action, and outcome. Referred Code downloadSitlBinary: async function (url, version) { const tempDir = (path.join(app.getPath('temp'), 'inav')); var currentVersion = await this.getCurretSITLVersion(); if (currentVersion && semver.eq(currentVersion, version)) { throw new Error("This version is already installed."); } try { await mkdir(tempDir, { recursive: true }); } catch (err) { throw new Error(`Failed to create temp directory: ${err.message}`); } const filePath = path.join(tempDir, 'sitl_resources.zip'); try { const response = await fetch(url); if (!response.ok) { const message = (await response.json()).message; throw new Error(`Failed to download SITL resouces: ` + message \|\| `GitHub Api Error: ${response.statusText}}`); ... (clipped 48 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Typo in API name: The IPC handler and its usage use the misspelled name `getCurretSITLVersion`, which could reduce clarity and cause confusion. Referred Code ipcMain.handle('getCurretSITLVersion', (_event) => { return new Promise(async resolve => { try { const version = await sitl_tools.getCurretSITLVersion(); resolve(version); } catch (err) { resolve(null); } }); Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Error surfaced to UI: Errors from GitHub API and filesystem (including raw messages) are relayed to the renderer and logged to the user via `GUI.log`, which may expose internal details. Referred Code return new Promise(async resolve => { try { const response = await sitl_tools.getSitlReleases(devRelease, latest); resolve({error: false, response: response}); } catch (error) { resolve({error: true, message: error.message}); } }); }); Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Raw error logging: Logs concatenate raw error messages from downloads and extraction to the UI via `GUI.log`, which might include sensitive paths or system details. Referred Code sitlBinaries_e.on('change', () => { const asset = sitlBinaries_e.find(':selected').data('asset'); if (asset) { GUI.log(i18n.getMessage("sitlDownloadStarted")); window.electronAPI.downloadSitlBinary(asset.url, asset.version).then(error => { if (!error) { GUI.log(i18n.getMessage('sitlUpdateSuccsess', asset.version)); $('.sitlStart').removeClass('disabled'); } else { GUI.log(`${i18n.getMessage('sitlErrorDownload')} ${error}`); } }); }; Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

qodo-code-review · 2025-11-27T14:42:56Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Avoid downloading binaries from personal repositories The PR downloads `cygwin1.dll` from a personal GitHub repository, creating a security risk. This file should be hosted in an official project location or require manual installation until an official fix is released. Examples: js/main/sitl_tools.js [64-73] downloadCygwinDll: async function (path) { const responseDll = await fetch('https://raw.githubusercontent.com/Scavanger/cygwin-test/main/cygwin1.dll'); if (!responseDll.ok) { throw new Error(`Failed to download cygwin1.dll: ${reponse.statusText}`); } const buffer = await responseDll.arrayBuffer(); await writeFile(path, Buffer.from(buffer)); }, js/main/sitl_tools.js [139-141] } else if (!await this.fileExists(installedCygwin1Path)) { await this.downloadCygwinDll(installedCygwin1Path); // <-- Fixme! } Solution Walkthrough: Before: // in js/main/sitl_tools.js async function downloadCygwinDll(path) { // Downloads DLL from a personal, non-project repository const response = await fetch('https://raw.githubusercontent.com/Scavanger/cygwin-test/main/cygwin1.dll'); // ... write file } async function downloadSitlBinary(...) { // ... if (platform === 'win32') { // ... const installedCygwin1Path = '.../sitl/cygwin1.dll'; if (cygwinDllIsMissing) { // This call introduces the security risk await downloadCygwinDll(installedCygwin1Path); } } // ... } After: // in js/main/sitl_tools.js async function downloadSitlBinary(...) { // ... if (platform === 'win32') { const cygwin1Path = '.../resources/sitl/windows/cygwin1.dll'; const installedCygwin1Path = '.../sitl/cygwin1.dll'; if (await fileExists(cygwin1Path)) { await copyFile(cygwin1Path, installedCygwin1Path); } else if (!await fileExists(installedCygwin1Path)) { // Instead of downloading from an untrusted source, // inform the user about the missing dependency. throw new Error("Missing 'cygwin1.dll'. Please ensure you are using a release that includes it, or follow manual installation instructions."); } } // ... } Suggestion importance[1-10]: 9 __ Why: This suggestion correctly identifies a critical security vulnerability by downloading a DLL from an untrusted, personal repository, which has a high impact on application integrity.	High
Possible issue	✅ ~~Fix race condition in API calls~~ Suggestion Impact: The commit implemented the concurrent fetching with Promise.all, removed the redundant second call, consolidated error handling, and updated logic to call upddateSitlReleases after both responses are processed. code diff: + Promise.all([ + window.electronAPI.getSitlReleases(false, false), + window.electronAPI.getSitlReleases(true, false) + ]).then(([stableData, devData]) => { + if (!stableData.error && stableData.response.length >= 1) { + sitlReleases = stableData.response; + } else { + sitlReleases = null; + if (stableData.message) { + GUI.log(`${i18n.getMessage("sitlErrorGithub")} ${stableData.message}`); + } + } + + if (!devData.error && devData.response.length >= 1) { + sitlDevReleses = devData.response; + } else { + sitlDevReleses = null; + if (devData.message) { + GUI.log(`${i18n.getMessage("sitlErrorGithub")} ${devData.message}`); + } + } + + if (sitlReleases) { upddateSitlReleases(); } else { - sitlReleases = null; - GUI.log(`${i18n.getMessage("sitlErrorGithub")} ${data.message}`); sitlBinaries_e.empty().append(`<option value="0">${i18n.getMessage('sitlOffline')}</option>`); } }); Refactor the SITL release fetching logic to use `Promise.all`. This will fetch stable and development releases concurrently, preventing race conditions and eliminating redundant API calls. tabs/sitl.js [134-160] -window.electronAPI.getSitlReleases(false, false).then(data => { - if (!data.error && data.response.length >= 1) { - sitlReleases = data.response; - - window.electronAPI.getSitlReleases(true, false).then(data => { - if (!data.error && data.response.length >= 1) { - sitlDevReleses = data.response; - } - }); - +Promise.all([ + window.electronAPI.getSitlReleases(false, false), + window.electronAPI.getSitlReleases(true, false) +]).then(([stableData, devData]) => { + if (!stableData.error && stableData.response.length >= 1) { + sitlReleases = stableData.response; + } else { + sitlReleases = null; + if (stableData.message) { + GUI.log(`${i18n.getMessage("sitlErrorGithub")} ${stableData.message}`); + } + } + + if (!devData.error && devData.response.length >= 1) { + sitlDevReleses = devData.response; + } else { + sitlDevReleses = null; + if (devData.message) { + GUI.log(`${i18n.getMessage("sitlErrorGithub")} ${devData.message}`); + } + } + + if (sitlReleases) { upddateSitlReleases(); } else { - sitlReleases = null; - GUI.log(`${i18n.getMessage("sitlErrorGithub")} ${data.message}`); sitlBinaries_e.empty().append(`<option value="0">${i18n.getMessage('sitlOffline')}</option>`); } }); -window.electronAPI.getSitlReleases(true, false).then(data => { - if (!data.error && data.response.length >= 1) { - sitlDevReleses = data.response; - } else { - sitlDevReleses = null; - GUI.log(i18n.getMessage('sitlErrorGithub')); - sitlBinaries_e.empty().append(`<option value="0">${i18n.getMessage('sitlOffline')}</option>`); - } -}); - `[Suggestion processed]` Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies redundant API calls and a race condition where `upddateSitlReleases()` is called before all data is fetched, and proposes a robust solution using `Promise.all` to fix these critical issues.	Medium
Possible issue	✅ ~~Fix incorrect property access in array~~ Suggestion Impact: The commit updated the property access to use the first element of the response array when logging the success message. code diff: - GUI.log(i18n.getMessage('sitlUpdateSuccsess', data.response.version)); + GUI.log(i18n.getMessage('sitlUpdateSuccsess', data.response[0].version)); Fix a property access error in the `downloadSitlBinary` success callback. Change `data.response.version` to `data.response[0].version` to correctly access the version from the releases array. js/serial_backend.js [215-222] window.electronAPI.downloadSitlBinary(data.response[0].url, data.response[0].version).then((error) => { if (!error) { - GUI.log(i18n.getMessage('sitlUpdateSuccsess', data.response.version)); + GUI.log(i18n.getMessage('sitlUpdateSuccsess', data.response[0].version)); startDemoMode(); } else { GUI.log(`${i18n.getMessage('sitlErrorDownload')} ${error}`); } }); `[Suggestion processed]` Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a `TypeError` bug where `data.response.version` is used on an array, which would prevent logging a success message and could cause runtime errors.	Medium
Learned best practice	✅ ~~Fix path and variable references~~ Suggestion Impact: The commit replaced the shadowing 'path' variable with 'commandPath', updated !== comparisons, and used 'commandPath' in spawn. It also changed var process to let process (not to 'proc'), still addressing variable update. code diff: + let process; try { - const path = path.join(app.getPath('userData'), 'sitl', command); + const commandPath = path.join(app.getPath('userData'), 'sitl', command); - if (os.platform() != 'win32') - { + if (os.platform() !== 'win32') { const stats = fs.statSync(commandPath); const permission = stats.mode & 0o777; - if (permission != 0o755) { + if (permission !== 0o755) { fs.chmodSync(commandPath, 0o755); } } - process = spawn(path, args, opts); + process = spawn(commandPath, args, opts); Avoid shadowing the imported `path` module and fix the undefined `commandPath` reference by using distinct variable names for the computed path. This prevents broken bindings and runtime errors. js/main/child_process.js [10-28] import path from 'path'; ... start: function (command, args, opts, window) { - var process; + let proc; try { - const path = path.join(app.getPath('userData'), 'sitl', command); + const commandPath = path.join(app.getPath('userData'), 'sitl', command); - if (os.platform() != 'win32') - { + if (os.platform() !== 'win32') { const stats = fs.statSync(commandPath); const permission = stats.mode & 0o777; - if (permission != 0o755) { + if (permission !== 0o755) { fs.chmodSync(commandPath, 0o755); } } - process = spawn(path, args, opts); + proc = spawn(commandPath, args, opts); } catch (err) { console.log(err); return -1; } `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Ensure identifiers and references point to the correct variables and paths to avoid runtime errors (Pattern 1).	Low
Learned best practice	Correct error handling identifier Fix the misspelled `response` identifier in the error message and include a null-safe status text to avoid reference errors during failure handling. js/main/sitl_tools.js [64-73] -downloadCygwinDll: async function (path) { - - const responseDll = await fetch('https://raw.githubusercontent.com/Scavanger/cygwin-test/main/cygwin1.dll'); - - if (!responseDll.ok) { - throw new Error(`Failed to download cygwin1.dll: ${reponse.statusText}`); +downloadCygwinDll: async function (outPath) { + const response = await fetch('https://raw.githubusercontent.com/Scavanger/cygwin-test/main/cygwin1.dll'); + if (!response.ok) { + const statusText = response.statusText \|\| 'Unknown error'; + throw new Error(`Failed to download cygwin1.dll: ${statusText}`); } - const buffer = await responseDll.arrayBuffer(); - await writeFile(path, Buffer.from(buffer)); + const buffer = await response.arrayBuffer(); + await writeFile(outPath, Buffer.from(buffer)); }, Apply / Chat Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Gate asynchronous flows with checks and handle errors consistently; reference correct identifiers to avoid typos causing failures (Pattern 1 and 2).	Low
Update

sensei-hacker · 2025-12-24T02:17:37Z

I wonder if I'm missing something here.
The description is "Get rid of SITL binaries floating that around in the Configurator repro".

This PR would change 24 different files and add quite a bit of code - to download a required file from another repo instead of having it included here in the repo?

It seems like maybe a lot code in order to avoid the need to check a file into the repo?

Lemontree-Softworks added 3 commits November 27, 2025 10:20

SITL Download

371d211

Better permission handling for SITL

88ff13a

Revoke test min firmware version

a522ba5

qodo-code-review bot added the Review effort 3/5 label Nov 27, 2025

Fix path in child_process.js

3c76fd0

Refactor SITL binary handling and improve error messages

7d306f6

Scavanger mentioned this pull request Dec 24, 2025

Feat fix SITL #2492

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Download SITL binaries from GitHub #2438

Download SITL binaries from GitHub #2438

Uh oh!

Scavanger commented Nov 27, 2025 •

edited

Loading

Uh oh!

qodo-code-review bot commented Nov 27, 2025 •

edited

Loading

Uh oh!

qodo-code-review bot commented Nov 27, 2025 •

edited

Loading

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

sensei-hacker commented Dec 24, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Download SITL binaries from GitHub #2438

Are you sure you want to change the base?

Download SITL binaries from GitHub #2438

Uh oh!

Conversation

Scavanger commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-code-review bot commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Compliance Guide 🔍

Uh oh!

qodo-code-review bot commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

sensei-hacker commented Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Scavanger commented Nov 27, 2025 •

edited

Loading

qodo-code-review bot commented Nov 27, 2025 •

edited

Loading

qodo-code-review bot commented Nov 27, 2025 •

edited

Loading

sensei-hacker commented Dec 24, 2025 •

edited

Loading