findings.md

SecureVault reliability and security review

Reviewed: 2026-09-12
Scope: the current SecureVault.sln / SecureVault.csproj source tree and a local Release build.
Severity: Critical means immediate or broad secret/account compromise; High means a practical compromise or serious data-loss/outage path; Medium means a conditional or limited-impact weakness.

Ranked findings

Rank	ID	Severity	Finding
1	SV-01	Critical	A known administrator credential is shipped in source and build output
2	SV-02	Critical (original)	Implemented: first-login password hashing and encrypted vault/history; temporary bootstrap plaintext retained by request
3	SV-03	Implemented	HTTPS-only developer launch, HTTP-to-HTTPS redirection, production HSTS, and Secure session cookies
4	SV-04	Implemented	Shared read access no longer grants secret modification or ACL administration
5	SV-05	High (partially remediated)	Login throttling and failed-attempt audit implemented; MFA and lockout remain out of scope
6	SV-06	High (remaining)	Individual entry/history files now use atomic replacement; multi-file transactions remain open
7	SV-07	High (remaining)	Shared-directory writes and IDs now coordinated across processes; external durable storage remains open
8	SV-08	Medium	Unbounded version and audit growth enables authenticated denial of service
9	SV-09	Medium	Session identity and username normalization can produce stale or ambiguous authorization
10	SV-10	Medium	Every authenticated user can enumerate every entry title and owner
11	SV-11	Medium	The audit trail is incomplete, rewriteable, and not tamper-evident

Detailed findings

1. SV-01 — A known administrator credential is shipped in source and build output

Severity: Critical — authentication bypass with administrator impact

App_Data/users.json is tracked by Git and contains a non-empty plaintext password for an administrator. The README publishes the corresponding default credential (README.md:54-59 and README.md:311-322). SecureVault.csproj:8-11 copies every App_Data/*.json file to the build/publish output. A fresh Release build was successfully authenticated with the tracked credential during this review.

Adding App_Data/*.json to .gitignore does not remove files already tracked, and the credential remains recoverable from Git history even after a future deletion.

Impact: Anyone who can read the repository, a source archive, or an unchanged deployment artifact can sign in as an administrator and read or edit every vault entry.

Recommendation:

1. Treat the shipped credential as compromised and rotate it everywhere it may have been deployed.
2. Remove live data from Git and build artifacts, purge it from distributable history where appropriate, and use a non-secret example file.
3. Provision the first administrator out of band at deployment and fail closed when no identity provider or provisioned account is available. Never fall back to a documented default password.

2. SV-02 — User passwords and vault contents are stored in plaintext

Implementation status (2026-09-12): Complete with the requested temporary-password exception. Deployment requires external key provisioning and restart.

• Preserve existing/manual Password values until successful login, then atomically remove the field and write PasswordHash in the same users.json account object before issuing a session. Use ASP.NET Core Identity V3 salted PBKDF2-HMAC-SHA512 (210,000 iterations), verify existing hashes, and upgrade weaker hashes on successful login.
• A present hash is authoritative. Wrong passwords and malformed hashes never fall back to plaintext or rewrite credentials. Duplicate usernames ignoring case fail authentication. Other accounts and additional user metadata survive migration.
• Encrypt each entry and its complete history in an individual numbered file (entries/0001.json, etc.) using AES-256-GCM bound to the entry ID. Validate/decrypt and migrate plaintext or encrypted legacy arrays before serving requests; all subsequent reads/writes use encrypted envelopes.
• Require an explicitly provisioned key ring outside the application directory. Support active key rotation with retained old keys; reject missing keys and damaged ciphertext without overwriting the vault. Document separate key/data backups and recovery in README.md.
• Add automated service, HTTP, and separate-process regression coverage: 55 tests passed, including password login/restart, failed entry replacement, interrupted legacy migration, concurrent edits and ID allocation, writer termination, tampering/file swapping, rotation/backup restore, and read-only shared-entry authorization.

Accepted residual exposure: Accounts that have not successfully logged in still have plaintext provisioning passwords, as requested. Migration does not scrub Git history, old backups/artifacts, or recoverable disk blocks. Keys must be kept separately with restricted permissions. Audit metadata remains plaintext. Cross-process coordination now covers cooperating processes sharing the same local data directory; multi-file transactions remain open. Tests used synthetic isolated data; deployment data has not been migrated by this implementation task.

The text below records the original review evidence, before this remediation.

Severity: Critical — complete disclosure after filesystem, backup, or artifact access

Models/User.cs:5-7 models a password as a directly serializable string, and Services/JsonDataStore.cs:37-43 validates it with plaintext equality. Vault payloads are held in Entry.Details and duplicated into every EntryHistoryRecord.Details (Models/Entry.cs:6-10, Models/EntryHistoryRecord.cs:7-9, and Services/JsonDataStore.cs:164-178). Save serializes the complete object graph directly to readable JSON (Services/JsonDataStore.cs:476-480).

The history feature multiplies exposure: rotating a secret in the current entry does not remove any older value from the history array.

Impact: A read of App_Data, a backup, source control, a publish artifact, or a recovered partial disk image reveals reusable login passwords plus all current and historical secrets. Plaintext also makes offline password guessing unnecessary.

Recommendation: Hash login passwords with a supported adaptive password hasher (prefer ASP.NET Core Identity or an external OIDC provider); passwords must not be reversibly encrypted. Encrypt vault payloads and history with authenticated encryption and keys held outside the data directory, preferably in a platform KMS/secret store. Design and test key rotation, backup, and recovery. OWASP's Password Storage and Secrets Management guidance describe these separate requirements.

3. SV-03 — The application accepts credentials and session cookies over HTTP

Implementation status (2026-09-12): Complete. HTTPS must remain the public deployment boundary.

• The developer launch profile exposes only https://localhost:6708; it no longer offers an HTTP application endpoint.
• Every session cookie is HttpOnly, essential, and uses CookieSecurePolicy.Always, so it is never returned over HTTP.
• HTTP requests are redirected to HTTPS before routing or session handling. HSTS is enabled outside Development so browsers retain the HTTPS-only policy in deployed environments.
• The Release process smoke test starts explicit HTTP and HTTPS listeners with a test certificate, confirms /Account/Login redirects to the actual HTTPS port, confirms the production HSTS header, and fails unless the successful-login session cookie contains both HttpOnly and Secure.

TLS-terminating reverse proxies must be explicitly configured to forward HTTPS correctly and must be trusted by the deployment; this application does not treat arbitrary forwarded headers as proof of TLS. The 30-minute session timeout remains an idle timeout, not an absolute session lifetime.

4. SV-04 — Read access also grants secret modification and ACL administration

Implementation status (2026-09-12): Complete. Shared access is read-only; only the entry owner and administrators can manage an entry.

• CanUserReadEntry remains the read policy. New CanUserModifyEntry and CanUserAdministerEntryAccess policies require the entry owner or an administrator (Services/JsonDataStore.cs:135-143).
• Both Edit actions require both management policies and return HTTP 403 when the current user has read-only access (Controllers/EntriesController.cs:95-130). The details page does not render an Edit action for a reader (Views/Entries/Details.cshtml:39).
• UpdateEntry reloads the current account and enforces the management policies under the storage lock before changing either the secret or Users (Services/JsonDataStore.cs:212-226). This protects callers that bypass MVC routing.
• Regression tests verify that a shared reader cannot modify a secret or set Users to *, including authenticated HTTPS GET and POST attempts (tests/SecureVault.Tests/StorageSecurityTests.cs:222-239 and 449-470).

Residual policy limits: The implementation intentionally has two management capabilities with the same owner/administrator membership; it does not provide delegated write-only or ACL-only roles. Authorized owners and administrators can still overwrite a concurrent authorized edit because optimistic concurrency is not implemented.

The text below records the original review evidence, before this remediation.

Severity: High — integrity compromise and privilege expansion by any reader

Before remediation, both Edit actions authorized with CanUserReadEntry. The edit form exposed Users, and the POST passed the reader-supplied value to UpdateEntry, which overwrote both the secret and its ACL. The README documented this behavior.

Original attack scenario: A user granted read-only access to one credential could edit the entry, change the secret, and set Users to * or add another account. The original owner received no approval prompt. This broke the reasonable security distinction between viewing a shared credential and administering it.

Original recommendation: Introduce distinct read, write, and share/ACL permissions. Limit ACL changes to the owner (and, if required, administrators); decide explicitly whether readers may edit content. Enforce this in the service/domain layer as well as controllers, and add authorization tests for direct POSTs. Consider optimistic concurrency so one authorized editor cannot silently overwrite another.

5. SV-05 — Login has no throttling, lockout, MFA, or failed-attempt audit

Implementation status (2026-09-12): Partially complete as scoped.

• The login POST has a source-address sliding-window limit of five attempts per minute. Excess requests return HTTP 429; this is automatic throttling, not an account lockout.
• Every login request that reaches credential validation and fails records a LoginFailure audit event with the attempted username and source address. Submitted passwords are never logged.
• MFA was deliberately not implemented.
• Account lockout was deliberately not implemented.

The text below records the original review evidence, before the scoped remediation.

Severity: High — unrestricted online password guessing

Controllers/AccountController.cs:27-46 accepts every valid anti-forgery-protected login attempt and immediately retries plaintext validation. Program.cs configures no rate limiter. JsonDataStore records only successful logins (Services/JsonDataStore.cs:319-329), so password spraying is neither slowed nor recorded. Anti-forgery protection prevents cross-site form submission; it does not limit a client that first fetches its own token.

Impact: An attacker can make unlimited guesses against known usernames. The shipped default credential makes the current state worse, but this remains exploitable after that credential is rotated.

Recommendation: Prefer a mature identity provider with MFA, lockout, credential rotation, and security-stamp/session revocation. At minimum, apply a tested rate-limit policy to login using both account and source signals, add bounded exponential delay/lockout, log failed attempts without recording submitted passwords, and alert on spraying. ASP.NET Core provides rate-limiting middleware.

6. SV-06 — In-place JSON writes can corrupt the vault; multi-file operations are not transactional

Remediation update (2026-09-12):

• Store each entry and its full history in one encrypted numbered file; a save rewrites only that entry file.
• Flush temporary output and atomically replace the destination, committing current content and history together. Failed replacement leaves the previous file intact.
• EntryFileStore.Save writes the serialized entry and its History through AtomicFile.WriteAllText; the helper flushes a same-directory temporary file before replacing the numbered entry file.
• Stage and verify legacy migration before directory publication and source removal; resume after interruption without silently choosing conflicting data.
• Verify failed replacements, interrupted migration, and termination during repeated saves using isolated data.
• Provide recoverable transactions spanning entry, request, and audit files. A later audit/request write failure can still occur after an entry has committed.

Validation (2026-09-12): dotnet test SecureVault.sln -c Release --nologo passed 56 of 56 tests.

The following paragraphs retain the original review evidence.

Severity: High — data loss and persistent outage after ordinary failures

Every mutation ends in File.WriteAllText(path, json) (Services/JsonDataStore.cs:476-480). This replaces the target in place; process termination, disk exhaustion, or an I/O error after truncation can leave empty or partial JSON. The next JsonSerializer.Deserialize (Services/JsonDataStore.cs:468-474) throws, and there is no recovery path, validation, backup, or exception handling.

Operations spanning files can also commit only halfway. For example, approval writes entries.json and later writes requests.json (Services/JsonDataStore.cs:260-292); entry creation/update writes the entry and later rewrites the audit file (Services/JsonDataStore.cs:109-148 and 156-189). A failure can grant access while leaving a request pending, or return an error after a successful secret mutation with no matching audit event.

Recommendation: Move state to a transactional database with constraints, durable commits, backup, and recovery. If JSON must remain temporarily, write and flush a validated file in the same directory, atomically replace the destination while retaining a known-good backup, and serialize multi-file changes through a recoverable journal. Add fault-injection tests for disk-full, interrupted-write, and malformed-file cases.

7. SV-07 — The storage model loses updates across processes and couples live data to deployments

Remediation update (2026-09-12):

• Use an OS-held exclusive .store.lock across each storage read/modify/write operation, including initialization and ID allocation. Reload an entry under the lock before appending a version; the last committed save wins and prior versions remain in history. Process termination releases the lock.
• Record access-list approvals in history under the same lock so concurrent edits cannot erase the approved version's record.
• Exclude the complete App_Data tree from new build/publish content and ignore its individual entry, staging, and lock files in Git. Existing installed data must be preserved during deployment.
• Verify concurrent migration, 40 same-entry updates, unique concurrent creation, and writer termination using separate .NET processes on Windows.
• Move durable storage outside the deployment/content root and provide database/distributed coordination for replicas or unvalidated shared filesystems. Existing Git history and older artifacts are not purged; SV-01 credential rotation remains outstanding.

The following paragraphs retain the original review evidence.

Severity: High — lost secrets, duplicate identifiers, and deployment-time data loss

The only concurrency control is an in-memory lock (Services/JsonDataStore.cs:8), so it protects one JsonDataStore instance in one process. IDs are allocated as Max(Id) + 1 from a snapshot (Services/JsonDataStore.cs:111-137 and 208-226). Two application processes sharing a directory can read the same snapshot, assign the same ID, and overwrite one another's changes. Processes using separate local disks instead diverge into different vaults.

Live mutable data is located under IHostEnvironment.ContentRootPath (Services/JsonDataStore.cs:19-27) while the project copies seed/live JSON into output (SecureVault.csproj:8-11). A clean deployment can replace or omit the only copy of production state; an in-place deployment can overwrite it based on file timestamps.

Recommendation: Put durable state in an external database or secret-management service, use database-generated IDs and transaction/concurrency controls, and keep mutable volumes entirely outside application artifacts. Document and test backup/restore, rolling deployment, and multi-instance behavior.

8. SV-08 — Unbounded version and audit growth enables authenticated denial of service

Severity: Medium — disk, memory, and request-queue exhaustion

The input models have no length constraints (Models/EntryCreateViewModel.cs:3-8 and Models/EntryEditViewModel.cs:3-9). Each edit appends a full copy of the title, secret, and ACL to history (Services/JsonDataStore.cs:164-178). Each read of an authorized entry appends an audit record (Controllers/EntriesController.cs:45-46), and audit logging reads and rewrites the entire growing log (Services/JsonDataStore.cs:347-351). All of this synchronous file I/O occurs behind the same global process lock used by login and vault operations.

Attack/failure scenario: A low-privilege account repeatedly submits large secrets or edits, or repeatedly opens an entry. Disk use grows without retention, and every subsequent request allocates/deserializes and rewrites progressively larger arrays. One account can therefore degrade all users or fill the disk.

Recommendation: Enforce small, explicit request and field limits; set per-user/entry quotas; cap or archive history; make audit writes append-oriented and independently bounded; use asynchronous durable storage; and apply endpoint/concurrency limits. Test behavior at quota boundaries and under concurrent load.

9. SV-09 — Session identity and username normalization can produce stale or ambiguous authorization

Severity: Medium — offboarded users retain capabilities; case-colliding accounts may inherit the wrong record

Authentication is represented only by a non-empty session string (Controllers/AccountController.cs:43, Controllers/EntriesController.cs:153-154, and Controllers/RequestsController.cs:49-50). Controllers do not centrally revalidate that the account still exists. A removed user with an active session can still create entries (Controllers/EntriesController.cs:66-83), submit requests (Controllers/EntriesController.cs:136-150), and approve/decline requests for entries whose CreatedBy string matches the stale session (Services/JsonDataStore.cs:260-313). Activity can keep the idle session alive.

Identity comparison is also inconsistent: login matches username case-sensitively (Services/JsonDataStore.cs:37-43), while user lookup, ownership, and ACL checks are case-insensitive (Services/JsonDataStore.cs:46-52, 68-81). The manually edited user store has no uniqueness validation. If two records differ only by case, a user can authenticate against one record but GetUser can return the other, potentially conferring its administrator flag.

Recommendation: Use ASP.NET Core authentication/authorization with an immutable unique user ID, normalized unique usernames, and a server-side account/security-stamp check. Revoke sessions on account disablement, deletion, password reset, or privilege change. Validate the user file at startup and refuse ambiguous identities while migrating away from it.

10. SV-10 — Every authenticated user can enumerate every entry title and owner

Severity: Medium — confidential metadata disclosure

EntriesController.Index loads all entries before calculating CanRead (Controllers/EntriesController.cs:16-31). The list renders every title and, for restricted entries, the owner (Views/Entries/Index.cshtml:49-78). The restricted details page repeats title, numeric ID, and owner (Views/Entries/Details.cshtml:32-37 and 67-71).

Titles such as customer names, production systems, incident names, or credential types can themselves be sensitive. Sequential IDs also make complete enumeration trivial.

Recommendation: Decide explicitly whether vault existence metadata is public to all accounts. If not, return only authorized entries and implement access requests through a separate owner-mediated discovery mechanism. If discovery is intentional, require non-sensitive titles, minimize returned metadata, and document the boundary.

11. SV-11 — The audit trail is incomplete, rewriteable, and not tamper-evident

Severity: Medium — weak detection and incident reconstruction

Although README.md:115-149 describes access.json as append-only, LogAccess loads the complete array and replaces the file (Services/JsonDataStore.cs:347-351). There is no integrity chain, signature, access separation, rotation, or external sink. Failed logins, access requests, approvals, declines, and authorization failures are not logged. The same host/filesystem compromise that exposes secrets can alter the only audit evidence.

Recommendation: Send structured security events to an append-only or access-separated external sink. Include success and failure outcomes, request/approval changes, stable actor and target IDs, correlation IDs, and safe source context; never log passwords or secret contents. Define retention, monitoring, clock, and tamper-detection requirements and test that audit failure does not silently create half-committed business operations.

Positive controls observed

• Every current state-changing MVC action uses [ValidateAntiForgeryToken].
• Razor rendering HTML-encodes the reviewed user-controlled values by default.
• The current read check defaults private entries to owner/admin only and treats only the exact trimmed value * as public (Services/JsonDataStore.cs:64-81).
• Direct .json URL paths are rejected before static-file handling (Program.cs:18-30), and App_Data is outside wwwroot.
• Current .NET analyzers reported no warning/error diagnostics. A live dotnet list package --vulnerable --include-transitive check reported no vulnerable NuGet packages from the configured feeds on 2026-09-12.

These controls do not mitigate the critical plaintext/default-credential findings or the HTTP, authorization, and persistence risks above.

Validation performed and limits

• dotnet build SecureVault.sln -c Release --nologo — passed with 0 warnings and 0 errors.
• Roslyn Release workspace load — 1 project, net10.0, no workspace-load diagnostics.
• Compiler/analyzer scan including suppressed diagnostics — four informational performance suggestions; no warning/error security diagnostics.
• Release runtime HTTP/login check — confirmed HTTP acceptance and a non-Secure session cookie; the test used only the copied build-output data and did not disclose the credential.
• NuGet vulnerability audit — no vulnerable packages reported by the configured feeds at review time.
• dotnet test tests\\SecureVault.Tests\\SecureVault.Tests.csproj --no-restore — 55 passed, including the shared-reader authorization regression and HTTPS GET/POST checks. This is automated regression coverage, not a penetration test or deployed-environment validation.