7. Security

This chapter combines three phases of work: the Phase 0 discovery (initial findings), Phase 0.1 (independent verification of the two most consequential findings), and Phase 0.2 (the critical-fix pass that closed the highest-severity confirmed defects). Nothing below is invented — findings are carried forward from the source discovery record, and fixes are the actual Phase 0.2 code changes, version 1.0.48.

7.1 Current State at a Glance

#	Finding	Phase 0/0.1 severity	Phase 0.2 status
1	ZATCA Basic-Auth credentials built but never attached to outgoing requests (all 4 client services, both forks)	High → confirmed via static analysis in Phase 0.1	Fixed
2	Private key stored as plaintext in AppSetting.Value, no encryption anywhere in the write path	Critical	Not fixed — storage model unchanged (see § 7.6)
3	Private key logged via Serilog {@object} destructuring, both forks	Critical	Fixed
4	CSID BinarySecurityToken/Secret logged via Serilog {@object}, both forks	Critical	Fixed
5	Raw OTP logged in plaintext (core fork only)	High	Fixed
6	IntegrationSettings.OTP returned in plaintext by an unpermissioned GET endpoint	High	Fixed (redacted)
7	File-download endpoint has no filename sanitization (path traversal)	Medium-High	Fixed
8	Literal plaintext private key/certificate password present in a local, gitignored appsettings.production.json	Medium	Not fixed — operational/rotation decision, not a code change
9	ZATCA response/request objects logged via {@object} — would have leaked the Basic-Auth header once #1 was fixed	Informational (forward-looking, Phase 0.1)	Fixed in the same change as #1, per the Phase 0.1 recommendation

7.2 ZATCA Authentication — Confirmed and Fixed

Original finding (Phase 0): each of the four ZATCA client services (ClientCsidService, ClientClearanceService, ClientComplianceService, ClientReportingService, in both the core and Tenancy forks) had a private SetAuthentication() helper that built a Basic-Auth-configured RestClient and returned it — but the actual outbound request was sent via a separate, unauthenticated _client field set in the constructor, which was never reassigned.

Phase 0.1 verification: this was upgraded from "needs live verification" to conclusively proven by static analysis — the _client field was readonly, SetAuthentication()'s return value was discarded at every call site (in one case, in the Tenancy fork, without even being awaited), and RestSharp 112's Authenticator is strictly per-instance with no ambient/global state. Every request sent through this code carried no Authorization header at all.

Phase 0.2 fix: the _client field is no longer readonly and is now reassigned to the authenticated RestClient returned by SetAuthentication() before each request, in all four services, in both forks. Focused regression tests were added that assert the field is actually reassigned (using an unreachable loopback address so the test fails fast without depending on a live network endpoint).

7.3 Secrets Handling

Value	Stored	Encrypted?	Logged (pre-fix)?	Logged (post Phase 0.2)?
Private Key (FatooraSecretSettings.PrivateKey)	Plaintext in AppSetting.Value	No	Yes — full object via {@object}	No
Certificate / EncryptionPassword	Plaintext in AppSetting.Value	No	Not confirmed directly logged	Unchanged
CSID BinarySecurityToken/Secret	Plaintext in AppSetting.Value	No	Yes — full object via {@object}, plus the raw ZATCA response object	No
OTP	Plaintext in AppSetting.Value	No	Yes, core fork only, raw value	No — masked via the SDK's own pre-existing (previously unused) MaskOtp() helper

What Phase 0.2 changed, precisely:

• X509CertificateService.GenerateCSR() (both forks) no longer logs _fatooraSecret via {@object}.
• ClientCsidService.Compliance() (both forks) now logs only RequestID, not the full CSID secret object; the core fork's raw-OTP log now uses the SDK's own pre-existing MaskOtp()/GetBodyPreview() helpers, which existed in the file already but were never wired in before this fix.
• ZatcaClientExtensions (both forks) no longer logs the full RestResponse/RestRequest.Parameters or the raw ZATCA response body — only status codes. This closes the forward-looking gap the Phase 0.1 report flagged: once authentication is actually attached (§ 7.2), the unmodified response-logging code would have started leaking the Basic-Auth header and, for CSID calls, ZATCA's own response body (which contains binarySecurityToken/secret directly).

What did not change: the underlying storage model. Private keys, certificates, and CSID tokens are still persisted as plaintext strings in the shared AppSetting table, via the same ToString() path every other tenant setting uses. Shumoul.Framework.Infrastructure/Secrets/FutureVaultProvider.cs exists as an unimplemented placeholder (throws NotImplementedException for Azure Key Vault / AWS Secrets Manager / HashiCorp Vault) — no secret-manager integration exists for this or any other subsystem today.

7.4 OTP-Returning Endpoint

Original finding: GET api/EInvoicing/Settings/Integration returned IntegrationSettings.OTP in plaintext to any authenticated user — no permission check beyond class-level [Authorize].

Phase 0.2 fix: the controller now builds a redacted response DTO with OTP set to null, preserving ExpiresOn/Invalid for status/expiry display. The underlying setting object fetched from the (possibly cached) IEInvoicingAppSettingService is never mutated — a new DTO is constructed so no cached instance is corrupted.

7.5 File-Download Path Traversal

Original finding: EInvoicingController.GetFile (both .Api and .TenancyApi) interpolated the caller-supplied fileName query parameter directly into a file path with no sanitization:

var filePath = $"./Files/Invoices/Signed/{fileName}.xml";

Gated only by [Authorize] — no permission check, no tenant/ownership check.

Phase 0.2 fix: fileName is now passed through Path.GetFileName() and compared against the original input — any value containing a path separator is rejected outright rather than silently truncated — and the resolved full path is verified to remain inside the intended ./Files/Invoices/Signed/ directory before the file is read. Focused tests cover both ../-style and backslash-style traversal attempts, plus a benign non-existent filename to confirm the fix doesn't produce false rejections.

7.6 What Remains Open

• Plaintext storage at rest (private keys, CSID tokens, certificate password) — unchanged. This would require encrypting values in the AppSetting write path, or routing E-Invoicing secrets through a real secret manager — neither exists today.
• appsettings.production.json's local plaintext secrets — confirmed not leaked to source control (gitignored, untracked), but still unencrypted on whatever machine or deployment holds that file. Rotation is an operational decision, not a code change.
• Cache key has no tenant identifier (§ 6.3) — flagged as a risk pattern, not confirmed as an active cross-tenant leak; needs explicit verification.
• Zero test coverage for signing, certificate, QR, and onboarding code beyond the two Phase 0.2 additions — the security-critical cryptographic core of the SDK still has no regression protection for its own logic (only for the two fixes above).
• CSR OID divergence between forks and an EC curve identifier inconsistency — documented in Phase 0, classified Medium/Low, not addressed in Phase 0.2 (out of that phase's Critical/High-only scope). See Chapter 11 — Technical Debt.

None of these were in scope for the Phase 0.2 critical-fix pass, which was explicitly limited to the Critical/High findings listed in § 7.1. See Chapter 12 — Known Limitations.