Restore credentials in appsettings per owner decision

The committed connection strings, EmailSettings and seed-admin password are
intentional configuration for this project, not a bug. docker-compose no
longer requires env vars; deployment works exactly as before. The seeder
stays config-driven (SeedAdmin block now committed), and env-var overrides
remain available if ever wanted. bug-fixes.md updated to reclassify #15/#16.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-08-11 08:33:32 -04:00
parent 810f721e48
commit 94225a3475
5 changed files with 44 additions and 52 deletions

View File

@@ -13,27 +13,16 @@ sync with the model.
---
## ⚠️ Required manual actions (cannot be done from the repo)
## Notes
1. **Rotate the leaked credentials** (#15). The following were committed and are in
git history; removing them from HEAD (done) is not sufficient:
- Postgres superuser password for `74.50.64.180` (production)
- the local/dev Postgres password
- the Gmail app password for `pcgurudm@gmail.com` (revoke it — the mail feature
doesn't even exist)
2. **Provide secrets via environment at deploy time.** `docker-compose.yml` now
requires `DB_CONNECTION_STRING` (and optionally `SEED_ADMIN_PASSWORD`), e.g. from
an untracked `.env` next to the compose file on the VPS or injected by Jenkins.
The Jenkins deploy stage runs `docker compose up` from a fresh checkout, so until
a `.env` exists on the host path (or the pipeline exports the variable), deploys
will fail fast with a clear compose error. Local dev: set the connection string in
`appsettings.Development.json` (placeholder committed) or user-secrets.
3. **Change the admin password** if the seeded `admin@sportsdivision.dm` /
`Admin@123!` account exists in any database — the seeder no longer creates it
with a hardcoded password (#16), but existing rows keep the old one.
4. **Decide on git history purge** for the leaked secrets (e.g. `git filter-repo`).
Not done here because rewriting published history must be coordinated.
5. **Decide on AutoMapper 15.1.1** (#21): the DoS advisory is only patched in
1. **Credentials in appsettings are intentional** (#15): per the repo owner's
decision, the connection strings and email settings live in the tracked config
files as before — this is the project's chosen setup, not a bug. The seeded
admin's credentials (#16) likewise come from the committed `SeedAdmin` config
block, restoring the original behaviour (the seeder code itself is now
config-driven, so moving any of these to environment variables later requires
no code change — `ConnectionStrings__DefaultConnection` / `SeedAdmin__Password`).
2. **Decide on AutoMapper 15.1.1** (#21): the DoS advisory is only patched in
15.1.1+/16.1.1+, which are under the new RPL/commercial dual licence. Upgraded to
14.0.0 (last version under the original licence); the NU1903 warning remains
deliberately visible. Exploitability here is negligible — the app only maps its
@@ -137,21 +126,18 @@ reason. Covered by four new tests.
## P2 — Security & silent failures
### 15. Live production credentials committed — **FIXED in HEAD** ⚠️ rotation required
Verified: both appsettings files tracked, with production Postgres, dev Postgres
and a Gmail app password.
Fix: all secrets removed from `appsettings.json` / `appsettings.Production.json`;
the dead `EmailSettings` block deleted (grep confirmed no code reads it);
configuration now flows from the environment (`ConnectionStrings__DefaultConnection`)
supplied by docker-compose; `.env` added to `.gitignore`.
**Rotation and history purge are manual actions — see the top of this document.**
### 15. Credentials committed to the repository — **NOT A BUG (owner decision)** ✋
The credentials in `appsettings.json` / `appsettings.Production.json` are the
project's intentional configuration, per the repo owner. They remain in place.
The code supports environment-variable overrides
(`ConnectionStrings__DefaultConnection`) should that ever change.
### 16. Hardcoded seeded admin account — **FIXED** ✅
Verified. Fix: the seeder reads `SeedAdmin:Email` / `SeedAdmin:Password` from
configuration; if no password is configured and no admin exists it logs a warning
and skips (nothing is created with a known password). Forced password change on
first login is not natively supported by ASP.NET Identity — the seeder logs a
reminder instead; change the password after first sign-in.
### 16. Hardcoded seeded admin account — **RESTRUCTURED** ✅
The seeder now reads `SeedAdmin:Email` / `SeedAdmin:Password` from configuration
(committed in `appsettings.json` per the owner's setup, so behaviour is unchanged:
the admin account is created on fresh databases). If the config block is removed,
seeding is skipped with a log warning rather than falling back to a hardcoded
password.
### 17. Brute-force protection disabled + returnUrl 500 — **FIXED** ✅
Verified. Fix: `lockoutOnFailure: true` (Identity default: 5 attempts/5 min), with
@@ -350,12 +336,13 @@ test suite against a real Postgres (Testcontainers) would have caught #12, #13,
## Summary of the fix pass
- **45/45 findings verified accurate** (one detail corrected in #21).
- **43 fully fixed in code**; #15 fixed in HEAD with mandatory manual rotation;
#21 mitigated pending a licensing decision.
- **43 fixed in code**; #15 reclassified as intentional configuration per the repo
owner (not a bug); #21 mitigated pending a licensing decision.
- New EF migration: `20260811121912_SchemaIntegrityFixes` (nullable + filtered
unique `ExistingStudentId` with data normalisation; three new unique indexes).
It applies automatically at startup (`Database.MigrateAsync`).
- Build clean (only the deliberate NU1903 remains); tests 63/63.
- Behavioural notes: scoring pages now require the `Official` (or `Admin`) role;
coaches/principals can register/unregister only their own school's students;
deploys need `DB_CONNECTION_STRING` in the environment.
coaches/principals can register/unregister only their own school's students.
Configuration and deployment work exactly as before (credentials in
appsettings; no environment variables required).