Close the remaining installer review findings

Eleven findings, grouped by the root cause each belongs to.

Wizard input reaching a command line unchecked (ShopDBFlask.iss). Port fields
were spliced in bare and arrive as [int] parameters, so a blank or mistyped
port shifted every argument after it; both port fields are now validated as
1-65535 digits. A path ending in a backslash, which is what a drive root looks
like, ended its argument with \" and CommandLineToArgvW read that as an escaped
quote, so paths are now quoted through a helper that doubles the trailing
backslash. A drive root is refused outright as well: uninstall deletes the
application directory recursively, so installing to D:\ would have wiped the
drive on removal. The password handoff was written with SaveStringToFile, which
writes an AnsiString, and read back as UTF-8, so a correct non-ASCII password
was reported as wrong; it now goes out as UTF-8 without a BOM.

Launching without checking the result. Plugin deregistration invoked "flask
plugin uninstall" without --yes, and the command carries a click
confirmation_option that aborts with exit 1 when nothing can answer the prompt,
so it could never once have succeeded; the bare 2>&1 under EAP Stop then turned
that into a terminating error which the catch downgraded to a warning while the
plugin directory was deleted regardless. It now passes --yes, brackets the
error preference, restores the location in a finally, and keeps the code on
disk unless deregistration actually succeeded. MarkShortcutRunAs had four
quotes where it needed three, which kept the whole command inside one Pascal
literal so LnkPath was never interpolated and no shortcut ever got the
elevation flag; its exit code is now logged too.

Comparing IIS physical paths as raw strings. IIS stores the path as typed, so
it may carry environment variables or a trailing backslash. A Test-SamePath
helper now normalises both sides. That closes a real hazard in uninstall, which
matched applications on alias alone and would remove an unrelated application
of the same name under another site, unattended, since -OnFailure never
suppresses the confirmation.

Accepting existing IIS state without reconciling it. "Site already exists" took
the site however it was, so re-running with a different port left the old
binding while CORS_ORIGINS, the firewall rule and the smoke test all used the
new one, failing a working server. It now refuses with both ports named rather
than silently re-binding, and refuses a site of that name serving a different
directory.

Preflight rows drawn past the panel. The failures loop had no cap at all and
the warnings loop capped at 6, a number unrelated to the panel, which holds
about three rows. The cap is now measured from the panel height, applies to
both loops, and the footer counts what was actually left out instead of
inferring it.

Also: a failed upgrade now says the application pool is still stopped and how
to start it, rather than only "part-configured", since stage 2 stops a pool
that was serving. It is deliberately not restarted automatically, because after
a stage 3 failure the deployed code and the schema may disagree. shopdb-admin
Restart-App starts a stopped pool or site instead of recycling, which is a
no-op on a stopped pool and then reported the application as unresponsive. A
dead Write-Log line that parsed as three arguments is gone, and a preflight
warning no longer tells the operator to add a directory to a compiled exe.

deploy/windows/installer/ShopDBFlask.iss

@@ -655,7 +655,7 @@ end;
 // If nothing is wrong the page collapses to a single reassuring statement.
 procedure RenderPreflight(Lines: TArrayOfString; var HasBlockers: Boolean);
 var
-  I, Y, Shown, Fails, Warns, Passes: Integer;
+  I, Y, Shown, Fails, Warns, Passes, RowHeight, MaxRows, Hidden: Integer;
   Parts: TArrayOfString;
   Check, Detail, Fix: String;
 begin
@@ -663,6 +663,16 @@ begin
   Y := ScaleY(4);
   Shown := 0;
 
+  // How many rows actually FIT, measured, not guessed. The old cap of 6 bore no
+  // relation to the panel, which holds roughly three: at four or five notes the
+  // rows below were drawn past the bottom edge and silently vanished - taking
+  // with them the warning that the bundled MySQL collides on port 3306, which is
+  // exactly what the operator needs before the very next page.
+  RowHeight := ScaleY(46) + ScaleY(ROW_GAP);
+  if RowHeight < 1 then RowHeight := 1;
+  MaxRows := DetailPanel.Height div RowHeight;
+  if MaxRows < 1 then MaxRows := 1;
+
   // Blockers first, then warnings - a second pass rather than one, so severity
   // ordering does not depend on the order the checks happen to run in.
   for I := 0 to GetArrayLength(Lines) - 1 do
@@ -676,6 +686,9 @@ begin
 
   for I := 0 to GetArrayLength(Lines) - 1 do
   begin
+    // Blockers are capped too. Without this the FAIL loop drew every failure,
+    // however many, straight off the bottom of the panel.
+    if Shown >= MaxRows then Break;
     Parts := StringSplit(Lines[I], ['|'], stAll);
     if GetArrayLength(Parts) < 4 then Continue;
     if Parts[0] <> 'FAIL' then Continue;
@@ -690,7 +703,7 @@ begin
 
   for I := 0 to GetArrayLength(Lines) - 1 do
   begin
-    if Shown >= 6 then Break;
+    if Shown >= MaxRows then Break;
     Parts := StringSplit(Lines[I], ['|'], stAll);
     if GetArrayLength(Parts) < 4 then Continue;
     if Parts[0] <> 'WARN' then Continue;
@@ -727,8 +740,13 @@ begin
            + 'verified. Nothing needs your attention.');
   end;
 
-  if (Shown >= 6) and (Warns > 6 - Fails) then
-    FooterText.Caption := 'Some notes are not shown. The full check is in the install log.'
+  // Count what was actually left out rather than inferring it from the old cap,
+  // which under-reported: with 2 failures and 5 warnings it claimed everything
+  // was shown while four rows had been dropped.
+  Hidden := (Fails + Warns) - Shown;
+  if Hidden > 0 then
+    FooterText.Caption := IntToStr(Hidden) + ' further item(s) are not shown here. '
+                          + 'All of them are in the install log.'
   else
     FooterText.Caption := IntToStr(Passes) + ' checks passed. Nothing has been changed on this server.';
 end;
@@ -837,10 +855,76 @@ begin
   Log('[shopdb] pre-fill base directory: ' + Result);
 end;
 
+// Everything typed in the wizard ends up on a command line that
+// CommandLineToArgvW parses, and reaches PowerShell parameters typed [int] or
+// [string]. A blank, a space, or a trailing backslash therefore has to be caught
+// HERE - by the time the script sees it, the arguments have already shifted.
+
+function IsValidPort(const S: String): Boolean;
+var
+  I, N: Integer;
+begin
+  Result := False;
+  if (S = '') or (Length(S) > 5) then Exit;
+  for I := 1 to Length(S) do
+    if (S[I] < '0') or (S[I] > '9') then Exit;   // also rejects spaces and signs
+  N := StrToIntDef(S, -1);
+  Result := (N >= 1) and (N <= 65535);
+end;
+
+function IsAsciiOnly(const S: String): Boolean;
+var
+  I: Integer;
+begin
+  Result := True;
+  for I := 1 to Length(S) do
+    if Ord(S[I]) > 126 then
+    begin
+      Result := False;
+      Exit;
+    end;
+end;
+
+// A path that ends in a backslash - a drive root such as D:\ - closes the
+// argument with \" , which CommandLineToArgvW reads as an ESCAPED quote. The
+// argument never terminates and every argument after it shifts one place.
+// Doubling the backslash inside the quotes is the documented way out.
+function QuotePathArg(const P: String): String;
+begin
+  if (P <> '') and (P[Length(P)] = '\') then
+    Result := '"' + P + '\' + '"'
+  else
+    Result := '"' + P + '"';
+end;
+
 function NextButtonClick(CurPageID: Integer): Boolean;
 begin
   Result := True;
 
+  // A drive root is refused outright rather than quoted around. Uninstall
+  // removes the application directory recursively, so accepting D:\ here would
+  // mean uninstalling ShopDB-Flask wipes the whole drive.
+  if CurPageID = wpSelectDir then
+  begin
+    if Length(WizardDirValue) <= 3 then
+    begin
+      MsgBox('Choose a folder rather than a whole drive.' + #13#10#13#10
+             + 'Removing ShopDB-Flask deletes the folder it was installed into, '
+             + 'so installing to a drive root would delete everything on that '
+             + 'drive when it is uninstalled.', mbError, MB_OK);
+      Result := False;
+    end;
+  end;
+
+  if CurPageID = SitePage.ID then
+  begin
+    if not IsValidPort(SitePage.Values[1]) then
+    begin
+      MsgBox('Enter the port as a number between 1 and 65535.', mbError, MB_OK);
+      Result := False;
+    end;
+  end;
+
   if CurPageID = wpWelcome then
     RunPreflight;
 
@@ -872,6 +956,13 @@ begin
       begin
         MsgBox('Enter the database host.', mbError, MB_OK);
         Result := False;
+      end
+      else if not IsValidPort(DbDetailsPage.Values[1]) then
+      begin
+        MsgBox('Enter the database port as a number between 1 and 65535.'
+               + #13#10#13#10 + 'MySQL uses 3306 unless it was changed.',
+               mbError, MB_OK);
+        Result := False;
       end;
     end;
   end;
@@ -1056,22 +1147,31 @@ var
   Cmd: String;
 begin
   if not FileExists(LnkPath) then Exit;
+  // THREE quotes each side, not four. Four is an escaped quote twice over, which
+  // keeps the whole expression inside one Pascal literal: LnkPath was never
+  // interpolated, PowerShell got the bare word "LnkPath", failed to parse, and
+  // no shortcut ever received the elevation flag. Self-elevation hid it - the
+  // only symptom was a second window after an extra prompt.
   Cmd := '-NoProfile -ExecutionPolicy Bypass -Command "'
-       + '$p='''' + LnkPath + ''''; '
+       + '$p=''' + LnkPath + '''; '
        + '$b=[IO.File]::ReadAllBytes($p); '
        + '$b[21]=$b[21] -bor 0x20; '
        + '[IO.File]::WriteAllBytes($p,$b)"';
   Exec(PowerShellPath, Cmd, '', SW_HIDE, ewWaitUntilTerminated, ResultCode);
+  if ResultCode <> 0 then
+    Log('[shopdb] could not mark ' + LnkPath + ' run-as-administrator (exit '
+        + IntToStr(ResultCode) + '); the console will self-elevate instead');
 end;
 
 function RunInstallStages: String;
 var
   ResultCode: Integer;
   PwFile, Args, Common: String;
+  PwLines: TArrayOfString;
 begin
   Result := '';
-  Common := '-BundleRoot "' + ExpandConstant('{tmp}\shopdb-bundle') + '"' +
-            ' -AppRoot "' + ExpandConstant('{app}') + '"' +
+  Common := '-BundleRoot ' + QuotePathArg(ExpandConstant('{tmp}\shopdb-bundle')) +
+            ' -AppRoot ' + QuotePathArg(ExpandConstant('{app}')) +
             ' -SiteHost "' + SitePage.Values[0] + '"' +
             ' -SitePort ' + SitePage.Values[1] +
             ' -OnFailure never' +
@@ -1110,7 +1210,13 @@ begin
     if DbCredsPage.Values[1] <> '' then
     begin
       PwFile := ExpandConstant('{tmp}\dbpw.txt');
-      SaveStringToFile(PwFile, DbCredsPage.Values[1] + #13#10, False);
+      // UTF-8, no BOM. SaveStringToFile writes an AnsiString, but the installer
+      // reads this back with -Encoding UTF8 - so any non-ASCII character in the
+      // password came out mangled and the database rejected a password that was
+      // typed correctly, reported as "wrong username or password".
+      SetArrayLength(PwLines, 1);
+      PwLines[0] := DbCredsPage.Values[1];
+      SaveStringsToUTF8FileWithoutBOM(PwFile, PwLines, False);
     end
     else
       PwFile := '';

deploy/windows/installer/shopdb-admin.ps1

@@ -314,10 +314,38 @@ function Stop-App {
 
 function Restart-App {
     Head 'Restarting'
-    # Recycle rather than stop/start: it drains existing requests instead of
-    # cutting them off, and it is what a config change actually needs.
-    & $AppCmd recycle apppool /apppool.name:$AppPool 2>&1 | ForEach-Object { Say "  $_" }
-    Start-Sleep -Seconds 2
+    # A recycle CANNOT start something that is stopped - it is a no-op on a
+    # stopped pool. Restarting after a failed upgrade, which is exactly when the
+    # pool is stopped, therefore did nothing and then reported "did not respond"
+    # in red, as though the application were broken. Start it first when needed.
+    Import-Module WebAdministration -ErrorAction SilentlyContinue
+    $started = $true
+    try {
+        if (Test-Path "IIS:\AppPools\$AppPool") {
+            $started = ((Get-Item "IIS:\AppPools\$AppPool").State -eq 'Started')
+        }
+    } catch { }
+    if (-not $started) {
+        Say '  the application pool is stopped; starting it' 'Yellow'
+        Start-WebAppPool -Name $AppPool -ErrorAction SilentlyContinue
+        Start-Sleep -Seconds 3
+    } else {
+        # Recycle rather than stop/start: it drains existing requests instead of
+        # cutting them off, and it is what a config change actually needs.
+        & $AppCmd recycle apppool /apppool.name:$AppPool 2>&1 | ForEach-Object { Say "  $_" }
+        if ($LASTEXITCODE -ne 0) { Say "  recycle reported exit $LASTEXITCODE" 'Yellow' }
+        Start-Sleep -Seconds 2
+    }
+
+    # A stopped SITE answers nothing however healthy the pool is.
+    try {
+        $siteNow = Get-Website -Name $SiteName -ErrorAction SilentlyContinue
+        if ($siteNow -and $siteNow.State -ne 'Started') {
+            Say '  the site is stopped; starting it' 'Yellow'
+            Start-Website -Name $SiteName -ErrorAction SilentlyContinue
+            Start-Sleep -Seconds 2
+        }
+    } catch { }
     try {
         $r = Invoke-WebRequest -Uri ((Get-Deployment).BaseUrl + '/') -UseBasicParsing -TimeoutSec 30
         Say ("  back up (HTTP {0})" -f $r.StatusCode) 'Green'

deploy/windows/installer/shopdb-install.ps1

@@ -301,6 +301,22 @@ function New-Secret {
     return [Convert]::ToBase64String($b).Replace('+','-').Replace('/','_').TrimEnd('=')
 }
 
+function Resolve-IisPath {
+    # IIS stores physicalPath as it was typed, so it can carry %SystemDrive%-style
+    # variables and a trailing backslash. A raw string compare therefore misses
+    # paths that ARE ours - on a hand-built deployment that means stage 4 fails to
+    # recognise its own application, and the uninstaller fails to recognise what
+    # it may remove.
+    param([string] $Path)
+    if (-not $Path) { return '' }
+    return [System.Environment]::ExpandEnvironmentVariables($Path).TrimEnd('\').ToLowerInvariant()
+}
+
+function Test-SamePath {
+    param([string] $A, [string] $B)
+    return (Resolve-IisPath $A) -eq (Resolve-IisPath $B)
+}
+
 # Paths derived once.
 $Py        = Join-Path $AppRoot 'venv\Scripts\python.exe'
 $Flask     = Join-Path $AppRoot 'venv\Scripts\flask.exe'
@@ -960,7 +976,6 @@ FLUSH PRIVILEGES;
         $rootPass
     ) -Encoding UTF8
     Write-Log "MySQL root password written to $rootFile (Administrators and SYSTEM only)" 'OK'
-    Write-Log 'MYSQLROOTFILE:' + $rootFile
     Write-Log 'stage 0 complete' 'OK'
 }
 
@@ -1084,16 +1099,42 @@ what is on this server, or restore a backup taken before the upgrade.
                     # Uninstall FIRST, while the code is still importable - the
                     # registry entry outlives the directory otherwise, and the app
                     # then fails to load a plugin it still believes is installed.
+                    $deregistered = $false
                     try {
                         if (Test-Path $Flask) {
                             Push-Location $AppRoot
-                            $env:FLASK_APP = 'shopdb'
-                            & $Flask plugin uninstall $dir.Name 2>&1 |
-                                ForEach-Object { Write-Log "    $_" }
-                            Pop-Location
+                            try {
+                                $env:FLASK_APP = 'shopdb'
+                                # --yes is REQUIRED: the command carries a click
+                                # confirmation_option, which aborts with exit 1
+                                # when there is no console to answer the prompt.
+                                # Without it this deregistration could never once
+                                # have succeeded.
+                                #
+                                # EAP bracket for the reason documented on
+                                # Invoke-Native: '2>&1' under EAP 'Stop' turns any
+                                # stderr line into a terminating error, which the
+                                # catch below then downgraded to a WARN while the
+                                # code was deleted regardless.
+                                $prevEap = $ErrorActionPreference
+                                $ErrorActionPreference = 'Continue'
+                                try {
+                                    & $Flask plugin uninstall $dir.Name --yes 2>&1 |
+                                        ForEach-Object { Write-Log "    $_" }
+                                    $deregistered = ($LASTEXITCODE -eq 0)
+                                } finally { $ErrorActionPreference = $prevEap }
+                            } finally { Pop-Location }
                         }
                     } catch { Write-Log "  could not deregister $($dir.Name): $($_.Exception.Message)" 'WARN' }
-                    Remove-Item $dir.FullName -Recurse -Force -ErrorAction SilentlyContinue
+                    # Delete the code ONLY once the registry entry is really gone.
+                    # Deleting it anyway left the application loading a plugin
+                    # whose directory no longer existed - the exact failure the
+                    # uninstall-first ordering above exists to avoid.
+                    if ($deregistered) {
+                        Remove-Item $dir.FullName -Recurse -Force -ErrorAction SilentlyContinue
+                    } else {
+                        Write-Log "  left $($dir.Name) on disk: deregistration did not succeed" 'WARN'
+                    }
                 }
             }
         }
@@ -1832,7 +1873,7 @@ has to come from the bundle either way.
         } else {
             foreach ($site in (Get-Website)) {
                 foreach ($app in (Get-WebApplication -Site $site.Name -ErrorAction SilentlyContinue)) {
-                    if ($app.PhysicalPath -eq $AppRoot) {
+                    if (Test-SamePath $app.PhysicalPath $AppRoot) {
                         $doomed += @{ Kind = 'app'; Name = $app.Path.Trim('/'); Site = $site.Name }
                     }
                 }
@@ -1898,6 +1939,36 @@ If not, re-run matching how the application is published today: pass
         # METHOD A: its own site on $SitePort.
         if (Get-Website -Name $SiteName -ErrorAction SilentlyContinue) {
             Write-Log "site $SiteName already exists" 'OK'
+            # RECONCILE, do not just accept it. "Already exists" used to take the
+            # site however it was, so re-running with a different port left the
+            # binding alone while everything downstream - CORS_ORIGINS, the
+            # firewall rule, the shortcut, the stage 5 smoke test - used the NEW
+            # port. A working server was then reported as a failed install.
+            #
+            # Refuse rather than silently re-bind: moving where people reach the
+            # site is not something to do because a field defaulted.
+            if (-not $WhatIfOnly) {
+                $existingPorts = @()
+                $existingPath  = ''
+                try {
+                    $siteNow = Get-Website -Name $SiteName
+                    $existingPath = [string] $siteNow.physicalPath
+                    foreach ($b in $siteNow.bindings.Collection) {
+                        if ($b.protocol -eq 'http' -and ($b.bindingInformation -match '^[^:]*:(\d+):')) {
+                            $existingPorts += [int] $Matches[1]
+                        }
+                    }
+                } catch { }
+                if (($existingPorts.Count -gt 0) -and ($existingPorts -notcontains [int] $SitePort)) {
+                    Fail ("site '{0}' is bound to port {1}, but this run was told to use {2}" -f `
+                            $SiteName, ($existingPorts -join ', '), $SitePort) `
+                         'Re-run and enter the port the site already uses, or change the binding in IIS Manager first.'
+                }
+                if ($existingPath -and (-not (Test-SamePath $existingPath $AppRoot))) {
+                    Fail ("site '{0}' serves {1}, not {2}" -f $SiteName, $existingPath, $AppRoot) `
+                         'Something else owns that site name. Install into the directory it serves, or use a different site name.'
+                }
+            }
         } else {
             Write-Log "creating site $SiteName on port $SitePort"
             if (-not $WhatIfOnly) {
@@ -2340,11 +2411,19 @@ function Invoke-Uninstall {
         $removed = $false
         foreach ($site in (Get-Website -ErrorAction SilentlyContinue)) {
             $app = Get-WebApplication -Site $site.Name -Name $mount -ErrorAction SilentlyContinue
-            if ($app) {
-                Remove-WebApplication -Site $site.Name -Name $mount -ErrorAction SilentlyContinue
-                Write-Log "removed application /$mount from site '$($site.Name)'" 'OK'
-                $removed = $true
+            if (-not $app) { continue }
+            # OWNERSHIP CHECK. Matching on the alias alone removed ANY application
+            # on the server that happened to share the name - a second instance,
+            # or an unrelated /shopdb under a different site - and -OnFailure
+            # never suppresses the typed confirmation, so it happened unattended.
+            if (-not (Test-SamePath $app.PhysicalPath $AppRoot)) {
+                Write-Log ("left /{0} under '{1}' alone: it serves {2}, not {3}" -f `
+                    $mount, $site.Name, $app.PhysicalPath, $AppRoot) 'WARN'
+                continue
             }
+            Remove-WebApplication -Site $site.Name -Name $mount -ErrorAction SilentlyContinue
+            Write-Log "removed application /$mount from site '$($site.Name)'" 'OK'
+            $removed = $true
         }
         if (-not $removed) { Write-Log "application /$mount not present" }
     }
@@ -2441,6 +2520,20 @@ catch {
         if ($doRollback) { Invoke-Rollback }
         else { Write-Log 'rollback declined; partial install left in place' 'WARN' }
     }
+    # Stage 2 stops the pool on an upgrade so files can be replaced. If the run
+    # then failed, a site that was SERVING before this started is now off, and
+    # saying only "part-configured" left the operator to discover that from users.
+    # Not restarted automatically: after a failure in or past stage 3 the deployed
+    # code and the schema may no longer agree, and starting it would put a broken
+    # application back in front of people. Say it plainly instead.
+    $poolStopped = $false
+    try { $poolStopped = [bool] $script:PoolWasStopped } catch { }
+    if ($poolStopped) {
+        Write-Log "the application pool '$AppPool' was stopped for this upgrade and is STILL STOPPED" 'FAIL'
+        Write-Log '  the site was serving before this run and is now offline' 'FAIL'
+        Write-Log "  once the cause above is fixed, re-run this installer, or start it by hand with:" 'FAIL'
+        Write-Log "    Start-WebAppPool -Name '$AppPool'" 'FAIL'
+    }
     Write-Log "log: $script:LogPath" 'FAIL'
     exit 1
 }

deploy/windows/installer/shopdb-preflight.ps1

@@ -398,7 +398,7 @@ Invoke-Check 'MySQL' 'Backup client' {
         Add-Result 'MySQL' 'Backup client' 'PASS' "mysqldump found ($found)"
     } else {
         Add-Result 'MySQL' 'Backup client' 'WARN' 'mysqldump not found on this server' `
-            'Needed for the automatic pre-upgrade backup and for "shopdb-admin.ps1 backup". A first install works without it; upgrades will not be protected. Add mysqlclient\ to the installer bundle, or install the MySQL client on this server.'
+            'Needed for the automatic pre-upgrade backup and for "shopdb-admin.ps1 backup". A first install works without it; upgrades will not be protected. It is not on this server yet - the installer stages its own copy, so this normally resolves itself during installation.'
     }
 }