Security Hardening 2026 08 Developer Guide

Security hardening 2026-08 — Developer Guide

The second round of the August 2026 security work: nine items raised by Erlend Volden when he re-reviewed our fixes to his original report, plus four bugs in our own first-round code.

The plain-language version is Security hardening 2026-08. The first round is on Security, and the full response document lives in the repository at docs/security-review-2026-08.md.

⚠️ Status: not yet merged. All of this is on security/findings-2026-08, rebased onto main and force-pushed, pending Erlend's re-check. main does not have it yet, so anything below describing current behaviour describes the branch.

Branch note. The F5 commit is absent by design — it reached main earlier as cherry-pick d3d1579f, so the rebase correctly dropped it. Its content is present in the tree. One commit per finding, as before, so each can be reviewed in isolation.

---

1. 📁 The files involved

Colour key: 🗄️ schema · ⚙️ shared service · 🔌 API · 🖥️ page · 🧰 script · 🧪 tests · 🌍 i18n · 📄 docs

🎨	File	What changed
⚙️	includes/tenancy.php	S3. tenancyTablesReady(), tenantCount(), tenancyColumnExists() — fail closed
🔌	api/tickets/save_user.php	S1. Authorise the resolved destination tenant, not the sent one
🔌	api/self-service/change_password.php	R4. session_start() instead of read_and_close
🔌	api/auth/oidc_callback.php	S9. Honour must_change_password
⚙️	includes/encryption.php	R1. New settingsGetDecrypted() / settingGetDecrypted()
🖥️	system/webhooks/index.php	R1. Decrypt before printing the cron URL
🧰	scripts/cron_token.php	R1. New. Prints decrypted cron tokens; --url for full URLs
🔌	api/knowledge/send_share_email.php	R2. Decrypt on read, driven by the rule
🔌	api/knowledge/save_email_settings.php	R2. encryptValue() + isMaskedNoChangeValue()
🔌	api/knowledge/get_email_settings.php	R2. maskSecret() instead of the raw column
⚙️	includes/uploads.php	R3. UPLOAD_TYPES_AUDIO/VIDEO/MAIL, attachmentAllowedTypes()
⚙️	includes/settings_keys.php	R3. Registers attachment_allowed_extensions
⚙️	includes/messaging/ingest.php	R3. Passes the configured allow-list
🔌	api/self-service/create_ticket.php · reply_ticket.php · api/tickets/check_mailbox_email.php	R3. Same
🖥️	system/security/index.php	R3 UI, and minIpAttempts min="0"
🗄️	database/freeitsm.sql	S5. Admin seed sets must_change_password
🗄️	api/system/db_verify.php	S5 catch-up migration · S7 rename collision · R3 seed
⚙️	includes/session_security.php	Strict-overwrite and X-Forwarded-Proto comma-list
🔌	api/myaccount/verify_login_otp.php	unset() ordering vs the audit log
⚙️	config.php · docker/config.php · docker-compose.yml	TRUST_PROXY_HTTPS documented
🧪	tests/security-findings/run.php	Round-two section: round-trip and behavioural assertions
🌍	lang/en/system.php · lang/en/tickets.php	R3 setting strings; corrected SLA help text
📄	docs/*-cron-setup.md (×4) · tickets/help-sla.php	R1. Stop telling readers to SELECT the token

---

2. The two structural lessons

2.1 Storage is half a round-trip

Inverting the secret rule in F3 was correct, but it widened what is ciphertext on disk. Every site still doing a bare SELECT setting_value carried on passing that ciphertext to its consumer. Two features broke (R1, R2) and neither threw.

The first-round test asserted "every secret in the database is ciphertext" — and passed throughout, because a read path that never decrypts is indistinguishable from a correct one when you only look at storage. Assert the whole round-trip.

2.2 A guard's own preconditions are part of the guard

F9 removed the fail-open catch from seven analystCanAccess*() functions. It did not touch tenancyTablesReady(), which every one of them depends on — so the fail-open simply moved up one frame (S3).

---

3. The items

S1 — save_user.php create path

// before
if ($tenantSent && $tenantId !== null && !analystCanAccessTenant($conn, $analystId, $tenantId)) {

Gating on $tenantSent meant an omitted tenant_id skipped the check entirely. The create branch then called resolveTenantForNewUser(), which maps the email domain against tenant_domains. Net effect: an analyst scoped to A posts

{"email":"victim@companyB.com","password":"chosen"}

and receives a portal account owned by B with a known password, then authenticates as it. includes/knowledge/portal_reader.php scopes by the signed-in user's tenant, so B's restricted articles follow.

Pre-existing rather than introduced by F9 — that commit added the analystCanAccessUser() subject check and left this gate as found.

// after
$destinationTenantId = $tenantSent
    ? $tenantId
    : ($id ? null : resolveTenantForNewUser($conn, $email));

if (isMultiTenant($conn)
    && $destinationTenantId !== null
    && !analystCanAccessTenant($conn, (int)$_SESSION['analyst_id'], $destinationTenantId)) {

⚠️ The isMultiTenant() short-circuit is load-bearing. On a single-tenant install an ordinary analyst has no analyst_tenant_access rows, so getAccessibleTenantIds() returns [] and analystCanAccessTenant() denies everything — the suggested fix applied alone would have broken requester creation on every single-company install. Every other caller of that function is an inherently multi-tenant action (move_to_company.php, set_active_tenant.php, move_ticket_to_company.php, resolve_triage.php), which is why none of them needed the guard. It matches what analystCanAccessTicket() and analystCanAccessUser() already do on line 1.

Editing with no tenant_id means "leave the company alone", so there is no destination to authorise; creating with none means "work it out", so there is.

S3 — the master switch

tenancyTablesReady() ended in a bare catch (Exception $e) { $ready = false; }. That value propagates: $ready = false → tenantCount() returns 1 → isMultiTenant() false → every guard returns true on its first line, cached in a static for the request.

So a lock-wait timeout, dropped connection or permissions error on one SELECT 1 FROM tenants disabled all tenant isolation — the precise outcome F9 existed to prevent.

Now only dbErrorIsMissingTable() degrades to "single company". Anything else reports ready and lets the real query fail into tenancyDegradeAllowed(), which denies; deliberately uncached so a transient error cannot pin the answer. tenantCount() reports 2 on an uncountable table for the same reason, and tenancyColumnExists() answers true, since callers read false as "not migrated → allow" (analystCanAccessArticle() returns true outright on it).

Erlend filed this as a follow-up; we pulled it forward to block the merge. Shipping a commit titled "stop the tenant guards failing open" with the switch above them still failing open leaves the finding looking fixed when it is not.

One correction to the report: it says every guard short-circuits to return true. analystCanAccessTenant() actually degrades to [TENANCY_FALLBACK_TENANT_ID] — narrow, not open. The substance holds for analystCanAccessTicket()/analystCanAccessUser(), which is where it matters.

S5 — the Docker default password

docker-compose.yml:31 mounts database/freeitsm.sql at /docker-entrypoint-initdb.d/01-schema.sql. That seed omitted must_change_password, which defaults to 0. Because the row then exists, db_verify.php's COUNT(*) === 0 test is false and the seed that does set the flag never runs. admin / freeitsm stayed valid indefinitely on the flagship path, contradicting README.md.

Both seeds now set it. A catch-up migration handles existing installs and is deliberately narrow — it acts only when the published password still verifies:

if ($defAdmin
    && (int)$defAdmin['must_change_password'] === 0
    && password_verify('freeitsm', (string)$defAdmin['password_hash'])) {

Testing the password rather than the username means an admin who changed theirs is never nagged, and an unrelated account named admin is untouched. Wrapped so dbErrorIsUnknownColumn() is tolerated on a part-migrated install.

Confirmed against a live install that turned out to still be on admin/freeitsm.

S7 — the quarantine migration could destroy a file

@rename($oldAbs, $newAbs) had no destination check. Two attachments on one email collide once both are rewritten: report.htm + report.html → report.bin, as do index.html + index.php. POSIX rename() overwrites silently, and both rows were then updated to the survivor's path. Now it walks a -1, -2, … suffix to a free name; the display filename is a separate column, so the suffix is never surfaced.

The sweep's denylist gains phar, pht, hta, mhtml, svgz, xml, xsl, xslt, cer. It stays a denylist — unlike the ingest side, which is an allow-list — because it inspects names already on disk rather than deciding what may arrive. .xml/.xsl matter: a legacy .xml served as text/xml can carry a self-referencing xml-stylesheet processing instruction and execute script same-origin.

⚠️ Writing that PI into a // comment closes PHP mode. The ?> inside it cost a parse error.

S9 — SSO bypassed the password gate

api/auth/oidc_callback.php set $_SESSION['analyst_id'] without consulting must_change_password, so SSO was the only path around enforcePasswordChangeGate(). It now sets $_SESSION['password_expired'] from the flag.

Only the flag, never the expiry policy — an SSO account may have no local password to expire, and auth/login.php draws the same distinction for LDAP via $skipPasswordExpiry. The reasoning: the flag marks an account whose local password is the published default, and that password keeps working at auth/login.php regardless of how the analyst arrived.

oidcLoadAnalyst() uses SELECT *, so the column is already present.

R1 — cron tokens unreadable where a human must read them

isEncryptedSettingKey() matches /_(password|secret|token|api_key)$/i, so all four *_cron_token values are encrypted in place. All four workers decrypt correctly. The break was confined to the human-facing side:

Site	Problem
system/webhooks/index.php	Raw SELECT, no encryption.php include; ?token=ENC%3A… in the copy-paste URL, worker 403s
docs/sla-cron-setup.md · docs/webhook-cron-setup.md · tickets/help-sla.php	Told the reader to SELECT setting_value
docs/sla-cron-setup.md	Said to rotate by UPDATE — works, but silently returns the column to cleartext until the next Verification
docs/workflow-scheduled-cron-setup.md · docs/integration-poll-cron-setup.md	Named the column without a query — vague rather than wrong

New in includes/encryption.php:

function settingsGetDecrypted(PDO $conn, array $keys): array
function settingGetDecrypted(PDO $conn, string $key, string $default = ''): string

Anything reading a setting in order to use it goes through these. decryptValue() passes plaintext through, so they are safe pre-migration, on uncovered keys, and on re-runs. A value that will not decrypt logs and returns '' rather than the ciphertext — the failure mode being fixed.

scripts/cron_token.php is CLI-only, since the point of the encryption is that reading the row is not enough.

A sweep for other raw readers of encrypted settings found none. The three save_ai_settings.php writers all call encryptValue(), and test_ai_key.php / test_ai_connection.php / includes/rfp_ai.php all decrypt. R1 and R2 were the only two.

R2 — knowledge_email_smtp_password

Three-sided:

• Read — send_share_email.php built $settings from a raw LIKE 'knowledge_email_%' query and passed it to sendSmtpEmail(), authenticating with the literal ENC:…. Now decrypts per row on isEncryptedSettingKey() rather than a hardcoded key name, so a future secret under the prefix is covered.
• Write — save_email_settings.php stored it raw while wrapping ai_api_key and openai_api_key in encryptValue() two lines below. The value therefore oscillated (encrypted by Verify, plaintext by Save) rather than failing consistently, which is why it survived review.
• Display — get_email_settings.php returned the column verbatim: plaintext before the rule, ciphertext after. Now maskSecret(decryptValue(...)), with isMaskedNoChangeValue() on the save side so a form posted back untouched does not store ****abcd as the password.

R3 — the attachment allow-list

UPLOAD_TYPES_ATTACHMENT was UPLOAD_TYPES_DOCUMENT + UPLOAD_TYPES_IMAGE. No caller passes a custom list, so it governed the messaging media path too. Nine of the nineteen extensions messagingExtForMime() can emit were rejected:

accepted (10)	doc docx gif jpg pdf png txt webp xls xlsx
rejected (9)	3gp aac amr heic m4a mp3 mp4 ogg vcf

So a WhatsApp voice note (audio/ogg), video (video/mp4), iPhone photo (image/heic) and contact card (text/vcard) were quarantined by the module built to receive them, and six ATTACHMENT_SERVE_TYPES entries were unreachable.

Added UPLOAD_TYPES_AUDIO, UPLOAD_TYPES_VIDEO, UPLOAD_TYPES_MAIL, plus heic/heif, and application/CDFV2 on doc/xls/ppt — modern libmagic reports that for unclassifiable OLE2 files, so genuine Office documents were failing gate 2. CDFV2 is listed only against extensions already on the allow-list, never accepted globally, since it is equally the shape of .msi and .msg.

Which types an install accepts is now attachment_allowed_extensions, read by attachmentAllowedTypes():

$types = array_intersect_key(UPLOAD_TYPES_ATTACHMENT, array_flip($wanted));
return $cached = ($types ?: UPLOAD_TYPES_ATTACHMENT);

⚠️ It can only narrow. The value is intersected with the catalogue, so an unknown extension has no mime list to be validated against and is discarded — php in the box is inert. Empty means the whole catalogue, so installs inherit future safe types rather than freezing at their install date; an emptied box returns the default rather than refusing everything, which would be indistinguishable from the product being broken.

Verified against attachment_allowed_extensions = 'pdf, png, php, phtml, exe' → effective list pdf, png.

---

4. Bugs in the first round's own code

Where	Problem
sessionCookieParamsAreHardened()	Required samesite === 'Lax' exactly, so an admin-configured Strict failed the check and line 122 then overwrote their Strict cookie with Lax. Now "at least as strong as" — Lax or Strict pass, and Secure on when only off is required is not a failure
requestIsHttps()	=== 'https' against X-Forwarded-Proto, which chained proxies append to (https, http). Now takes the first comma-separated token
TRUST_PROXY_HTTPS	Appeared only in a code comment — not config.php, docker/config.php, README.md or docker-compose.yml. The commonest production topology (this image behind nginx/Traefik/Caddy) would therefore ship cookies without Secure indefinitely. Documented in all three, still off by default since the header is client-settable
verify_login_otp.php	Read $_SESSION['mfa_pending_username'] after the unset() that removed it, so every abandoned-MFA row in login_attempts logged unknown

---

5. 🧪 Testing

php tests/security-findings/run.php http://localhost/freeitsm-app/ → 131 pass, 0 fail.

The round-one suite was largely strpos() over source text, which is why it certified R4 while the call did nothing. The new section asserts behaviour:

• every cron token and csat_token_secret is ciphertext at rest and plaintext through settingsGetDecrypted() — the missing half;
• uploadStoreBytes() fed structurally valid Ogg/Opus, MP4, 3GP, HEIC, vCard, iCalendar and RFC822 bytes, asserting on the returned stored_name;
• against negative controls — shell.php, shell.phtml, evil.svg, page.html, .htaccess, and a traversal-plus-null-byte name — that must still land on .bin;
• the effective attachment list is a subset of the catalogue and reaches nothing executable;
• save_user.php authorises the resolved destination and still short-circuits on single-tenant;
• the tenancy helpers forgive only a missing table;
• the SQL seed sets the flag, and an admin still on the published password has been flagged.

The existing "rotates the session id" check now also asserts the file keeps the session open, so R4's exact shape cannot pass again.

⚠️ A malformed fixture failed first and looked like a product bug. A hand-built .ogg that was not a valid Ogg stream detected as application/octet-stream and was quarantined. Real Ogg/Opus and Ogg/Vorbis pages both detect as audio/ogg. Build structurally valid fixtures, or you debug your own test.

R4 was proved with both controls. Against the unfixed code the response carried no Set-Cookie at all and the old session file survived on disk; against the fix the id rotates and session_regenerate_id(true) deletes the old file.

---

6. 🔴 Outstanding

• S2 — four sibling endpoints, the next piece of work. api/tickets/delete_user.php (no tenancy check at all; its refusal text is also a cross-tenant oracle), api/v1/resources/users.php (apiKeyCanAccessTenantRow() exists and is simply not called), api/integrations/escalate_ticket.php (preview=1 returns before the service, so the "every check lives in the service" invariant holds for writes but not previews), api/messaging/test_channel.php and slack_diagnose.php. None are regressions and the branch does not touch them — folding an unrelated sweep into a branch under review makes it harder to review.
• S6 — MFA counter. Session-scoped, and a successful password step resets failed_login_count and locked_until, while recordIpAttempt() only fires for an already-locked account or an unknown username. An attacker holding a valid password loops for unlimited guesses at ~25% more requests. Needs a per-account or per-IP counter in the database — a design change, not a correction.
• S4 — CSRF. request_guard.php refuses text/plain only. application/x-www-form-urlencoded is CORS-safelisted too, so relabelling the same body is a no-preflight request and the endpoint's json_decode(file_get_contents('php://input')) parses it. Unique coverage is the JS-free form variant. The write-up now says this rather than claiming a second line of defence, and two related claims are corrected: the front end does not send application/json at all fetch sites, and auth/login.php is a tokenless form POST. The token layer remains the actual fix.
• S9 remainder — ticket_merge.php hand-writing .html; unauthenticated PHP version/extension disclosure in setup/; SVG accepted by save_branding.php; IIS <handlers><clear/> returning 500.19 rather than a clean deny; get_attachment.php lacking realpath() containment.
• From round one — uploadStoreFile() still skips the content check without finfo where its sibling fails closed; lms/content and system/uploads/branding have no deny-all.

---

7. 🚑 If something breaks after this

Most of this round tightened things, and five changes can now refuse something that previously worked. If a regression turns up, start here rather than in the diff — the symptom rarely names the cause.

7.1 The five changes that can newly deny

Symptom a user reports	Likely cause	Confirm it
"I can't create a customer contact" / You do not have access to that company	S1. The analyst genuinely lacks the destination company, or the email domain resolves to one they can't reach	Multi-company installs only. Check analyst_tenant_access for that analyst, and tenant_domains for the email domain
Records intermittently 404 / not found that used to load	S3. The tenancy check is now denying on a database error instead of allowing	grep 'tenancy:' the PHP error log — see 7.2
Admin is forced to change password unexpectedly	S5. The account was still on the published freeitsm password	Intended. SELECT must_change_password FROM analysts WHERE username='admin'
SSO users hit the change-password screen	S9. Their account carries must_change_password	Intended. Same query for their account
An attachment type stopped being accepted	R3, only if someone has set attachment_allowed_extensions	SELECT setting_value FROM system_settings WHERE setting_key='attachment_allowed_extensions' — empty means everything safe, which is the default

⚠️ S3 is the one to watch. It deliberately converted a fail-open into a fail-closed. On a healthy install it changes nothing, but on one with a genuinely odd schema state it will now deny where it used to wave things through. That is the correct direction, but it means a database problem now looks like a permissions problem.

7.2 The log lines to grep

Every new failure path logs. None of them is silent, which was a deliberate response to R4 having failed silently.

tenancy: tenants table unreadable (…)      ← S3. Something is wrong with the DB, not with permissions
tenancy: could not count tenants (…)       ← S3, same
tenancy: could not inspect <table>.<col>   ← S3, same
tenancy: denying access after an unexpected database error   ← the F9 path, unchanged
settings: could not decrypt <key>          ← R1. Encryption key mismatch — see 7.3
knowledge email: could not decrypt <key>   ← R2, same cause
session_security: cannot rotate the session id, output already started at …

If you see any tenancy: line, treat it as a database fault. The access denial is a symptom, not the problem.

7.3 The one genuinely new failure mode

settingsGetDecrypted() returns '' when a value will not decrypt, rather than passing the ciphertext through. That is the right call — handing ENC:… to an SMTP server is what R2 was — but it changes the shape of the failure:

• Before: a broken secret surfaced as a visibly wrong value (ENC:… in a URL).
• Now: it surfaces as an empty value, plus a log line.

So "the cron token is blank" and "the SMTP password is empty" both mean the encryption key does not match the data, not the setting is unset. Usual cause: the key file at ENCRYPTION_KEY_PATH was replaced or lost. php scripts/cron_token.php will show (not seeded…) for the same reason.

7.4 Backing a change out

If one of these has to go in a hurry, each is independently revertable — they were committed one per finding for exactly this reason.

git log --oneline main..security/findings-2026-08
git revert <sha>

Two that need care rather than a plain revert:

• S5 — reverting the code does not clear a must_change_password already set. UPDATE analysts SET must_change_password = 0 WHERE id = ? if you need the account back immediately.
• R3 — reverting narrows the allow-list again, which will start quarantining media that has been arriving fine. Prefer setting attachment_allowed_extensions to the narrower list you want and leaving the code alone.

S3 should not be reverted. If it is causing denials, the database fault it is reporting is the thing to fix.

7.5 First thing to run

php tests/security-findings/run.php http://localhost/freeitsm-app/

131 checks. It exercises behaviour rather than source text, so a genuine regression in any of this round should fail it rather than pass quietly — which is the whole point of the rewrite.

---

Related pages

• Security hardening 2026-08 — the same items, in plain English
• Security · Database Verification — Developer Guide
• Admin Access Control · Roles & Permissions