fix: security & privacy (rate limiter fix, csrf session cookie, logout on server, drafts stale token passw, login timing, password over 72b, JWT_SECRET complexity, SMTP TLS, CSV export neutralised, fonts bundled, upload metadata, hourly cleanup job, privacy.md report)

This commit is contained in:
Florian Sylvain committed 2026-10-22 00:49:19 +02:00
1 parent e2812ee0b9
commit ea98c5ec05
118 files changed
+3828 -130

No files matched your search

+2
View File
@@ -358,6 +358,8 @@ Feature: The back-office keeps an append-only activity log that owners can read
| target | +1+1 | '+1+1 |
| target | -2 | '-2 |
| actor | @SUM(A1) | '@SUM(A1) |
| target | \t=cmd | '\t=cmd |
| target | \r=cmd | '\r=cmd |
| target | Spring release | Spring release |
| target | a=b | a=b |
+6 -4
View File
@@ -53,12 +53,14 @@ Feature: Password and code hashing go through a domain Hasher interface
Then the result is the error user.ErrInvalidCredentials
And errors.Is(err, user.ErrInvalidCredentials) is true
Scenario: Authenticating an unknown username never calls the hasher
Scenario: Authenticating an unknown username spends the same hashing work as a wrong password
Given a user repository with no user "bob"
When AuthenticateUserUseCase.Authenticate is called with username "bob" and password
When AuthenticateUserUseCase.Authenticate is called twice with username "bob" and password
"hunter2example"
Then the result is the error user.ErrUserNotFound
And the hasher's Compare is never called
Then each result is the error user.ErrUserNotFound
And the hasher's Compare is called once per attempt, against a decoy hash of a random value
that no password can match, so response time does not reveal whether a username exists
And the decoy is hashed once and reused for every later unknown username
Scenario Outline: Login over HTTP still can't distinguish a bad password from an unknown username
Given a user repository containing a user "alice" with stored password "hashed:hunter2example"
+4 -3
View File
@@ -29,9 +29,10 @@ Package: `test` (black-box, alongside the existing `article_test.go` / `user_tes
assert the returned user matches and `err` is `nil`.
- `Authenticate("alice", "wrongpassword12")` with `fakeHasher.Compare` returning an error
→ assert `errors.Is(err, user.ErrInvalidCredentials)` is `true`.
- `Authenticate("bob", "hunter2example")` with no "bob" in the repo → assert
`errors.Is(err, user.ErrUserNotFound)` is `true` and `fakeHasher`'s `Compare` was never
invoked (record a `compareCalled` flag).
- `Authenticate("bob", "hunter2example")` twice with no "bob" in the repo → assert
`errors.Is(err, user.ErrUserNotFound)` is `true` each time, that `fakeHasher`'s `Compare`
ran once per attempt (against the decoy hash), and that `Hash` ran exactly once overall
(the decoy is computed lazily and reused).
## HTTP checks (real BcryptHasher, in-memory fake repo, mirroring api/router.go)
8. Call `validator.InitValidator()`.
@@ -16,7 +16,8 @@ Feature: Login and password-reset requests are rate-limited per client IP
(github.com/go-chi/chi/v5/middleware) runs first and rewrites r.RemoteAddr from
X-Forwarded-For/X-Real-IP when present (matching the nginx reverse proxy setup in
docs/deployment.md that already sets X-Forwarded-For), falling back to the raw RemoteAddr
otherwise; RateLimiter keys its counters on r.RemoteAddr
otherwise; RateLimiter keys its counters on the host portion of r.RemoteAddr (the source
port is ignored, so opening a new connection never yields a fresh counter)
And RateLimiter(maxAttempts int, window time.Duration) func(http.Handler) http.Handler keeps
an in-memory, mutex-guarded fixed-window counter per (route, client IP) pair: the first
request from a client opens a window starting now; each subsequent request from the same
+3 -2
View File
@@ -26,8 +26,9 @@ Feature: Pending registration screen
you'll log in there first. Can't find it? Check your spam folder. If the link has expired,
cancel your registration and start again.") is rendered through ResultPanel's default slot
as a second paragraph below the description, per components.md's "Slot default for extras"
And "Log out" calls POST /v1/auth/logout, which per loginHandler.Logout always responds 200 and
clears the jwt cookie; on its response, PendingPage calls session.clear() and navigates to
And "Log out" calls POST /v1/auth/logout, which per loginHandler.Logout clears the jwt cookie
and responds 200 (500 only when the session cannot be revoked server-side; the cookie is
cleared either way); on its response, PendingPage calls session.clear() and navigates to
/login, disabling both actions while the call is in flight
And "Cancel registration" opens ConfirmDialog (built by this slice; components.md documents it
but no file exists yet) via useOverlay().create(ConfirmDialog).open(), with title "Cancel
+190
View File
@@ -0,0 +1,190 @@
Feature: Sessions, uploads and stored personal data are hardened against forgery, replay and over-retention
As the operator of a RenewCMS instance
I want cross-site writes refused, logged-out sessions dead, weak secrets rejected, mail encrypted,
photo metadata removed and stale personal data deleted on a schedule
So that the instance meets the security-of-processing and storage-limitation duties of the GDPR
(art. 5(1)(c), 5(1)(e), 25 and 32) without the operator having to configure anything
Background:
Given api.InitRoutes wraps every route in middleware.CrossOriginProtection(settings.PublicURL),
built on the standard library's http.CrossOriginProtection, after CORS and before any router
And the jwt cookie is set and cleared with HttpOnly, SameSite=Lax, Path "/" and, in
production, Secure
Rule: Cross-site writes are refused
Scenario Outline: An unsafe cross-origin browser request is refused before any handler runs
Given a request <method> /v1/user/invite carrying the jwt cookie and the headers <headers>
Then the response is 403 with the body "Cross-origin requests are not allowed."
And the handler never runs
Examples:
| method | headers |
| POST | Sec-Fetch-Site: cross-site, Origin: https://evil.example |
| POST | Sec-Fetch-Site: same-site, Origin: https://blog.example.com |
| DELETE | Sec-Fetch-Site: cross-site, Origin: https://evil.example |
| POST | Origin: https://evil.example (an older browser, no Sec-Fetch-Site) |
Scenario Outline: Requests that cannot be forged are let through
Given a request <request>
Then the handler runs
Examples:
| request |
| POST with Sec-Fetch-Site: same-origin |
| POST with Sec-Fetch-Site: none |
| POST with neither Sec-Fetch-Site nor Origin (a non-browser client) |
| GET, HEAD or OPTIONS from any origin |
| POST from another origin carrying an Authorization header (an API key or bearer jwt) |
| POST whose Origin is the origin of PUBLIC_URL while a proxy rewrote Host |
Scenario: A PUBLIC_URL that cannot be parsed still leaves the protection on
Given PUBLIC_URL is not a URL
When a cross-site POST arrives
Then it is refused with 403
Rule: Logging out kills the session server-side
Scenario: Every session token carries a unique id and an expiry
When authtoken.Encode issues a token without "jti" or "exp"
Then it adds a random "jti" (crypto/rand.Text) and "exp" 24 hours after "iat"
And authtoken.Decode refuses a token without "exp" and a token whose "jti" is not a string
with ErrInvalid
Scenario: The token presented at logout is refused afterwards, other sessions are not
Given alice logged in twice, getting tokens A and B
When she sends POST /v1/auth/logout with token A
Then the response is 200 and clears the cookie
And a session.Revocation for A's jti, expiring at A's exp, is stored in revoked_sessions
And GET /v1/auth/me with token A answers 401 and with token B answers 200
Scenario: Logout without a valid session stores nothing
When POST /v1/auth/logout arrives without a cookie, or with an expired or revoked one
Then the response is 200, the cookie is cleared and nothing is stored
Scenario: A revocation that cannot be stored is reported
Given the revoked_sessions store refuses writes
When alice logs out
Then the response is 500 and the cookie is still cleared
Scenario: A token whose revocation cannot be checked is refused
Given the revoked_sessions store cannot be read
Then any request authenticated by a jwt is treated as unauthenticated
Rule: Only a current session sees drafts
Scenario: A verified user's stale token reads like an anonymous visitor
Given alice is verified and her token_version is 1
When GET /v1/article/ arrives with a token carrying token_version 0
Then only online articles are listed
Rule: Passwords fit bcrypt
Scenario Outline: A password over 72 bytes is refused with 400
When <request> carries a password of 73 bytes, or of 37 two-byte characters
Then the response is 400 with the body "The password must be 72 bytes or fewer."
And nothing is created or changed
Examples:
| request |
| POST /v1/auth/register |
| POST /v1/auth/password-reset/confirm (valid code) |
| POST /v1/auth/invite/accept (valid code) |
Scenario: A wrong reset code still wins over a long password
When POST /v1/auth/password-reset/confirm carries a wrong code and a 73-byte password
Then the response is 403 as for any wrong code
Rule: The server refuses to start with a guessable signing secret
Scenario Outline: A weak JWT_SECRET stops startup
Given JWT_SECRET is <value>
Then config.NewSettingsFromEnv fails with config.ErrWeakJWTSecret
And the message says to generate one with `openssl rand -hex 32`
Examples:
| value |
| empty |
| shorter than 32 bytes |
| "change-me-to-a-long-random-string" |
| "replace-with-output-of-openssl-rand-hex-32" |
Rule: Mail is never sent in clear text over the network by default
Scenario: A non-loopback SMTP server without STARTTLS is refused
Given SMTP_PORT is not 465, SMTP_HOST is not localhost, 127.0.0.1 or ::1, and the server does
not offer STARTTLS
When a mail is sent
Then delivery fails before MAIL FROM with an error naming SMTP_ALLOW_INSECURE
Scenario: The operator can opt out for a trusted relay
Given SMTP_ALLOW_INSECURE is "true"
When a mail is sent to such a server without credentials
Then it is delivered in clear text
And credentials are still never sent in clear text
Rule: Uploaded photos lose their metadata
Scenario Outline: Metadata is removed before the bytes are stored, hashed or measured for size
When a <format> image is uploaded
Then the stored bytes no longer contain <removed>
And they still contain <kept>
And the image's size and sha256 describe the stored bytes, and its pixels are unchanged
Examples:
| format | removed | kept |
| JPEG | EXIF, XMP, IPTC (APP13), comments, MPF and other APPn segments, trailing images | JFIF, ICC profile, Adobe |
| PNG | eXIf, tEXt, zTXt, iTXt, tIME and data after IEND | every other chunk |
| WebP | EXIF and XMP chunks (their VP8X flags cleared) and data after the RIFF size | every other chunk |
Scenario: A rotated photo keeps only its orientation
Given an uploaded JPEG, PNG or WebP whose EXIF orientation is 2 to 8
Then the stored file carries a minimal EXIF block holding only that orientation (after SOI or
JFIF for JPEG, after IHDR for PNG, at the end with the EXIF flag set for WebP)
Scenario: An image whose container cannot be walked is refused
When a JPEG, PNG or WebP with a truncated header, a chunk longer than the file or no IEND
(PNG) is uploaded
Then the upload fails with image.ErrImageUnreadable and nothing is stored
And a JPEG cut off inside its scan data is kept as it is
Scenario: SVG is stored as uploaded
When an SVG is uploaded
Then its bytes are stored unchanged
Rule: Stale personal data is deleted on a schedule
Scenario: The retention job runs hourly and logs only when it removed something
Given scheduler.NewSystemRetentionRunner runs useCases.EnforceRetentionUseCase a minute after
startup and then every hour
When a run removes something
Then the log reads "scheduler: personal data retention completed: revoked_sessions=<n>
unverified_users=<n> invites=<n>"
And a run that removes nothing logs nothing, and a failed run logs "... failed: <error>"
Scenario Outline: What is removed
Given <record>
Then the retention job <outcome>
Examples:
| record | outcome |
| a revoked session whose exp has passed | deletes it |
| an unverified account whose verification code expired more than 7 days ago | deletes it |
| an unverified account without a code expiry created more than 7 days ago | deletes it |
| an invite that expired more than 30 days ago | deletes it |
| an invite accepted more than 30 days ago | deletes it |
| a verified account, a pending invite or an account still inside its 7 days | keeps it |
Scenario: A failure stops the run and keeps the counts of what was done
Given deleting a user fails
Then the run returns the error with the sessions already purged counted and touches no invite
Rule: The back office loads nothing from third parties
Scenario: Fonts are bundled
Given the SPA's fonts (IBM Plex Sans, IBM Plex Mono, Source Serif 4) live in
web/src/assets/fonts with their OFL licences
Then the Content-Security-Policy allows "font-src 'self'" and "style-src 'self'
'unsafe-inline'" with no Google Fonts host
And loading the back office sends no request to fonts.googleapis.com or fonts.gstatic.com
+84
View File
@@ -0,0 +1,84 @@
# QA: security and privacy hardening
Everything below runs in `task test` (package `test`, `-tags nofrontend`) unless marked manual.
## Cross-origin protection and cookie flags
1. `test/cross_origin_protection_http_test.go`: wrap a counting handler in
`middleware.CrossOriginProtection("https://cms.example.com")`.
- Refused with 403 and `apperr.CrossOriginErrorMessage`, handler not reached: a cross-site POST, a
same-site POST from `https://blog.example.com`, a cross-site DELETE, and a POST with only
`Origin: https://evil.example`.
- Allowed: same-origin POST, `Sec-Fetch-Site: none`, no headers, cross-site GET and OPTIONS, a
cross-site POST with `Authorization: Bearer rcms_key`, and a POST whose `Origin` is the public
URL while `Host` is `127.0.0.1:8080`.
- With `PUBLIC_URL` `::not a url`, a cross-site POST is still 403.
2. `test/session_revocation_http_test.go` drives `api.InitRoutes`: the login cookie is HttpOnly,
Secure and `SameSite=Lax`, and a cross-site `POST /v1/auth/logout` is 403 and revokes nothing.
## Session revocation
3. `test/authtoken_test.go` (`TestAuthTokenRevocation`, `TestAuthTokenDecodeRejections`): distinct
`jti` per token; explicit `jti` kept; a revoked token gives `ErrRevoked` and is stored until its
`exp`; other tokens stay valid; store read failure gives `ErrInvalid`; store write failure is
returned by `Revoke`; no `jti` is a no-op; no `exp` refuses; tokens without `exp` or with a
non-string `jti` are refused by `Decode`.
4. `TestSessionRevocationUseCase`: an already-expired session is not stored; an empty or 65-byte id
is `session.ErrInvalidSessionID`; 64 bytes is accepted.
5. `TestRevokedSessionRepository` on a migrated SQLite file: revoking twice is harmless,
`DeleteExpired(now)` removes rows whose `expires_at <= now` and reports the count.
6. `TestLogoutRevokesTheSession` through `api.InitRoutes`: two logins, logout with the first, `/me`
with it is 401 and with the second is 200; logout without a cookie is 200 and stores nothing;
a failing write gives 500 with the cookie cleared; a failing read makes `/me` 401.
7. `test/verified_account_required_http_test.go`: a verified user whose `token_version` moved on
only sees online articles.
## Passwords
8. `test/password_length_limit_test.go`: 72 bytes accepted, 73 bytes and 37 × "é" refused by
`user.ValidatePassword`; register, reset (valid code) and invite accept answer 400 with
`apperr.PasswordTooLongErrorMessage` and change nothing; a wrong reset code still answers 403.
9. `test/hasher_test.go`: an unknown username compares against a decoy hash once per attempt and
hashes the decoy once.
## Configuration and mail
10. `test/runtime_settings_from_env_test.go`: empty, short, 31-byte and both example
`JWT_SECRET` values fail with `config.ErrWeakJWTSecret` and mention `openssl rand -hex 32`;
32 bytes passes. `SMTP_ALLOW_INSECURE` reads `""`/`false` as false, `true`/`TRUE` as true, and
`maybe` fails naming the variable.
11. `test/smtp_mailer_test.go` against the fake server on `127.0.0.2`: no STARTTLS and no opt-out
fails before `MAIL FROM` naming `SMTP_ALLOW_INSECURE`; with the opt-out the message is
delivered in clear text; with the opt-out and credentials, nothing is sent.
## Upload metadata
12. `test/image_metadata_stripping_test.go` builds JPEG, PNG and WebP files carrying a GPS-like
marker in EXIF, XMP, IPTC, comments, text chunks, MPF and trailing data. Assert the marker is
gone, ICC is kept, pixels decode identically, the orientation-only EXIF appears only when the
orientation is not 1 and sits in the documented place, WebP flags and RIFF size are correct,
a plain WebP and an SVG come back unchanged, and each malformed container is
`ErrImageUnreadable`.
13. `TestUploadStoresTheStrippedImage`: the stored bytes carry no marker and `Size`/`SHA256` match
them. `TestUploadRefusesAnImageWhoseContainerIsBroken`: nothing is stored.
## Retention
14. `test/personal_data_retention_test.go`: the user and invite rules at their boundaries; one run
over a seeded world removes exactly one expired revocation, one abandoned account and two
invites; a second run removes nothing; a session store failure touches nothing else; a user
or invite deletion failure returns the partial counts; the runner logs counts only when
something was removed and logs failures.
## Fonts
15. `test/security_headers_test.go`: the CSP contains `font-src 'self';` and no Google host.
16. Manual: `task build:web`, open the back office with the network panel; no request leaves for
`fonts.googleapis.com` or `fonts.gstatic.com`, and `task visual:check -- log-in` passes.
## Manual verification
- Behind the real reverse proxy, log in, copy the `jwt` cookie, log out, then replay the cookie
with `curl`: `/v1/auth/me` answers 401.
- With an SMTP relay that lacks STARTTLS on a non-loopback host, sending fails until
`SMTP_ALLOW_INSECURE=true` is set.
@@ -10,7 +10,7 @@ Feature: The rate limiter keys on the real connection address, trusting a forwar
Background:
Given POST /v1/auth/login and POST /v1/auth/password-reset are each wrapped, inside
NewAuthRouter, by a middleware that runs before RateLimiter (api/middleware/rateLimit.go),
which keys its counters on r.RemoteAddr
which keys its counters on the host portion of r.RemoteAddr, never on its source port
And this slice replaces chimiddleware.RealIP on those two route groups with
TrustedProxyRealIP(trustedProxies []string) func(http.Handler) http.Handler
(api/middleware/trustedProxyRealIP.go)
@@ -96,3 +96,23 @@ Feature: The rate limiter keys on the real connection address, trusting a forwar
X-Real-IP header
Then the first response is not rejected by rate limiting and the second is 429 — both are
counted against "10.0.0.1" itself
Scenario: New connections from the same address share one counter
Given TRUSTED_PROXIES is "" and AUTH_RATE_LIMIT_MAX_ATTEMPTS is 1
When a request arrives with connection address "203.0.113.7:40000"
And a second request arrives with connection address "203.0.113.7:40001"
Then the first response is not rejected by rate limiting and the second is 429, because the
counter is keyed on "203.0.113.7" and the source port is ignored
Scenario: A forwarded client keeps one counter across the proxy's connections
Given TRUSTED_PROXIES is "10.0.0.1" and AUTH_RATE_LIMIT_MAX_ATTEMPTS is 1
When a request arrives with connection address "10.0.0.1:51000" and header
X-Real-IP: "198.51.100.9"
And a second request arrives with connection address "10.0.0.1:51001" and the same header
Then the first response is not rejected by rate limiting and the second is 429
Scenario: Expired counters do not accumulate
Given requests from many distinct addresses
When a full window has passed since a counter started
Then the next request evicts every counter whose window has ended, so memory stays bounded by
the addresses seen within one window
@@ -56,6 +56,14 @@ setup (`mockUserRepo`, `stubHasher`, `mockMailRepo`, the full `useCases` set, `a
no forwarding header at all. Assert not `429`. Send a second, identical request. Assert `429` —
a trusted proxy with nothing to forward falls back to its own address as the key.
14. `trustedProxies = nil`, limit 5. Send 100 requests from `203.0.113.7` on ports `40000`–`40099`.
Assert exactly 95 are `429` — the source port never yields a fresh counter
(`test/rate_limit_connection_key_http_test.go`).
15. Same for IPv6: `[2001:db8::1]:50000` then `[2001:db8::1]:50001` with limit 1 → `200` then `429`.
16. `trustedProxies = ["10.0.0.1"]`, limit 1. `10.0.0.1:51000` and `10.0.0.1:51001`, both with
`X-Real-IP: 198.51.100.9` → `200` then `429`.
17. Limit 1, window 20 ms: `200`, `429`, wait 30 ms, `200` — an expired counter starts over.
## Manual verification (not exercised by the Go test)
- Real behaviour behind the documented nginx and Caddy reverse proxies (`docs/deployment.md`) —