fix(installer): correct a false security claim, and clear the should-fix list
Some checks failed
CI / backend (push) Failing after 8s
CI / naming (push) Successful in 2s
CI / frontend (push) Successful in 9s
CI / migrations-mysql (push) Failing after 7s

CLIENT IP / SPOOFABILITY. docs/geenforce-api-cutover.md claimed that removing the
IIS rewrite rule made the allowlist fail closed and that it does NOT become
spoofable. The opposite is true. IIS never sets X-Forwarded-For on its own; the
rule is the only thing that does. Remove it and IIS still forwards whatever
X-Forwarded-For the CALLER sent, waitress trusts it because it arrives from
127.0.0.1, and remote_addr becomes attacker-controlled - so a token-less caller
can fetch manifests from anywhere on the network. The document and the
_trusted_client_ip docstring now say so, waitress runs with
--trusted-proxy-count=1, and stage 5 checks the rule is actually live rather than
assuming it. The wizard question is rephrased to something an operator can verify
with their network team instead of guessing at.

NON-ASCII. The style gate only ever checked .py/.vue/.js/.ts, so documentation
accumulated em-dashes, arrows and box-drawing characters against this repo's own
convention - including in files added this week. Cleaned, and the gate now uses
INCLUDES_ALL so Markdown, JSON and YAML are covered.

PLUGIN DEFAULTS. The wizard pre-ticked measuringtools and printedparts, both of
which ship default_enabled=false, so every site taking the defaults installed and
enabled them against their manifests. Inno has no JSON parser so the list must be
hardcoded, but tests/test_installer_defaults.py now fails when it drifts.

UPGRADES. The payload copy merges, so a plugin dropped from a site's profile kept
its code forever - which defeats a lean build and leaves core's optional-import
guards succeeding for a plugin the site no longer has. Stale plugin directories
are now deregistered and removed before the copy.

add-plugin used 'plugin install', which for the five default_enabled=false
plugins left them installed but DISABLED - and printed a green success line
anyway. It now goes through apply-profile, and the success line is gated on the
exit code. Invoke-Flask records its own exit status, because $LASTEXITCODE keeps
a stale value when flask.exe is missing and no native command runs.

CHARSET. The utf8mb4 compiler hook lived inline in migrations/env.py, so it
covered the CORE chain only: plugin baselines inherited the server default, which
on a latin1 server means two charsets in one database. It is now
shopdb/utils/mysql_charset.py, imported by both, and preflight reports the
database's default charset.

BACKUP HONESTY. The dump was described as 'all of your asset data'. Uploaded
branding and floor-map images live in instance\ on disk, not in the database, so
a restore from the .sql alone comes back with no map. backup now archives
instance\ alongside it and says both are needed.

VERSIONING. AppVersion was hardcoded at 0.9.0 while the product, the frontend and
the newest tag said 0.7.0 - and 0.9.0 collides with a retired contract version.
Both builders now generate version.iss from shopdb/__init__.py.

Smaller: rollback overwrites .env before deleting it, as uninstall already did;
appcmd unlocks are scoped to this site's location rather than server-wide, with
the wide unlock as a fallback; DEVELOPMENT-SETUP says Python 3.14; the README
plugin list gains printedparts; prune-schema --force is documented as
first-provisioning-only; HTTPS is documented as not-the-default with the steps to
add it; the DBA SQL is on the wizard's database page; the features page says
unticking does not remove an installed feature; and the installer README states
that bundle-lock cannot vouch for the exe itself - that needs signing or an
out-of-band hash, neither of which is wired up.
This commit is contained in:
cproudlock
2026-08-03 14:57:38 -04:00
parent aea2905de0
commit 2c415a1712
24 changed files with 582 additions and 119 deletions

View File

@@ -3,3 +3,4 @@ bundle/
Output/
# Generated by build-installer.sh from the staged bundle.
plugins.iss
version.iss

View File

@@ -94,6 +94,23 @@ Windows wheelhouse. A Linux-only resolve had silently omitted `colorama`, a
win32-only dependency of `click` - which in hash-checking mode is a hard error
rather than a quiet omission.
### Verifying the installer itself
`bundle-lock.json` is **inside** the thing it describes, so it proves the payload
was not altered between build and install - not that the `.exe` you received is
the one that was built. That needs something out of band. Two options, in order
of preference:
1. **Authenticode-sign the `.exe`** with a GE code-signing certificate. Windows
then shows a real publisher instead of "Unknown", which is also what stops an
operator learning to click through the SmartScreen warning.
2. **Publish a sha256 per release** through a different channel than the file
itself, and have the receiving site check it:
`Get-FileHash ShopDBFlask_Installer_*.exe -Algorithm SHA256`
Neither is wired up yet. Until one is, an installer is only as trustworthy as
the share it arrived on.
### Changing what ships
```powershell
@@ -145,10 +162,10 @@ Every build stages a CycloneDX 1.6 SBOM at `sbom.cdx.json`, inside the
application tree, so it installs onto the server with the app. Both ecosystems,
in one document:
- **Python** every pin in `requirements.txt`, with the sha256 the installer
- **Python** - every pin in `requirements.txt`, with the sha256 the installer
enforces. Environment markers are ignored: a `sys_platform == 'win32'`
dependency still installs on the target.
- **npm** every package in `frontend/package-lock.json`. Build-only packages
- **npm** - every package in `frontend/package-lock.json`. Build-only packages
are marked `scope: excluded` rather than dropped, so "not here" is
distinguishable from "not looked for".
@@ -161,7 +178,7 @@ shopdb-admin.ps1 verify -Path leaflet # is that component here, at what versio
```
Generated by `scripts/generate_sbom.py` from files that are already pinned and
committed, so it is a translation rather than a scan no network, no extra
committed, so it is a translation rather than a scan - no network, no extra
toolchain on the build box, and byte-identical output for the same inputs. It is
deliberately not in `bundle-lock.json`: its provenance is git, not the payload.

View File

@@ -18,11 +18,16 @@
#endif
#define AppName "ShopDB-Flask"
; Pre-release. This has not shipped, so it is 0.x by definition. It becomes
; 1.0.0 when a real site installs from it successfully - not before.
; Do NOT bump this to work around a locked output file: the BUILD STAMP below
; makes every compile a unique filename, which is what that problem needed.
#define AppVersion "0.9.0"
; The PRODUCT version, generated into version.iss by the builder from
; shopdb/__init__.py. It was hardcoded, and had drifted to 0.9.0 while the
; product, the frontend and the newest tag all said 0.7.0 - so the delivered exe,
; its Add/Remove Programs entry and the version stamp written onto the server all
; disagreed with the code inside it, and 0.9.0 collided with a retired contract
; version. Never hardcode it here again.
;
; Do NOT bump a version to work around a locked output file: the BUILD STAMP
; below makes every compile a unique filename, which is what that problem needed.
#include "version.iss"
#define AppPublisher "GE Aerospace"
#define BundleDir "bundle"
; Single source for the install directory - used by DefaultDirName and by the
@@ -239,9 +244,18 @@ end;
// True when a fresh install should tick this by default. The five omitted here
// are specialised; a site that wants them can tick them.
// Pre-tick to match each plugin's own manifest. These five ship
// "default_enabled": false, so a site gets them only by asking. The list used to
// omit measuringtools and printedparts, which meant the wizard installed and
// enabled them against their manifests on every site that took the defaults -
// creating tables nobody asked for.
//
// Keep this in step with the manifests. tests/test_installer_defaults.py fails
// if they drift.
function PluginDefault(const Name: String): Boolean;
begin
Result := (Name <> 'usb') and (Name <> 'employees') and (Name <> 'geenforce');
Result := (Name <> 'usb') and (Name <> 'employees') and (Name <> 'geenforce')
and (Name <> 'measuringtools') and (Name <> 'printedparts');
end;
// Comma-separated list of what the operator ticked.
@@ -372,8 +386,10 @@ begin
// have and ticking a new box adds it.
PluginPage := CreateInputOptionPage(PreflightPage.ID,
'Features', 'Which parts of ShopDB-Flask does this site use?',
'Everything is included in this installer. Unticked features are simply not ' +
'set up, and their database tables are not created.',
'Unticked features are simply not set up, and their database tables are not ' +
'created. On an upgrade this shows what the site already has - ticking a new ' +
'box adds it, but UNTICKING one does NOT remove a feature that is already ' +
'installed. Removing is a deliberate step, not a side effect of an upgrade.',
False, True);
PluginNames := StringSplit('{#AvailablePlugins}', [','], stAll);
for I := 0 to GetArrayLength(PluginNames) - 1 do
@@ -394,7 +410,11 @@ begin
'Existing database', 'Connection details',
'The database and application user must already exist. Your DBA creates ' +
'them; the installer does not, so that it never needs administrative ' +
'rights on your database server.');
'rights on your database server. If they need the exact statements:' + #13#10 +
' CREATE DATABASE shopdb_flask CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci;' + #13#10 +
' CREATE USER ''shopdb''@''%'' IDENTIFIED BY ''<password>'';' + #13#10 +
' GRANT ALL PRIVILEGES ON shopdb_flask.* TO ''shopdb''@''%'';' + #13#10 +
'The utf8mb4 charset matters - the default on older servers is latin1.');
DbDetailsPage.Add('Host:', False);
DbDetailsPage.Add('Port:', False);
DbDetailsPage.Add('Database:', False);
@@ -445,12 +465,17 @@ begin
// wrong behind a proxy (REMOTE_ADDR is the proxy, so the real client IP is
// discarded). Hence a question rather than a default.
ClientIpPage := CreateInputOptionPage(SitePage.ID,
'Client addresses', 'How does this server see who is connecting?',
'Client addresses', 'Does anything sit between your users and this server?',
'ShopDB records the address of every request, and some features decide what '
+ 'to show based on it. Choose whichever describes this server.',
+ 'to show based on it. If you are not sure, choose the first option - it is '
+ 'correct for a server users reach directly.',
True, False);
ClientIpPage.Add('Clients connect to this server directly (installs URL Rewrite)');
ClientIpPage.Add('A proxy or load balancer sits in front of this server');
ClientIpPage.Add('No - users reach this server directly (installs URL Rewrite)');
// Phrased as "already adds the visitor's address" rather than "is a proxy":
// the operator can check that with their network team, whereas "is there a
// proxy" invites a guess, and guessing wrong here silently discards the real
// client address on every request.
ClientIpPage.Add('Yes - a load balancer or gateway that already adds the visitor''s address');
ClientIpPage.SelectedValueIndex := 0;
end;

View File

@@ -220,6 +220,17 @@ if (Test-Path $aliasFile) { $aliasBuilt = (Get-Content $aliasFile -TotalCount 1)
"@ | Set-Content -Path (Join-Path $PSScriptRoot 'plugins.iss') -Encoding ASCII
Say " $shipped" 'White'
# The product version, read from the code rather than restated in the .iss.
Step 'Writing version.iss'
$initText = Get-Content (Join-Path $RepoRoot 'shopdb\__init__.py') -Raw
if ($initText -notmatch "(?m)^__version__\s*=\s*'([^']+)'") { Die 'could not read __version__ from shopdb\__init__.py' }
$appVersion = $Matches[1]
@"
; GENERATED by build-installer.ps1 from shopdb/__init__.py - do not edit.
#define AppVersion "$appVersion"
"@ | Set-Content -Path (Join-Path $PSScriptRoot 'version.iss') -Encoding ASCII
Say " $appVersion" 'White'
# --- 5. installer scripts ---------------------------------------------------
# From THIS directory, which is the reviewed copy under version control. They
# used to be copied from a downloads folder, so the logic that shipped was not

View File

@@ -59,6 +59,17 @@ cat > "$HERE/plugins.iss" <<EOF
EOF
echo " $PLUGINS"
# The product version, read from the code rather than restated here. A hardcoded
# AppVersion in the .iss had drifted two minor versions from shopdb/__init__.py.
echo "==> Writing version.iss"
APPVERSION=$(sed -n "s/^__version__ = '\\(.*\\)'/\\1/p" "$REPO/shopdb/__init__.py" | head -1)
[ -n "$APPVERSION" ] || { echo "could not read __version__ from shopdb/__init__.py"; exit 1; }
cat > "$HERE/version.iss" <<EOF
; GENERATED by build-installer.sh from shopdb/__init__.py - do not edit.
#define AppVersion "$APPVERSION"
EOF
echo " $APPVERSION"
# From THIS directory, which is the reviewed copy under version control. These
# used to be copied from $HOME/Downloads, so the installer logic that shipped was
# not the logic that was committed, and the build only worked on one machine.

View File

@@ -406,7 +406,31 @@ function Backup-Db {
$tail = @(Get-Content $file -Tail 5 -ErrorAction SilentlyContinue)
if ((Test-Path $file) -and ((Get-Item $file).Length -gt 1024) -and ($tail -match 'Dump completed')) {
Say (" done - {0:N1} MB, verified complete" -f ((Get-Item $file).Length / 1MB)) 'Green'
Say ' Store this off the server. It contains all of your asset data.' 'Yellow'
# The dump is NOT everything. Uploaded branding, map blueprints and
# generated files live in instance\ on disk, not in the database, so a
# restore from the .sql alone comes back with no floor map. Saying "all of
# your asset data" was true and misleading at the same time.
$instance = Join-Path $AppRoot 'instance'
if (Test-Path $instance) {
$zip = [System.IO.Path]::ChangeExtension($file, $null) + 'instance.zip'
try {
Add-Type -AssemblyName System.IO.Compression.FileSystem -ErrorAction SilentlyContinue
if (Test-Path $zip) { Remove-Item $zip -Force }
[System.IO.Compression.ZipFile]::CreateFromDirectory($instance, $zip)
Say (" uploaded files: {0:N1} MB -> {1}" -f ((Get-Item $zip).Length / 1MB), (Split-Path $zip -Leaf)) 'Green'
} catch {
Say (" could not archive instance\: {0}" -f $_.Exception.Message) 'Yellow'
Say ' copy it by hand - it holds branding and floor-map images' 'Yellow'
}
}
Say ''
Say ' Store BOTH files off this server. The .sql holds the records; the' 'Yellow'
Say ' .zip holds uploaded branding and floor-map images, which the' 'Yellow'
Say ' database does not. A restore needs both.' 'Yellow'
Say ''
Say ' Contains user password hashes - treat it as sensitive.' 'Yellow'
} else {
Say ' backup is empty or truncated - do NOT rely on it' 'Red'
Remove-Item $file -Force -ErrorAction SilentlyContinue
@@ -525,17 +549,30 @@ function Show-Sessions {
}
# Set by every Invoke-Flask call. Callers MUST test this rather than
# $LASTEXITCODE: when flask.exe is missing no native command runs at all, so
# $LASTEXITCODE keeps whatever value it had from something earlier - which reads
# as success and made a command that did nothing report that it had worked.
$script:LastFlaskExit = 0
function Invoke-Flask {
param([string[]] $Arguments)
$flask = Join-Path $AppRoot 'venv\Scripts\flask.exe'
if (-not (Test-Path $flask)) { Say ' application not installed' 'Red'; return $null }
if (-not (Test-Path $flask)) {
Say ' application not installed' 'Red'
$script:LastFlaskExit = 127
return $null
}
Push-Location $AppRoot
$env:FLASK_APP = 'shopdb'
# The app logs plugin startup to STDERR even on success; with EAP=Stop that
# becomes a terminating error and a healthy command looks like a failure.
$prev = $ErrorActionPreference
$ErrorActionPreference = 'Continue'
try { & $flask @Arguments 2>&1 }
try {
& $flask @Arguments 2>&1
$script:LastFlaskExit = $LASTEXITCODE
}
finally { $ErrorActionPreference = $prev; Pop-Location }
}
@@ -578,10 +615,47 @@ function Add-Plugin {
return
}
Say (" installing {0}..." -f $Name)
Invoke-Flask @('plugin','install',$Name) | ForEach-Object { Say " $_" }
# apply-profile, not plugin install. Five plugins ship default_enabled=false,
# so `install` alone left them installed-but-disabled: the command printed a
# green success line and the feature did not appear anywhere in the UI.
# apply-profile installs AND enables, and pulls in the dependency closure.
Say (" adding {0}..." -f $Name)
$profilePath = Join-Path $AppRoot 'site-profile.json'
$applied = $false
if (Test-Path $profilePath) {
try {
$profile = Get-Content $profilePath -Raw | ConvertFrom-Json
$wanted = @($profile.plugins)
if ($wanted -notcontains $Name) { $wanted += $Name }
$profile.plugins = $wanted
$profile | ConvertTo-Json -Depth 6 | Set-Content -Path $profilePath -Encoding UTF8
Invoke-Flask @('plugin','apply-profile',$profilePath) | ForEach-Object { Say " $_" }
$applied = ($script:LastFlaskExit -eq 0)
} catch { Say (" could not update site-profile.json: {0}" -f $_.Exception.Message) 'Yellow' }
}
if (-not $applied) {
# No profile on disk, or apply-profile failed: fall back, but enable
# explicitly so the outcome is the same either way.
Invoke-Flask @('plugin','install',$Name) | ForEach-Object { Say " $_" }
$installed = ($script:LastFlaskExit -eq 0)
Invoke-Flask @('plugin','enable',$Name) | ForEach-Object { Say " $_" }
$applied = $installed -and ($script:LastFlaskExit -eq 0)
}
if (-not $applied) {
# Do NOT print the green line on a failure. It used to be unconditional,
# so a failed add reported success and the operator went looking for a
# feature that was never enabled.
Say (" {0} was NOT added - see the output above" -f $Name) 'Red'
return
}
Say ' applying its database migrations...'
Invoke-Flask @('plugin','upgrade-all') | ForEach-Object { Say " $_" }
if ($script:LastFlaskExit -ne 0) {
Say ' migrations FAILED - the feature is installed but its tables are not' 'Red'
Say ' do not use it until this is resolved; restore a backup if needed' 'Red'
return
}
Say ' restarting so its routes register...'
Restart-App
Say (" {0} added" -f $Name) 'Green'

View File

@@ -890,6 +890,42 @@ what is on this server, or restore a backup taken before the upgrade.
} catch { }
}
# Plugin directories present on the server but NOT in the new bundle are
# removed BEFORE the copy. The copy merges rather than replaces, so a plugin
# dropped from a site's profile kept its code on disk forever - which defeats
# the point of a lean build (ADR-013) and, worse, leaves the core's
# `try: from plugins.X.models import ...` guards succeeding for a plugin this
# site no longer has, so SQLAlchemy still builds the relationship and queries
# still join a table that may have been pruned.
if (-not $WhatIfOnly) {
$newPlugins = @()
$srcPlugins = Join-Path $AppSource 'plugins'
if (Test-Path $srcPlugins) {
$newPlugins = @(Get-ChildItem $srcPlugins -Directory | Select-Object -ExpandProperty Name)
}
$livePlugins = Join-Path $AppRoot 'plugins'
if ($newPlugins.Count -gt 0 -and (Test-Path $livePlugins)) {
foreach ($dir in (Get-ChildItem $livePlugins -Directory)) {
if ($newPlugins -notcontains $dir.Name) {
Write-Log "removing '$($dir.Name)': not in this build" 'WARN'
# 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.
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
}
} catch { Write-Log " could not deregister $($dir.Name): $($_.Exception.Message)" 'WARN' }
Remove-Item $dir.FullName -Recurse -Force -ErrorAction SilentlyContinue
}
}
}
}
Write-Log "copying application payload to $AppRoot"
if (-not $WhatIfOnly) {
Copy-Item (Join-Path $AppSource '*') -Destination $AppRoot -Recurse -Force
@@ -1533,9 +1569,21 @@ has to come from the bundle either way.
# Handler sections are locked server-wide by default. Without unlocking,
# IIS returns 500.19 the moment it reads the app's web.config.
#
# `appcmd unlock config /section:X` unlocks it for EVERY site on the machine,
# which on a shared server hands every other application the ability to
# define its own handlers. Delegate to our own location instead, and fall
# back to the server-wide unlock only if that is refused - a 500.19 the
# operator cannot diagnose is worse than a wider delegation, but it should be
# the second choice, not the first.
$ourLocation = if ($MountAlias) { "$ParentSite/$($MountAlias.Trim('/'))" } else { $SiteName }
foreach ($section in @('system.webServer/handlers','system.webServer/httpPlatform')) {
Write-Log "unlocking $section"
if (-not $WhatIfOnly) {
if ($WhatIfOnly) { Write-Log "would unlock $section for $ourLocation"; continue }
Write-Log "unlocking $section for $ourLocation"
$scoped = & $appcmd unlock config "$ourLocation" /section:$section 2>&1
$scoped | ForEach-Object { Write-Log " $_" }
if ($LASTEXITCODE -ne 0) {
Write-Log " scoped unlock refused; unlocking $section server-wide" 'WARN'
& $appcmd unlock config /section:$section 2>&1 | ForEach-Object { Write-Log " $_" }
}
}
@@ -1783,6 +1831,29 @@ public class ShopdbSmokeTestCertPolicy : ICertificatePolicy {
}
}
# Is the client-IP rule actually in effect? It is the control that makes
# remote_addr trustworthy: without it IIS forwards whatever X-Forwarded-For
# the CALLER sent, waitress trusts it because it arrives from loopback, and
# the GE-Enforce IP allowlist becomes bypassable from anywhere on the network.
# It does not fail closed. Verified here rather than assumed, because the
# failure is silent and looks exactly like a working server.
if ($ClientIpSource -eq 'direct') {
$liveCfg = Join-Path $AppRoot 'web.config'
$cfgText = if (Test-Path $liveCfg) { Get-Content $liveCfg -Raw } else { '' }
$ruleLive = ($cfgText -match '<rewrite>') -and ($cfgText -notmatch 'SHOPDB-CLIENTIP-BEGIN')
$moduleThere = Test-Path (Join-Path $env:windir 'system32\inetsrv\rewrite.dll')
if ($ruleLive -and $moduleThere) {
Write-Log 'client-IP rule is live; remote_addr is trustworthy' 'OK'
} else {
Write-Log 'CLIENT-IP RULE IS NOT IN EFFECT' 'WARN'
if (-not $moduleThere) { Write-Log ' URL Rewrite module is not installed' 'WARN' }
if (-not $ruleLive) { Write-Log ' web.config does not have an active <rewrite> block' 'WARN' }
Write-Log ' every client will be recorded as 127.0.0.1, and the GE-Enforce IP' 'WARN'
Write-Log ' allowlist is BYPASSABLE - a caller can send its own X-Forwarded-For' 'WARN'
Write-Log ' re-run with -ClientIpSource direct, or use token authentication' 'WARN'
}
}
# The self-hosted API documentation. It is the thing an operator - or the
# assistant an operator is asking - reaches for on a server with no internet,
# and it is served from a file that has to have been staged into the build.
@@ -1854,7 +1925,18 @@ function Invoke-Rollback {
}
'apppool' { Remove-WebAppPool -Name $item.Id -ErrorAction SilentlyContinue; Write-Log " removed app pool $($item.Id)" }
'firewall' { Remove-NetFirewallRule -DisplayName $item.Id -ErrorAction SilentlyContinue; Write-Log " removed firewall rule" }
'file' { Remove-Item $item.Id -Force -ErrorAction SilentlyContinue; Write-Log " removed $($item.Id)" }
'file' {
# Overwrite before deleting, same as uninstall does. A rolled
# back install left .env - which holds the database password
# in plaintext by design - recoverable on disk.
try {
if ((Split-Path $item.Id -Leaf) -eq '.env') {
Set-Content -Path $item.Id -Value ('0' * 512) -Encoding UTF8 -Force
}
} catch { }
Remove-Item $item.Id -Force -ErrorAction SilentlyContinue
Write-Log " removed $($item.Id)"
}
'dir' { Write-Log " left directory $($item.Id) in place (remove manually if wanted)" 'WARN' }
'python' { Write-Log " left Python install in place at $($item.Id)" 'WARN' }
'service' {