v9.5.3 Fix/security/q3 high audit findings

[tpurschke]

Fixes five findings from the Q3 security audit. Each fix ships with regression tests that were verified to fail against the pre-fix code.

SEC-01 — Auditor could escalate to admin by rewriting its own identity (Critical)

• auditor was the only non-admin role allowed to update the public.uiuser columns that define who an account is: uuid, uiuser_username, tenant_id, ldap_connection_id and the password flags — an outlier, since all ten other non-admin roles had uiuser_language only.
• uiuser.uuid is the DN that login and token refresh resolve against LDAP to derive roles, so rewriting it had the caller's authorization rebuilt as a different subject.
• The uuid UNIQUE constraint does not prevent this: the post-change lookup is by DN and finds the caller's own row, so createIfMissing: false never trips. Reachable targets are an admin account that has never logged into the UI, or a role-bearing group DN.
• Self-service updates of uiuser are now limited to uiuser_language for every role except middleware-server, each permission carries a post-update check equal to its filter so a row cannot be moved to another subject, and the auditor filter's mis-cased X-Hasura-uuid was normalised.
• middleware-server is left byte-identical; it legitimately writes identity during login.

SEC-04 — LDAP connection test carried credentials in a GET body (High)

• Most of this finding was already fixed in earlier commits, which I verified rather than re-implemented: auditor has no password columns, the endpoint is Admin-only, TestConnection binds with decryptPassword: false, and certificate plus hostname validation is enforced in ValidateLdapServerCertificate.
• Every secret-bearing GraphQL query is issued only by middleware server-side code; the UI reaches LDAP data solely through the WithoutSecrets variants.
• Remaining gap fixed here: the endpoint was [HttpGet] with [FromBody]. A GET carrying a body is handled inconsistently by proxies and HTTP clients and risks being cached or logged. It is now [HttpPost], with MiddlewareClient switched to Method.Post.
• Two tests pin the verb and the Admin-only restriction.

SEC-06 — Workflow state-change actions could be replayed (Medium)

• State is persisted before actions are requested, so the endpoint could only check that the object already stood in the requested state — which stays true after the transition, letting the same request re-send mails, re-raise external requests and re-create flows.
• The only existing de-duplication was client-side in ActionHandler and a direct POST /api/Workflow/Actions bypassed it entirely.
• New request.state_change_execution records which transition an object's actions last ran for. The middleware claims it with a single INSERT … ON CONFLICT … DO UPDATE … WHERE before executing anything: affected_rows = 1 means execute, 0 means already consumed.
• One row per object suffices — a replay repeats the last transition, while legitimately re-entering a state requires leaving it first. Postgres evaluates conflict and guard atomically.
• The claim is keyed on the server-resolved ticket, not the caller-supplied object id, so a caller cannot steer which guard is consumed.
• An already-consumed transition returns success with an informational message, because ExecuteInMiddleware throws on Success != true and an accidental double-submit should not surface as a failed promote. Replays are audit-logged.

SEC-09 — Hidden/removed flow objects and internal ANY were requestable (High)

• The flow catalog tables carried filter: {} for every workflow role, so a requester could read flow.nwobject, flow.svcobject and flow.timeobject in full — including entries hidden (show_in_request_module = false), retired (removed_date) or in a denied/removed state.
• request.reqelement exposed flow_nwobj_id, flow_nwgrp_id, flow_svcobj_id, flow_svcgrp_id and ip_proto_id as writable with check: {}, so any id could be written regardless of what the catalog offered. The only eligibility test in the whole path was client-side.
• ip_proto_id = -1 (GlobalConst.kAnyIpProtocolId) is the internal "any IP protocol" representation. The UI already excludes it from the protocol dropdown, but nothing stopped a direct write.
• Hasura select: the three flow tables now return only entries with show_in_request_module = true, removed_date IS NULL and state IN (requested, implemented) for approver, implementer, planner, requester and reviewer; flow.svcobject additionally requires ip_proto_id >= 0. middleware-server stays unfiltered — it runs FlowSync and the flow creation.
• Workflow insertion: reqelement insert (requester, approver, modeller) and update (requester, approver) refuse any flow id failing that predicate, and any negative ip_proto_id. The needed object relationships already existed; public.report_schedule_format already uses relationship traversal in an insert check, so the expression shape is proven in this metadata.
• Resolver lookup: FlowDbCreatorObjectResolution refuses an id naming a hidden or retired entry and records a FlowCreationRefusal instead of following it.
• FlowObjectEligibility holds the single predicate, reused by the request-module catalogs, FlowCatalogService and the flow creation. IsLive and IsRequestable are deliberately separate: the canonical ANY service passes IsLive because the flow creation attaches it itself for protocol-agnostic external requests, while IsRequestable rejects it for anything user-facing.
• WfDbAccess checks a task's flow ids before writing any element, so a stale editor gets localised message E8017 rather than a raw permission error.
• No other GraphQL call nests a flow table into another root and the reqelement fragments carry raw ids only, so historical tickets still render from each element's own stored name and address.

SEC-10 — Stored report values triggered headless-browser requests (High)

• Report and notification html is assembled from stored values — object, service, device, management and owner names, uids, comments, group members and section headers — and several reached the document without context-aware encoding.
• SetContentAsync loads subresources by default, so markup smuggled into such a value made the server's own browser issue outbound requests.
• Renderer lockdown (new PdfRenderSecurity): scripting off, cache off, request interception on with everything but about:/data: aborted, and --host-resolver-rules=MAP * ~NOTFOUND so no host name resolves. Applied to both Puppeteer paths — ReportBase and NotificationEmailLayoutHelper; the audit cited only the first. A test pins that the lockdown is applied before SetContentAsync.
• Each render already launches a dedicated browser that is closed afterwards, which is stronger than the isolated context the finding asks for, so no separate context was added.
• Encoding (new HtmlOutputEncoder): separate text, attribute and URL encoders. ConstructLink encodes all four parts for their own context and refuses any target that leaves the document. The URL check strips whitespace and control characters before reading the scheme, so java\nscript: cannot slip past a naive check.
• Two sites the audit did not list: the table of contents re-armed encoded markup, because CreateTOCContent pulls decoded InnerText out of the body and the ToC wrote it back raw; and <a name=...> was unquoted in ReportRules and ReportConnections with an object name interpolated in, where a space breaks out into new attributes. Both fixed.
• Headline, the section-header and statistics sites, and every imported field of the object, service and user tables (name, uid, comment, group members) are now encoded. MemberNamesWithoutHtml splits first, encodes each member, then joins with <br>, so the separator stays markup and the member name does not — which also fixes the six change-report call sites that share it.
• Exported documents carry a restrictive Content-Security-Policy meta, the only layer that also protects the exported .html when opened in a real browser, where scripting is not disabled.

Schema and upgrade impact

• roles/database/files/upgrade/9.5.3.sql creates request.state_change_execution; product_version → 9.5.3. The table is tracked in Hasura for middleware-server only.
• SEC-01, SEC-04, SEC-09 and SEC-10 need no migration. replace_metadata.json is re-applied on every install/upgrade run, and fworch-texts.sql is in database_idempotent_files and re-applied after a DELETE FROM txt, so the permission fixes and the new text keys land with any deployment.
• Existing objects start unclaimed, so the first transition after the upgrade is claimable for each in-flight ticket — it cannot block a legitimate promote, and the worst case matches pre-upgrade behaviour once.
• Two visible report-output changes: object anchor names are now quoted, and exported documents carry the extra policy element. Fourteen golden-output assertions in ExportTest were updated to match.

Verification

• dotnet build clean (0 warnings, 0 errors), dotnet format clean, full unit suite green: 5845 passed, 0 failed, 17 skipped (pre-existing live-backend integration tests).
• Every new test was run against the reverted behaviour and confirmed to fail, with positive controls that keep passing, so none of them pass vacuously.
• The environment had no live Postgres, Hasura or Chrome, so 9.5.3.sql, the new Hasura permissions and the renderer lockdown were exercised through unit tests and substitutes rather than against a running instance — worth one pass on a real deployment before release.

🤖 Generated with Claude Code

[tpurschke]

Review — v9.5.3 Q3 security audit findings

Reviewer: Claude Opus 5 (1M context), model id claude-opus-5[1m], extended thinking enabled.
Depth: deep (security-sensitive + schema-changing), performed directly against the fwo-review-pr correctness and security checklists.
Scope: upstream/develop...origin/fix/security/Q3-audit-findings-c01-05 — 46 files, +3501/-164. (Note: diffing against origin/develop shows 222 files; origin is the fork and its develop is stale. The real base is upstream/develop.)

Disclosure: I authored the SEC-09 and SEC-10 commits and wrote this PR's description. I have weighted the review towards my own changes accordingly, and F2 below is a defect in my own code found by testing it against a real browser for the first time.

Findings

#	Criticality	State	Subject
F1	high	🆕 new	SEC-06 replay guard is bypassable by varying the caller-supplied OldStateId
F2	high	🆕 new	--host-resolver-rules=MAP * ~NOTFOUND stops Chrome launching — all PDF export fails
F3	medium	🆕 new	Flow eligibility guard sits outside the try/catch, so an API failure escapes as an unhandled exception
F4	medium	🆕 new	reqelement update check blocks re-saving a task that references the canonical ANY service
F5	medium	🆕 new	The Hasura metadata changes are not applied or validated by any test or CI step
F6	low	🆕 new	No help content or whats_new_facts update for the visible request-module change

No prior review rounds exist on this PR, so numbering starts at F1 and no earlier rating could be lowered.
All detail is kept in this single comment: posting inline comments is outside the authority a review request grants.

---

F1 — SEC-06 replay guard is bypassable (high)

TryClaimStateChangeExecution keys the claim on (object_scope, object_id) and refuses only when the recorded transition equals the requested one. OldStateId is taken straight from the request body, and ValidatePersistedStateTransition only checks OldStateId != NewStateId and statefulObject.StateId == NewStateId — it never checks OldStateId against the object's actual previous state, because nothing records it.

So the guard is defeated by changing one integer:

1. Legitimate promote OldStateId=100, NewStateId=200 → row (Ticket, 42, 100, 200), object now in 200.
2. Replay with OldStateId=100, NewStateId=200 → refused. ✅ (this is the case the tests cover)
3. Replay with OldStateId=101, NewStateId=200 → 101 != 200 ✅, object stands in 200 ✅, guard sees (100,200) != (101,200) → affected_rows = 1 → actions execute again. Mail re-sent, external request re-raised, flow re-created. Repeatable with 102, 103, …

WorkflowMiddlewareUnitTest.WorkflowController_ExecuteActionsInMiddlewareContext_ExecutesNothingForAnAlreadyClaimedTransition stubs the API to return affected_rows = 0, so it asserts the controller's reaction to a refusal, not the guard's semantics. No test varies OldStateId.

Suggested fix: include only to_state_id in the guard (where: { to_state_id: { _neq: $toStateId } }), or persist the previous state and validate OldStateId against it. The PR description's claim that "a repeated request now returns without executing anything" does not hold as written.

F2 — Hardened browser argument stops Chrome launching (high)

PdfRenderSecurity.GetHardenedBrowserArgs() includes --host-resolver-rules=MAP * ~NOTFOUND. The value contains spaces, and passing it through LaunchOptions.Args makes Chrome fail at startup:

PuppeteerSharp.ProcessException: Failed to launch browser!
[ERROR:chrome/app/chrome_main.cc:236] Multiple targets are not supported in headless mode.

I verified this against a real Chrome 152 (downloaded via BrowserFetcher):

• full hardened arg list → launch fails, as above;
• same list with only that one argument removed → launches, SetContentAsync succeeds with scripting disabled, PdfDataAsync returns a 13 311-byte PDF;
• the same argument passed directly on a shell command line, correctly quoted as one argv entry → Chrome accepts it.

So the argument is valid for Chrome but cannot survive PuppeteerSharp's arg handling. Every PDF export and every PDF notification attachment fails at browser launch. It fails closed rather than open, so it is a functional break rather than an exposure — but it breaks both render paths completely.

Suggested fix: drop the argument. The same probe confirms request interception alone already does the job — with the CSP meta removed so only interception could act, both http://127.0.0.1:9/pixel.png and http://attacker.example/pixel.png were intercepted and aborted while the PDF still rendered. With the CSP present, neither request reached the interceptor at all, so the two layers are independently effective.

F3 — Eligibility guard escapes the error handling (medium)

In WfDbAccessReqTasks, FlowReferencesAreRequestable is awaited before the try in both AddReqTaskToDb and UpdateReqTaskInDb. Every other failure in these methods is caught and surfaced through DisplayMessageInUi. The guard issues two GraphQL queries, so a transient API error, a permission error or a timeout during a task save now propagates out of the method as an unhandled exception instead of a message. Move the call inside the existing try.

F4 — Update check blocks re-saving an element that references the canonical ANY service (medium)

The request.reqelement update check requires flow_svcobject.ip_proto_id >= 0. The canonical ANY service has ip_proto_id = -1 and is attached by the flow creation itself for protocol-agnostic requests created through the external REST API. A requester later editing such a ticket re-sends the stored flow_svcobj_id, and Hasura rejects the whole mutation — the save fails with a raw permission error, and the C# pre-check in WfDbAccessFlowElements does not cover it because it only validates ids it can read.

Reasoned from the metadata rather than observed at runtime; it needs a live Hasura to confirm. Worth either excluding the protocol condition from the update check (keeping it on insert, where the value is always user-authored) or extending the C# pre-check to give a clear message.

F5 — The Hasura metadata is never validated (medium)

This PR makes substantial replace_metadata.json changes, including relationship traversal inside request.reqelement insert/update check expressions. Nothing in the unit suite or CI applies the metadata, so a malformed or unsupported expression would first surface when roles/api/tasks/hasura-install.yml POSTs it during an install or upgrade, and fail the deployment. public.report_schedule_format already uses a relationship in an insert check, so the construct is plausible — but "plausible" is what F2 also looked like before it was executed. Applying the metadata once in the install test would cover all of it.

F6 — Missing help and What's-new updates (low)

The request module visibly changes: flow objects an administrator has hidden or retired disappear from the selection dropdowns, and a new error E8017 can appear on save. No file under roles/ui/files/FWO.UI/Pages/Help/ is touched and whats_new_facts is unchanged. These are security fixes rather than features, so this is a judgement call, but the behaviour change is user-visible.

---

Verified as correct

• SEC-01 is complete and precise. Every non-middleware role on public.uiuser is now columns: [uiuser_language] with filter and check both {"uuid": {"_eq": "x-hasura-uuid"}}; middleware-server is untouched and retains the columns it needs for login. The mis-cased X-Hasura-uuid in the old auditor filter is normalised. The post-update check is the part that actually stops a row being moved to another subject.
• SEC-04: [HttpGet] → [HttpPost] with the matching Method.Post in MiddlewareClient, Admin-only authorization retained.
• SEC-09 Hasura side: the eligibility filter is applied to exactly the five workflow roles on the three flow tables, middleware-server deliberately unfiltered, and flow.svcobject additionally requires ip_proto_id >= 0.
• SEC-10 encoding: HtmlOutputEncoder strips whitespace and control characters before the scheme check, so java\nscript: is caught; ConstructLink, the ToC, Headline and the object/service/user tables are encoded. The renderer design is sound where it runs — only F2 stops it running.
• Coding guidelines on the 2 097 added code lines: no inline array arguments in production code, no method over 100 lines, no file near the 1 000-line limit (largest new file 142 lines), numeric literals confined to test fixtures and one named constant, new public members carry XML docs.
• Localization: both new text keys (flow_creation_ineligible_flow_object, E8017) have German and English rows. product_version is 9.5.3 and 9.5.3.sql exists, matching the new request.state_change_execution table.

Recommendations

Fix before merge

• F2 — drop the --host-resolver-rules argument; without it nothing renders at all.
• F1 — tighten the claim guard so OldStateId cannot be varied to re-arm it, and add a test that does so.

Fix in this PR if convenient

• F3 — one-line move of the guard inside the try.
• F4 — decide between relaxing the update check and improving the pre-check message.

Worth doing separately

• F5 — apply replace_metadata.json once in the install test.
• F6 — help and What's-new text.

Review process

• Depth: deep, run directly against this skill's checklists (no separate review subagent capability was used for the judgement passes).
• Delegated to a reduced model tier: two mechanical scans (coding-guideline metrics; localization/help/whats-new presence checks). The coding-guideline scan came back against the wrong tree — the local working copy is on another branch — so I discarded it and redid those checks against the PR branch myself. All security judgement, finding verification, criticality and numbering were done on the primary model.
• Runtime verification: F2 was found and confirmed by building a standalone probe and running the exact launch/harden/render sequence against a real downloaded Chrome 152. F1 was confirmed by reading the guard and its validation chain. F4 is reasoned from the metadata and is not runtime-confirmed.
• Not run: I did not check out the PR branch or re-run the unit suite against its HEAD, because the working copy holds unrelated uncommitted work. Test results quoted in the PR description are not re-verified here. The Sonar quality gate on the PR reports passed.
• Usage budget: this environment exposes no usage indicator to me, so I could not measure the 25% ceiling. I enforced the proxy limits instead: 2 sub-agent dispatches (limit 6), one depth escalation, and reads confined to the diff plus its direct callers, contracts and tests.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
90.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud