SharepointToolbox-Web/SECURITY-TODO.md

Security findings

Review date: 2026-06-11. Items 1–4 and 6 fixed the same day; #5 reviewed and accepted by design.

---

1. [HIGH] ✅ FIXED — Open redirect in /connect/initiate leaked SharePoint session tokens

Was: returnUrl from the query string was stored verbatim in the OAuth flow state and used as the final redirect target after the code exchange — with token_key (a 2-minute, redeemable handle to the connected client's refresh token) appended. …?returnUrl=https://evil.com leaked that handle off-domain, enabling connection hijack.

Fix: returnUrl is now constrained to a site-relative path via the new string.ToLocalReturnUrl() helper before it is stored (Infrastructure/OAuth/OAuthEndpoints.cs). The helper rejects absolute and protocol-relative (//host, /\host) URLs.

---

2. [MEDIUM] ✅ FIXED — CSV formula injection in audit-log exports

Was: Both audit CSV exporters quoted per RFC 4180 but didn't neutralize a leading = + - @, so user-controlled fields (display/client/site names, details) could execute as formulas in Excel.

Fix: Both paths now use CsvSanitizer.Escape (which neutralizes formula prefixes): the /audit/export endpoint (Program.cs) and AuditService.ExportCsvAsync (Services/Audit/AuditService.cs). The local CsvEscape/Esc duplicates were removed.

---

3. [LOW-MEDIUM] ✅ FIXED — Spoofable X-Forwarded-Proto could yield a non-Secure auth cookie

Was: Forwarded headers are trusted from any source (proxy IP unknown inside the container network), and the prod auth cookie used SecurePolicy = SameAsRequest — so a spoofed X-Forwarded-Proto: http or a direct plaintext hit could emit a non-Secure cookie.

Fix: Prod auth cookie now uses CookieSecurePolicy.Always (Program.cs), so the Secure flag is set regardless of the (untrusted) forwarded scheme. The forwarded-headers trust config is unchanged (still required behind the proxy); the residual XFF concern is limited — client IP is not used for auth and is not recorded in the audit log.

---

4. [LOW] ✅ FIXED — Open redirect in /account/login/entra and /account/login/dev

Was: returnUrl was used directly as the post-auth redirect on the entra and dev sign-in paths; only local-login validated it (and even that check allowed protocol-relative //evil.com).

Fix: All three sign-in redirects now route returnUrl through ToLocalReturnUrl() (Program.cs), which also closes the protocol-relative gap in the old local-login check.

---

5. [LOW] ⚪ ACCEPTED (by design) — Authenticated users can download any export / report

Where: /export/download/{fileName} and /reports/download/{id} (Program.cs).

Decision: Not changed. The app has no per-user profile authorization anywhere — any tech may select and use any shared profile (see the "standard techs use profiles without sign-in" design), and GeneratedReport carries only a ProfileId, no owner. Reports are listed and shared per client/profile, not per user, so per-user download scoping would contradict the product model rather than fix a boundary. Both endpoints already require authentication and strip path traversal (Path.GetFileName). Revisit only if a per-tech profile ACL is introduced.

---

6. [LOW] ✅ FIXED — Admin pages gated only by a render-time role check

Was: /admin/users and /admin/audit used @attribute [Authorize] (any authenticated user) plus an in-markup if (Role != Admin) return;.

Fix: Added an Admin authorization policy (RequireClaim("app_role", "Admin")) in Program.cs and both pages now use [Authorize(Policy = "Admin")] (UserManagement.razor, AuditLogs.razor). A claim-value policy (not [Authorize(Roles=…)]) was used deliberately: the local/dev sign-in identities don't set a ClaimTypes.Role claim, so a Roles check would have silently denied local admins. The in-component checks were kept as defense-in-depth.