ospab
/
ostp
mirror of https://github.com/ospab/ostp.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix(gui): config went to the working directory; installer wrote unusable XML 

Three defects the first installer build exposed.

Settings could not be read or saved, "os error 5". With no config beside the
executable — which is the case for every fresh install — get_config_path fell
back to a bare relative "config.json", resolved against the process working
directory. Launched from a Start Menu shortcut that is whatever Windows chose,
frequently C:\Windows\System32. On a writable working directory the silent
outcome would have been worse than the error: settings persisting somewhere
unrelated and appearing to vanish. The config now lives beside the executable
only where that directory actually accepts writes, and otherwise under the
user's own profile, carrying an existing read-only copy across once.
Writability is measured, not inferred from the path: an install onto a data
drive may well be writable where Program Files is not.

The installer could not register the task: "The task XML is malformed.
(1,2)::ERROR: incorrect document syntax". Writing it from NSIS emitted a UTF-16
byte-order mark ahead of content whose encoding depends on whether makensis was
built in Unicode mode. Replaced with the ScheduledTasks cmdlets, which take the
same settings as arguments — no file, so no encoding to get wrong. Verified the
invocation reaches Register-ScheduledTask and fails only on "Access is denied"
when unelevated, which is exactly what the elevated installer supplies.

That command is delimited with backticks, NSIS's third quote character. As a
single-quoted string it would have ended at PowerShell's first quote.

"Copy failed" on wintun.dll: CopyFiles takes a destination directory, and it
was given a file path. It is also guarded now, so a missing resource says so
instead of failing mutely.

Finally, per request, the app no longer registers the task itself — that is the
installer's job alone. Without a task it goes straight to the direct elevated
launch, which prompts per connect as it always did, rather than spending a
prompt on a registration attempt and then another on the launch.
This commit is contained in:
ospab 2026-08-11 17:43:04 +03:00
parent a63c34669b
commit 234497759b
2 changed files with 126 additions and 247 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

249
ostp-gui/src-tauri/src/lib.rs
View File

@ -134,16 +134,84 @@ struct AppState(Mutex<AppStateInner>);
// ── Config helpers ────────────────────────────────────────────────────────────
/// Per-user config location, used whenever the config cannot live next to the
/// executable.
fn user_config_path() -> PathBuf {
let base = std::env::var_os(if cfg!(windows) { "APPDATA" } else { "HOME" })
.map(PathBuf::from)
.unwrap_or_else(std::env::temp_dir);
let dir = if cfg!(windows) { base.join("OSTP") } else { base.join(".config").join("ostp") };
dir.join("config.json")
}
/// Where the GUI reads and writes its configuration.
///
/// Portable installs keep the config beside the executable, which is what the
/// zip has always done, and that is preserved wherever the directory is
/// actually writable.
///
/// What it must never do again is fall back to a bare relative `config.json`.
/// That resolves against the process working directory, which for a Start Menu
/// shortcut is whatever Windows chose — often `C:\Windows\System32`. Reading
/// and saving settings then failed with "Access is denied" (os error 5), and on
/// a writable working directory it would have been worse still: settings would
/// silently persist somewhere unrelated and appear to vanish.
///
/// Writability is measured rather than inferred from the install location. An
/// installer can put the app anywhere — a per-machine install onto a data drive
/// may well be writable, while Program Files is not — so the location alone
/// says nothing.
fn get_config_path() -> PathBuf {
if let Ok(exe_path) = std::env::current_exe() {
if let Some(parent) = exe_path.parent() {
let path = parent.join("config.json");
if path.exists() {
return path;
let portable = parent.join("config.json");
if portable.exists() {
if is_file_writable(&portable) {
return portable;
}
// Read-only beside the exe: unusable as the live file, but its
// contents are still worth carrying over once.
let user = user_config_path();
if !user.exists() {
if let Some(dir) = user.parent() {
let _ = std::fs::create_dir_all(dir);
}
let _ = std::fs::copy(&portable, &user);
}
} else if is_dir_writable(parent) {
// No config yet and the directory takes writes: a portable
// unzip, so keep the config travelling with the folder.
return portable;
}
}
}
PathBuf::from("config.json")
let path = user_config_path();
if let Some(dir) = path.parent() {
let _ = std::fs::create_dir_all(dir);
}
path
}
/// Whether an existing file can actually be written to.
///
/// Answered by opening it, not by reading permission bits: on Windows the
/// effective answer depends on the ACL and on virtualization, and `readonly()`
/// reflects neither.
fn is_file_writable(path: &std::path::Path) -> bool {
std::fs::OpenOptions::new().append(true).open(path).is_ok()
}
/// Whether new files can be created in a directory, tested by doing it.
fn is_dir_writable(dir: &std::path::Path) -> bool {
let probe = dir.join(format!(".ostp-write-test-{}", std::process::id()));
match std::fs::File::create(&probe) {
Ok(_) => {
let _ = std::fs::remove_file(&probe);
true
}
Err(_) => false,
}
}
fn map_to_client_config(raw: &ClientConfigRaw, mode: &str) -> ostp_client::config::ClientConfig {
@ -836,21 +904,8 @@ fn helper_args_file() -> PathBuf {
base.join("OSTP").join("helper-args.json")
}
/// Minimal XML text escaping for the values interpolated into the task
/// definition. Paths and usernames are attacker-irrelevant here but can easily
/// contain `&`, which would otherwise produce invalid XML and a confusing
/// schtasks parse failure.
#[cfg(target_os = "windows")]
fn xml_escape(s: &str) -> String {
s.replace('&', "&amp;")
.replace('<', "&lt;")
.replace('>', "&gt;")
.replace('"', "&quot;")
.replace('\'', "&apos;")
}
/// Reverse of [`xml_escape`]. `&amp;` must be undone last or `&amp;lt;` would
/// come back as `<`.
/// Undoes XML entity escaping. `&amp;` must be handled last, or `&amp;lt;`
/// would come back as `<`.
#[cfg(target_os = "windows")]
fn xml_unescape(s: &str) -> String {
s.replace("&quot;", "\"")
@ -923,151 +978,6 @@ fn helper_task_matches(exe: &std::path::Path) -> bool {
}
}
/// Register the Scheduled Task. This is the ONLY step that needs elevation, and
/// it happens once per machine; every later tunnel start reuses the task.
///
/// RunLevel=HIGHEST makes the task run elevated, and because a task launch is
/// not an elevation request, Windows shows no consent dialog for it.
#[cfg(target_os = "windows")]
fn install_helper_task(exe: &std::path::Path) -> anyhow::Result<()> {
let args_file = helper_args_file();
if let Some(dir) = args_file.parent() {
std::fs::create_dir_all(dir)?;
}
// Register from an XML definition rather than /TR. The command line would
// otherwise need the exe path and the args path quoted INSIDE an already
// quoted /TR value, escaped again through ShellExecuteW — a notoriously
// brittle chain when either path contains a space, which both of these do
// by default (Program Files, and usernames with spaces). XML also lets the
// battery and time-limit settings below be stated explicitly.
let user = format!(
"{}\\{}",
std::env::var("USERDOMAIN").unwrap_or_else(|_| "%COMPUTERNAME%".into()),
std::env::var("USERNAME").unwrap_or_default()
);
let xml = format!(
r#"<?xml version="1.0" encoding="UTF-16"?>
<Task version="1.2" xmlns="http://schemas.microsoft.com/windows/2004/02/mit/task">
<RegistrationInfo>
<Description>Runs the OSTP TUN helper elevated so enabling the tunnel does not prompt for consent every time.</Description>
</RegistrationInfo>
<Principals>
<Principal id="Author">
<UserId>{user}</UserId>
<LogonType>InteractiveToken</LogonType>
<RunLevel>HighestAvailable</RunLevel>
</Principal>
</Principals>
<Settings>
<MultipleInstancesPolicy>Parallel</MultipleInstancesPolicy>
<DisallowStartIfOnBatteries>false</DisallowStartIfOnBatteries>
<StopIfGoingOnBatteries>false</StopIfGoingOnBatteries>
<StartWhenAvailable>false</StartWhenAvailable>
<RunOnlyIfNetworkAvailable>false</RunOnlyIfNetworkAvailable>
<ExecutionTimeLimit>PT0S</ExecutionTimeLimit>
<Enabled>true</Enabled>
<Hidden>false</Hidden>
<AllowHardTerminate>true</AllowHardTerminate>
</Settings>
<Actions Context="Author">
<Exec>
<Command>{exe}</Command>
<Arguments>--args-file "{args}"</Arguments>
</Exec>
</Actions>
</Task>
"#,
user = xml_escape(&user),
exe = xml_escape(&exe.display().to_string()),
args = xml_escape(&args_file.display().to_string()),
);
// schtasks /Create /XML expects UTF-16LE with a BOM.
let xml_path = std::env::temp_dir().join(format!("ostp_task_{}.xml", rand::random::<u32>()));
let mut utf16: Vec<u8> = vec![0xFF, 0xFE];
for unit in xml.encode_utf16() {
utf16.extend_from_slice(&unit.to_le_bytes());
}
std::fs::write(&xml_path, &utf16)?;
// Registering a HighestAvailable task is itself privileged: this is the one
// prompt, and it happens once per machine.
//
// Elevate through PowerShell's Start-Process -Wait rather than
// ShellExecuteW. ShellExecuteW returns as soon as the elevated process is
// LAUNCHED, so the XML below was being deleted while schtasks was still
// starting up — registration then failed, leaving the user with a consent
// prompt that accomplished nothing, followed by a second prompt from the
// fallback path. -Wait makes the deletion safe and lets the exit code be
// checked instead of guessed at by polling.
//
// ArgumentList takes an array, so the task name and XML path never need
// quoting or escaping through a command line, only PowerShell's own
// single-quote doubling.
let ps = format!(
"$p = Start-Process -FilePath 'schtasks.exe' -Verb RunAs -Wait -PassThru \
-WindowStyle Hidden -ArgumentList @('/Create','/TN','{}','/XML','{}','/F'); \
exit $p.ExitCode",
ps_quote(HELPER_TASK_NAME),
ps_quote(&xml_path.display().to_string()),
);
let status = quiet_command("powershell")
.args(["-NoProfile", "-NonInteractive", "-WindowStyle", "Hidden", "-Command", &ps])
.status();
// 1223 is ERROR_CANCELLED: the consent prompt was declined. Nothing was
// launched, so there is no point waiting for a task to appear.
if let Ok(s) = &status {
if s.code() == Some(1223) {
let _ = std::fs::remove_file(&xml_path);
anyhow::bail!("the consent prompt was declined");
}
}
// The exit code is advisory only, never proof of success. `-Verb RunAs`
// launches through ShellExecute, and a non-elevated parent frequently
// cannot read the elevated child's exit code — `$p.ExitCode` then yields
// $null, and `exit $null` leaves PowerShell reporting 0. A failed
// registration would sail straight through a `s.success()` check.
//
// Worse, -Wait does not reliably block until the elevated process exits.
// Deleting the XML right after the call raced schtasks reading it — the
// exact bug that made the previous attempt fail — so wait for the task
// itself to show up. These queries are windowless, so unlike the earlier
// polling loop they cost the user nothing to watch.
let deadline = std::time::Instant::now() + std::time::Duration::from_secs(15);
let mut registered = false;
while std::time::Instant::now() < deadline {
if helper_task_matches(exe) {
registered = true;
break;
}
std::thread::sleep(std::time::Duration::from_millis(250));
}
// Only now is deleting it safe.
let _ = std::fs::remove_file(&xml_path);
if registered {
return Ok(());
}
match status {
Ok(s) => anyhow::bail!(
"the scheduled task did not appear after registration (powershell exit {:?})",
s.code()
),
Err(e) => anyhow::bail!("could not run powershell to register the task: {e}"),
}
}
/// Escape a value for embedding in a PowerShell single-quoted string.
#[cfg(target_os = "windows")]
fn ps_quote(s: &str) -> String {
s.replace('\'', "''")
}
#[cfg(target_os = "windows")]
fn launch_as_admin(exe: &std::path::PathBuf, token: &str, port: u16) -> anyhow::Result<()> {
// Preferred path: hand the parameters over in a file and trigger the
@ -1083,11 +993,12 @@ fn launch_as_admin(exe: &std::path::PathBuf, token: &str, port: u16) -> anyhow::
let wrote_args = std::fs::write(&args_file, payload.to_string()).is_ok();
if wrote_args {
if !helper_task_matches(exe) {
if let Err(e) = install_helper_task(exe) {
eprintln!("[OSTP] could not register the helper task ({e}); falling back to a direct elevated launch");
}
}
// Deliberately does NOT create the task when it is missing. Registering
// one is privileged, so the app could only do it by raising the very
// prompt this exists to avoid — and it would then charge the user two
// prompts for the privilege. Creating it belongs to the installer,
// which is already elevated. Without it we simply fall through to the
// direct elevated launch, which prompts once per connect as before.
if helper_task_matches(exe) {
let run = quiet_command("schtasks")
.args(["/Run", "/TN", HELPER_TASK_NAME])

124
ostp-gui/src-tauri/windows/hooks.nsh
View File

@ -2,102 +2,70 @@
; without a consent prompt.
;
; This belongs in the installer, not in the app. Registering a task that runs
; with elevated rights is itself a privileged operation, so an unprivileged GUI
; can only get one by raising a UAC prompt — which is the very thing we are
; trying to remove. The installer already runs elevated, so here it costs
; nothing: the user consents once, to the install, and never again per connect.
; elevated is itself a privileged operation, so an unprivileged GUI could only
; obtain one by raising the very prompt we are trying to remove. The installer
; already runs elevated (installMode is perMachine), so here it costs nothing:
; the user consents once, to the install, and never again per connect.
;
; The task carries no usable trigger (a one-shot dated in the past), because it
; exists solely to be started on demand by the app.
!macro OSTP_WRITE_TASK_XML OUTFILE
; NSIS is built in Unicode mode here, so FileWrite emits UTF-16LE — which is
; what `schtasks /XML` requires. It still needs the byte-order mark itself.
FileOpen $R1 "${OUTFILE}" w
FileWriteWord $R1 65279
FileWrite $R1 '<?xml version="1.0" encoding="UTF-16"?>$\r$\n'
FileWrite $R1 '<Task version="1.2" xmlns="http://schemas.microsoft.com/windows/2004/02/mit/task">$\r$\n'
FileWrite $R1 ' <RegistrationInfo>$\r$\n'
FileWrite $R1 ' <Description>Starts the OSTP TUN helper elevated so connecting does not prompt for consent every time.</Description>$\r$\n'
FileWrite $R1 ' </RegistrationInfo>$\r$\n'
FileWrite $R1 ' <Principals>$\r$\n'
FileWrite $R1 ' <Principal id="Author">$\r$\n'
; S-1-5-32-545 is BUILTIN\Users by SID rather than by name: the name is
; localized ("Пользователи" on a Russian Windows) and would not resolve.
; Combined with InteractiveToken this makes the task run as whichever user
; actually launches it, so a machine-wide install still works for every
; account instead of only the one that happened to run the installer.
FileWrite $R1 ' <GroupId>S-1-5-32-545</GroupId>$\r$\n'
FileWrite $R1 ' <LogonType>InteractiveToken</LogonType>$\r$\n'
FileWrite $R1 ' <RunLevel>HighestAvailable</RunLevel>$\r$\n'
FileWrite $R1 ' </Principal>$\r$\n'
FileWrite $R1 ' </Principals>$\r$\n'
FileWrite $R1 ' <Settings>$\r$\n'
; Parallel: reconnecting before a previous helper has fully exited must not
; be silently dropped as a duplicate instance.
FileWrite $R1 ' <MultipleInstancesPolicy>Parallel</MultipleInstancesPolicy>$\r$\n'
; A VPN is most needed on battery, and a tunnel must not be killed on unplug.
FileWrite $R1 ' <DisallowStartIfOnBatteries>false</DisallowStartIfOnBatteries>$\r$\n'
FileWrite $R1 ' <StopIfGoingOnBatteries>false</StopIfGoingOnBatteries>$\r$\n'
FileWrite $R1 ' <StartWhenAvailable>false</StartWhenAvailable>$\r$\n'
FileWrite $R1 ' <RunOnlyIfNetworkAvailable>false</RunOnlyIfNetworkAvailable>$\r$\n'
; PT0S disables the execution time limit; the default would tear the tunnel
; down after three days.
FileWrite $R1 ' <ExecutionTimeLimit>PT0S</ExecutionTimeLimit>$\r$\n'
FileWrite $R1 ' <Enabled>true</Enabled>$\r$\n'
FileWrite $R1 ' <Hidden>false</Hidden>$\r$\n'
FileWrite $R1 ' <AllowHardTerminate>true</AllowHardTerminate>$\r$\n'
FileWrite $R1 ' </Settings>$\r$\n'
FileWrite $R1 ' <Actions Context="Author">$\r$\n'
FileWrite $R1 ' <Exec>$\r$\n'
FileWrite $R1 ' <Command>$INSTDIR\ostp-tun-helper.exe</Command>$\r$\n'
; The port and auth token change per launch and a task stores a fixed command
; line, so they travel in this file instead. %LOCALAPPDATA% is deliberately
; left unexpanded: Task Scheduler expands it when the task runs, which lands
; on the profile of whoever launched it rather than the installing user's.
FileWrite $R1 ' <Arguments>--args-file "%LOCALAPPDATA%\OSTP\helper-args.json"</Arguments>$\r$\n'
FileWrite $R1 ' </Exec>$\r$\n'
FileWrite $R1 ' </Actions>$\r$\n'
FileWrite $R1 '</Task>$\r$\n'
FileClose $R1
!macroend
; The task carries no trigger at all — it exists solely to be started on demand.
!macro NSIS_HOOK_POSTINSTALL
; Bundled resources land in $INSTDIR\resources, but the helper loads wintun
; with a plain LoadLibrary, which searches its own directory — so put a copy
; beside the executables.
DetailPrint "Placing wintun.dll next to the helper..."
CopyFiles /SILENT "$INSTDIR\resources\wintun.dll" "$INSTDIR\wintun.dll"
; beside the executables. The destination is the directory, not a file path:
; CopyFiles takes a target directory, and naming the file made it fail.
${If} ${FileExists} "$INSTDIR\resources\wintun.dll"
DetailPrint "Placing wintun.dll next to the helper..."
CopyFiles /SILENT "$INSTDIR\resources\wintun.dll" "$INSTDIR"
${Else}
DetailPrint "WARNING: resources\wintun.dll is missing; TUN mode will not start."
${EndIf}
; Registered through PowerShell's ScheduledTasks module rather than
; `schtasks /XML`. Generating the XML from NSIS wrote a UTF-16 byte-order mark
; ahead of content whose encoding depended on whether makensis was built in
; Unicode mode, and schtasks rejected the result outright:
; "The task XML is malformed. (1,2)::ERROR: incorrect document syntax"
; The cmdlets take the same settings as arguments, so no file is written and
; there is no encoding to get wrong.
;
; The command is delimited with backticks, NSIS's third quote character, so
; that PowerShell's own single quotes and the shell's double quotes can both
; appear literally — inside a single-quoted NSIS string the first PowerShell
; quote would have terminated the argument early.
;
; $$ is an escaped literal dollar for PowerShell's variables; a bare $ would
; be read by NSIS as one of its own. The helper argument is assembled with
; [char]34 instead of nested quotes so that a username containing a space
; still yields a correctly quoted path, without three levels of escaping.
;
; The principal is the SID S-1-5-32-545 (BUILTIN\Users) rather than the
; installing user, so a per-machine install serves every account instead of
; only whoever ran the installer. The SID is used because the name is
; localized and would not resolve. %LOCALAPPDATA% is likewise left unexpanded
; for Task Scheduler to resolve per running user.
DetailPrint "Registering the OSTP TUN helper task..."
!insertmacro OSTP_WRITE_TASK_XML "$PLUGINSDIR\ostp-helper-task.xml"
; /F overwrites an existing registration, so reinstalling or upgrading to a
; different directory repoints the task instead of leaving a stale path — the
; app verifies the registered path at runtime and would otherwise have to
; re-register it with a prompt.
nsExec::ExecToLog 'schtasks.exe /Create /TN "OSTP TUN Helper" /XML "$PLUGINSDIR\ostp-helper-task.xml" /F'
nsExec::ExecToLog `powershell -NoProfile -NonInteractive -ExecutionPolicy Bypass -Command "$$act = New-ScheduledTaskAction -Execute '$INSTDIR\ostp-tun-helper.exe' -Argument ('--args-file ' + [char]34 + '%LOCALAPPDATA%\OSTP\helper-args.json' + [char]34); $$prn = New-ScheduledTaskPrincipal -GroupId 'S-1-5-32-545' -RunLevel Highest; $$set = New-ScheduledTaskSettingsSet -AllowStartIfOnBatteries -DontStopIfGoingOnBatteries -ExecutionTimeLimit ([TimeSpan]::Zero) -MultipleInstances Parallel; Register-ScheduledTask -TaskName 'OSTP TUN Helper' -Action $$act -Principal $$prn -Settings $$set -Force | Out-Null"`
Pop $R0
Delete "$PLUGINSDIR\ostp-helper-task.xml"
${If} $R0 == 0
DetailPrint "Helper task registered; connecting will not prompt for consent."
DetailPrint "Helper task registered; connecting will not ask for consent."
${Else}
; Not fatal. The app keeps a fallback that registers the task itself on
; first connect, at the cost of the one prompt this was meant to avoid.
DetailPrint "Could not register the helper task (schtasks returned $R0)."
DetailPrint "OSTP will still work, but the first connect will ask for consent."
; Not fatal: the app still works, it just falls back to an elevated launch
; that asks for consent on each connect.
DetailPrint "Could not register the helper task (exit $R0)."
DetailPrint "OSTP will still work, but every connect will ask for consent."
${EndIf}
!macroend
!macro NSIS_HOOK_PREUNINSTALL
; Leaving the task behind would point at a deleted executable, and the app
; treats a mismatched path as grounds to re-register.
; Leaving the task behind would point it at a deleted executable, and
; `schtasks /Run` reports success for merely accepting such a request — the
; app would wait on a helper that never starts.
DetailPrint "Removing the OSTP TUN helper task..."
nsExec::ExecToLog 'schtasks.exe /Delete /TN "OSTP TUN Helper" /F'
Pop $R0
; This copy was made by the install hook, so the uninstaller does not know
; about it and would otherwise leave it behind.
; Copied by the install hook, so the uninstaller has no record of it.
Delete "$INSTDIR\wintun.dll"
!macroend