# YourPHR — Architecture & Security Review > __Correction (2026-06-07):__ this review says "Angular 14" throughout, but the frontend was already on __Angular 20__ at review time (upgraded 14→20 via foundation epic #12, merged 2026-06-04). The "Angular 14" framing led to filing #114 as a framework-upgrade need — actually a transitive lforms dep (fixed, #114 closed). Likewise H2's "JWT in localStorage" was since fixed (HttpOnly cookie, #103). Treat the stack details below as the point-in-time snapshot; the findings/backlog still stand. > > Independent evaluation of the `jwilleke/yourphr` codebase (community continuation of > Fasten OnPrem). Reviewed: backend (Go/Gin/GORM), frontend (Angular 14), deployment, > and the SMART-on-FHIR relay. __No code was changed__ — this is an assessment plus a > prioritized improvement backlog. > > Date: 2026-06-06 · Scope: `master`/`main` working tree. > > See also: [Standards Conformance](./Standards-Conformance.md) — how these findings map to > SMART App Launch / OAuth 2.0 / PKCE / OWASP, with verified-conformant items and tracked gaps. --- ## 1. Executive summary YourPHR is a self-hosted Personal Health Record viewer: a Go backend (Gin + GORM, encrypted SQLite) serving a compiled Angular 14 SPA, importing FHIR R4 bundles. The architecture is clean and conventional, and the fork shows __genuine, recent security hardening__ beyond upstream — a dedicated SSRF guard package, a store-and-poll OAuth relay that keeps tokens out of the browser, per-user data scoping at the repository layer, bcrypt (cost 14), JWT algorithm pinning, reserved-username blocking, and a "standby mode" that refuses to open the DB without an encryption key. The dominant residual risk is __a single shared HS256 JWT signing key that ships with a known public default and is never force-rotated on first run__. Because all per-user data isolation and the admin role gate ultimately trust that signature, a deployment left on the default key is fully compromisable (forge a token for any user/role). Everything else is secondary to closing that gap. __Overall posture: solid foundation, one critical config-hardening gap, several medium hardening opportunities.__ Appropriate for a self-hosted/family threat model, especially behind the documented forward-auth (Authentik) deployment; not yet ready to be exposed directly to the public internet without the fixes below. --- ## 2. Architecture evaluation ### 2.1 Component map | Layer | Tech | Notes | |---|---|---| | Frontend | Angular 14 SPA, served as static assets | JWT in `localStorage`; talks to `/api` | | Web/API | Gin router (`backend/pkg/web/server.go`) | Route groups: `/api` (public), `/api/secure` (JWT), `/api/unsafe` (dev only) | | Auth | HS256 JWT (`backend/pkg/auth/jwt_utils.go`) + bcrypt | Session token (1h) + long-lived "access" tokens (DB-backed) | | Data | `DatabaseRepository` interface → GORM | SQLite (encrypted build) is the only working backend; Postgres is stubbed/broken | | FHIR models | ~70 generated `fhir_*.go` structs | FHIRPath extraction via goja JS engine into indexed columns | | Relay | Separate tiny Go service (`backend/cmd/relay`) | Store-and-poll OAuth `code` bouncer; never sees tokens | | Sources | `fasten-sources` replaced by a local __stub__ | Live provider sync is non-functional in this fork; manual upload is the supported path | ### 2.2 Strengths - __Clean separation of concerns.__ The `DatabaseRepository` interface (`backend/pkg/database/interface.go`) is a single, testable data-access contract; handlers stay thin and pull dependencies from Gin context via typed middleware (`RepositoryMiddleware`, `ConfigMiddleware`, `EventBusMiddleware`). - __Per-user scoping is centralized.__ Almost every repository method derives the owner from `GetCurrentUser(ctx)` (`gorm_common.go:94`) and filters by `UserID`, rather than trusting an ID from the request. This is the right place for the check and makes horizontal isolation the default. - __Defense in depth on roles.__ The admin gate (`IsAdmin`, `handler/auth.go:22`) re-reads the role from the DB rather than trusting the `role` claim baked into the JWT — so a tampered claim cannot escalate without a valid signature. - __Code generation discipline.__ FHIR model structs and the frontend TS types are generated and committed, with the regeneration path documented. This keeps ~70 resource types consistent and reduces hand-written surface area. - __Reproducible builds.__ Multi-stage Dockerfile, vendored deps, `distroless/static` runtime image, Nix flake for the dev toolchain. ### 2.3 Architectural risks / weaknesses - __SQLite ceiling.__ The roadmap already flags this: a lifetime, multi-family PHR on a single-writer SQLite file will eventually hit write-contention and size limits. PR #95 ("isolate per-table query session") suggests concurrency friction is already showing. Postgres is the documented escape hatch but is currently broken. - __Multi-user model is half-built.__ The README describes admin/viewer roles and "grant access to another user's records," but the data layer currently scopes strictly to the current user (no implemented cross-user grant/ACL beyond `verifyAssociationPermission`). Shipping the *documented* sharing model will require a real authorization layer (see §3) — this is the highest-leverage architectural decision ahead, and it is much cheaper to design before the sharing feature than to retrofit. - __`Reinitialize()` server lifecycle__ (`server.go:40`) tears down and rebuilds the HTTP server on encryption-key setup. Functional, but the standby→active transition is a stateful dance worth integration-test coverage. - __Generated-model coupling to upstream identifiers.__ Intentional (documented in AGENTS.md), but it means the fork carries upstream's `fasten-*` naming and the private `fasten-sources` stub indefinitely. Fine as a decision; just a long-term maintenance tax. --- ## 3. Security evaluation Ordered by severity. Severity reflects impact on __PHI confidentiality__, which is the asset that matters most here. ### 🔴 Critical __C1 — Default JWT signing key + no forced rotation.__ ([#102](https://github.com/jwilleke/yourphr/issues/102)) The committed `config.yaml:39` ships `jwt.issuer.key: "thisismysupersecure…"`, a known public default. `JwtValidateFastenToken` (`auth/jwt_utils.go:42`) verifies tokens with this key, and *all* per-user scoping and the admin gate ultimately trust it. Unlike the __database__ encryption key — whose absence forces "standby mode" (`server.go:446`) — the JWT key has a working default, so the server starts silently insecure. An operator who deploys without overriding it exposes every record: an attacker can forge a valid token for any `username`/role. *Recommendation:* generate a random key on first run and persist it as a system setting (the in-code TODO already proposes this), or refuse to start if the key equals the known default. At minimum, log a prominent startup error (not just a doc note). ### 🟠 High __H2 — JWT stored in `localStorage`.__ ([#103](https://github.com/jwilleke/yourphr/issues/103)) `auth.service.ts` keeps the session JWT in `localStorage` (`:187`, `:224`). Any XSS in the Angular app yields full PHI exfiltration, and "access" tokens can be minted to persist access. For a PHI app this is a meaningful amplifier of any frontend injection bug. *Recommendation:* prefer an `HttpOnly`, `Secure`, `SameSite` cookie for the session token, or at least add a strict Content-Security-Policy (see H4) to shrink the XSS surface. __H3 — No rate limiting / lockout on auth, plus username enumeration.__ ([#104](https://github.com/jwilleke/yourphr/issues/104)) `AuthSignin` (`handler/auth.go:78`) has no throttling or lockout — bcrypt cost 14 slows but does not stop online guessing. It also returns __different responses__ for unknown user (`500`, "could not find user: X") vs. wrong password (`401`, "username or password does not match: X"), which enumerates valid usernames. Signup is likewise unauthenticated and unthrottled. *Recommendation:* add per-IP/username rate limiting + temporary lockout; return a single generic `401` ("invalid username or password") for both failure modes; avoid echoing the attempted username. __H4 — No security response headers.__ ([#105](https://github.com/jwilleke/yourphr/issues/105)) No `Content-Security-Policy`, `Strict-Transport-Security`, `X-Frame-Options`/`frame-ancestors`, `X-Content-Type-Options`, or `Referrer-Policy` are set anywhere in the backend. A PHR SPA should ship a strict CSP (mitigates H2), HSTS (HTTPS is on by default), and clickjacking protection. *Recommendation:* add a security-headers middleware; start CSP in report-only and tighten. ### 🟡 Medium __M5 — Open registration.__ `POST /api/auth/signup` is public; the first account silently becomes `admin` (`handler/auth.go:51`), and anyone who can reach the instance can self-register a regular account thereafter. Mitigated in the documented deployment by Authentik forward-auth, but the app itself has no toggle to disable open signup or require admin approval. *Recommendation:* add a `disable_open_registration` setting; consider admin-invite-only after first-run. __M6 — CORS proxy is unauthenticated and forwards all headers.__ `CORSProxy` (`handler/cors_proxy.go`) sits in the __public__ `/api` group and copies the inbound request headers verbatim to the upstream (`req.Header = c.Request.Header`, `:64`). It is constrained to endpoints whose definition has `CORSRelayRequired` and whose URL prefix-matches the known `TokenEndpoint` (good), and it forces `https://`. But the in-code `TODO: throw an error if the remote.Host is not allowed` (`:74`) is unresolved, and it then stamps `Access-Control-Allow-Origin: *` with `Allow-Credentials: true` on responses. The prefix-match is the only real guard. With the `fasten-sources` stub the catalog is small, limiting exposure today, but this is a latent SSRF/open-relay shape. *Recommendation:* require auth (move to `/api/secure` or gate behind a token), strip hop-by-hop and `Authorization` headers unless intended, and resolve the host-allowlist TODO. __M7 — Access tokens default to effectively no expiry.__ `buildAccessTokenModel` (`handler/access_token.go:113`) maps "expiration = 0" to the year __2099__. Long-lived bearer tokens for PHI are high-value; if one leaks it is valid for ~75 years until manually revoked. *Recommendation:* cap maximum lifetime, default to a finite expiry, and surface last-used/rotation in the UI. __M8 — DB-write failure on access-token creation is swallowed.__ `CreateAccessToken` (`handler/access_token.go:52`) returns the freshly minted JWT to the client even if persisting it fails ("Still return the token even if storage fails"). Since `RequireAuth` validates access tokens against the DB (`require_auth.go:46`), the token is issued but unusable — confusing, and it means a partial failure hands out a credential the server won't honor. *Recommendation:* fail closed — do not return a token that wasn't stored. ### 🔵 Low / informational - __L9 — Verbose error passthrough.__ Several handlers return `err.Error()` straight to the client (e.g. signup/signin `500`s). Low risk on a self-hosted box, but leaks internal detail; prefer generic messages + server-side logging. - __L10 — `/api/unsafe` dev endpoints.__ Guarded by `web.allow_unsafe_endpoints` (default off) with loud startup warnings (`server.go:227`). Correctly gated; just ensure it can never be enabled in the shipped image. - __L11 — `web.listen.host: 0.0.0.0`.__ Binds all interfaces by default. Expected for a containerized app, but worth a deployment note that the instance should sit behind the reverse proxy / forward-auth, not be exposed directly. - __L12 — `GetUserByUsername` can return a non-nil zero user on error__ (TODO noted at `gorm_common.go`), a potential nil/zero-value panic or logic hazard; tighten the error contract. - __L13 — Relay shared-secret model.__ The relay design is sound (never sees tokens; secret gates `/pending`). Ensure `YOURPHR_RELAY_SECRET` is high-entropy and the relay enforces the short TTL it documents; consider constant-time secret comparison. ### Things done well (credit where due) - SSRF guard (`backend/pkg/ssrf/ssrf.go`) blocks loopback/RFC1918/link-local/metadata and is wired into the SMART discovery path (`handler/source.go:30`); it even documents its own DNS-rebinding limitation. - Access-token operations verify ownership before delete (`gorm_repository_access_token.go:84`). - JWT validation pins HS256 and rejects other algorithms (`auth/jwt_utils.go:47`), closing the `alg=none`/algorithm-confusion class. - bcrypt cost 14 with empty-password rejection (`models/user.go`). - Reserved-username blocking (`gorm_common.go`) and strong `.gitignore`/AGENTS.md rules against committing PHI, DBs, keys, and certs. - DB encryption on by default with a standby-mode guard when the key is absent. --- ## 4. Prioritized improvement backlog | # | Improvement | Severity | Effort | Area | |---|---|---|---|---| | [C1](https://github.com/jwilleke/yourphr/issues/102) | Generate/persist a random JWT key on first run, or refuse to start on the default key | 🔴 Critical | S | Backend/config | | [H2](https://github.com/jwilleke/yourphr/issues/103) | Move session JWT out of `localStorage` (HttpOnly cookie) | 🟠 High | M | Frontend/auth | | [H3](https://github.com/jwilleke/yourphr/issues/104) | Auth rate limiting + lockout; generic `401`; stop username echo | 🟠 High | M | Backend | | [H4](https://github.com/jwilleke/yourphr/issues/105) | Security-headers middleware (CSP, HSTS, X-Frame-Options, nosniff) | 🟠 High | S | Backend | | M5 | `disable_open_registration` / invite-only mode | 🟡 Medium | S | Backend | | M6 | Authenticate + host-allowlist the CORS proxy; drop forwarded auth headers | 🟡 Medium | M | Backend | | M7 | Cap access-token lifetime; finite default | 🟡 Medium | S | Backend | | M8 | Fail closed when access-token persistence fails | 🟡 Medium | S | Backend | | — | Design the real authorization layer __before__ building cross-user record sharing | Arch | L | Backend | | — | Restore/finish Postgres backend for scale | Arch | L | Backend/DB | | L9–L13 | Error hygiene, relay secret hardening, `GetUserByUsername` contract | 🔵 Low | S | Various | __Suggested order:__ C1 → H4 → H3 → H2 → M5/M7/M8 (quick wins) → M6 → multi-user authz design → Postgres. ### Process / supply-chain suggestions (no app-code change) - Add `gosec` and `govulncheck` to CI (`ci.yaml`) and `npm audit`/`yarn audit` for the frontend; the project already runs `go vet` + tests and Dependabot. - Add `gitleaks` (or similar) as a pre-commit / CI gate to enforce the "never commit PHI or secrets" rule mechanically rather than by discipline alone. - Document a key-rotation runbook (JWT key, DB encryption key, relay secret). --- ## 5. Caveats This review is static (read-only) and time-boxed; it did not run the app, exercise the endpoints, or audit every one of the ~70 generated FHIR handlers. Severity ratings assume the self-hosted/family threat model the project targets, with the note that direct public exposure raises H3/H4/M5/M6 in practice. Findings cite `file:line` at review time and may drift as the code evolves — re-verify before acting.