mirror of
https://github.com/bitechdev/ResolveSpec.git
synced 2026-10-01 12:31:59 +00:00
Hein
8a94d884e7
Verify bcrypt passwords in Direct mode and the shipped procedures, hash on register/reset, ignore client-supplied roles and level at registration and drop the password from the jwt_login payload. Legacy cleartext upgrade is opt-in. Row security templates now bind the user as a parameter, validate identifiers, attach via common.SelectQuery and fail the request if the filter cannot be attached. ApplyColumnSecurity and GetRowSecurityTemplate convert panics to errors and the hooks fail closed. Update audit status.
3534 lines
149 KiB
Markdown
3534 lines
149 KiB
Markdown
# Audit: `pkg/security`
|
||
|
||
| | |
|
||
|---|---|
|
||
| **Package** | `github.com/bitechdev/ResolveSpec/pkg/security` |
|
||
| **Size** | 35 non-test files, **11 256 lines** — the largest package in the repo |
|
||
| **Key files** | `oauth_server.go` (1 306), `providers.go` (1 274), `middleware.go` (737), `oauth2_examples.go` (615), `passkey_provider.go` (540), `oauth2_methods.go` (511), `providers_direct.go` (473), `provider.go` (458), `hooks.go` (441), `totp_provider_database.go` (326), `keystore_database.go` (297), `sql_names.go` (267), `query_mode.go` (169), `composite.go` (120), `table_names.go` (101), `chain.go` (57) |
|
||
| **Schema** | `database_schema.sql`, `database_schema_sqlite.sql`, `keystore_schema.sql` |
|
||
| **Docs** | `README.md`, `SECURITY_FEATURES.md`, `QUICK_REFERENCE.md`, `OAUTH2.md`, `OAUTH2_REFRESH_*.md`, `PASSKEY_QUICK_REFERENCE.md`, `KEYSTORE.md` |
|
||
| **Tests** | 6 359 lines across 13 `_test.go` files |
|
||
| **Audit date** | 2026-09-29 |
|
||
| **Axes** | thread locking/waiting, slowness, security, panic handling & logging |
|
||
| **Threat model** | hostile internet client; request bodies, headers, query params, schema/table/column names and filter expressions all attacker-controlled |
|
||
| **Depth** | deep |
|
||
|
||
## Summary
|
||
|
||
This is the package that decides who may read and write what. Three of its four
|
||
pillars do not hold.
|
||
|
||
**Authentication does not verify passwords.** `loginDirect`
|
||
(`providers_direct.go:30-87`) never reads `req.Password`: it selects the user by
|
||
username, mints a session token, and returns it. The shipped stored procedures do
|
||
the same — `resolvespec_login` has the `crypt()` check commented out
|
||
(`database_schema.sql:124-129`), as does `resolvespec_jwt_login`
|
||
(`database_schema.sql:353-357`). The file comment at `providers_direct.go:20-23`
|
||
states this is deliberate ("the stored procedures never verify the password hash
|
||
either… Direct mode matches that behavior exactly"). So in **both** query modes,
|
||
knowing a username is sufficient to obtain a valid 24-hour session. Registration
|
||
stores the password in cleartext (`providers_direct.go:125`,
|
||
`database_schema.sql:506-512`) and lets the registrant choose their own
|
||
`user_level` and `roles`. `resolvespec_jwt_login` returns the stored password in
|
||
its JSON payload (`database_schema.sql:367`). This is finding 1, and it is the
|
||
finding that matters: every other control in this package sits behind it.
|
||
|
||
**Row-level security is silently inert.** `applyRowSecurity` builds the WHERE
|
||
clause, logs `"Applying row security filter…"` at `Info`, and then tries to attach
|
||
it through a type assertion for `interface{ Where(string, ...interface{})
|
||
interface{} }` (`hooks.go:134-136`). No type in this repository has that method:
|
||
`common.SelectQuery.Where` returns `SelectQuery`, not `interface{}`
|
||
(`pkg/common/interfaces.go:47`). I verified with a compiled program that the
|
||
assertion fails and that only the corrected signature matches. The clause is
|
||
discarded, the miss is recorded at `Debug` (`hooks.go:139`) while the preceding
|
||
`Info` line claims success, and every tenant sees every row. The one part of row
|
||
security that does work is the `HasBlock` deny at `hooks.go:92-95` — which is
|
||
exactly what a manual test would exercise, so the failure looks like success.
|
||
Finding 2.
|
||
|
||
**Column masking has large holes.** `setColSecValue`
|
||
(`provider.go:256-299`) dispatches on `strings.ToLower(fieldval.Kind().String())`.
|
||
`float64` and `float32` match no case; `bool` matches no case; `time.Time` has
|
||
`Kind() == struct`, so the `"time"`/`"date"` case never fires; `[]byte` has
|
||
`Kind() == slice`, and the JSON branch it would need tests `fieldTypeName`, which
|
||
the caller passes as the **column name** (`provider.go:355`), not a type. Salary,
|
||
balance, date-of-birth and JSONB columns are therefore returned unmasked. And
|
||
`ApplyColumnSecurity` carries `defer logger.CatchPanic("ApplyColumnSecurity")()`
|
||
(`provider.go:302`) on a function with unnamed results, so any panic inside makes
|
||
it return a zero `reflect.Value` **and a nil error** — `applyColumnSecurity` reads
|
||
that as success and the response goes out unmasked. `GetRowSecurityTemplate` has
|
||
the same construct (`provider.go:443`) and fails open to "no security". Findings
|
||
3, 4, 5.
|
||
|
||
**Performance: every secured request serialises on two global mutexes, each held
|
||
across a database round trip.** `LoadColumnSecurity` holds
|
||
`ColumnSecurityMutex.Lock()` from `provider.go:375` across the provider call at
|
||
`:388`; `LoadRowSecurity` holds `RowSecurityMutex.Lock()` from `:424` across the
|
||
provider call at `:433`. Neither honours the `pOverwrite` cache flag that
|
||
`hooks.go:46`/`:59` pass — both always call the provider and always overwrite — so
|
||
there is no caching at all. Maximum throughput for the whole API becomes
|
||
`1 / (2 × provider_latency)`, and a slow database converts into a total stall,
|
||
because a mutex wait ignores the request's context deadline. Finding 6.
|
||
|
||
**Credentials reach the logs and the error tracker.** `providers.go:391` logs the
|
||
raw `Authorization` header at `Warn` whenever it contains a comma — attacker
|
||
triggerable, and per cross-cutting finding X8 every `Warn` is forwarded to Sentry.
|
||
`hooks.go:93` logs `%v` of the user ref at `Warn`; for every spec adapter that ref
|
||
is the whole `*UserContext`, whose `SessionID` field is the live session token
|
||
(`interfaces.go:13`, set at `providers_direct.go:77`). `hooks.go:129-130` prints
|
||
the same struct at `Info` on every secured read. Finding 7.
|
||
|
||
What is genuinely good, and should be protected in any remediation: the OAuth 2.1
|
||
authorization-code flow in `oauth_server.go` is carefully built — S256 PKCE is
|
||
mandatory (`:550-555`), `redirect_uri` is matched exactly against the registered
|
||
list (`:562`, `:602`), authorization codes are single-use under lock
|
||
(`:840-845`), client secrets are hashed and compared with
|
||
`subtle.ConstantTimeCompare` (`:1259`). The password-reset flow is textbook: 32
|
||
random bytes, SHA-256 at rest, one-hour expiry, a deliberately generic response to
|
||
prevent user enumeration, and all sessions deleted on completion
|
||
(`providers_direct.go:310-406`). Those two flows show the package knows how to do
|
||
this; the gap is that the primary login path never got the same treatment.
|
||
|
||
## Findings
|
||
|
||
| # | Severity | Axis | Finding | Status |
|
||
|---|---|---|---|---|
|
||
| 1 | **Critical** | security | Password is never verified — in Direct mode *and* in the shipped stored procedures; passwords stored in cleartext; self-registration chooses its own roles | Fixed (legacy upgrade opt-in) |
|
||
| 2 | **Critical** | security | Row-level security is silently inert: the `Where` type assertion at `hooks.go:134` can never match (verified) | Fixed |
|
||
| 3 | **Critical** | security / panic handling | `defer logger.CatchPanic(...)()` on unnamed results makes `ApplyColumnSecurity` and `GetRowSecurityTemplate` return "success, no rules" after a panic | Fixed |
|
||
| 4 | **High** | security | `setColSecValue` never masks `float*`, `bool`, `time.Time` or `[]byte`; its JSON branch tests the column *name*, not the type (verified) | Open |
|
||
| 5 | **High** | security | `loadSecurityRules` fails open on every error path — a transient provider error yields an unfiltered, unmasked query | Open |
|
||
| 6 | **High** | thread locking / slowness | Two package-wide write mutexes each held across a provider DB call, with `pOverwrite` ignored so nothing is ever cached | Fixed |
|
||
| 7 | **High** | security / logging | Session tokens and raw `Authorization` headers written to logs and forwarded to Sentry | Open |
|
||
| 8 | **High** | security | `HeaderAuthenticator` trusts `X-User-ID`/`X-User-Roles` from the client and is the authenticator used in the README's complete example | Open |
|
||
| 9 | **High** | security / slowness | One request can submit unlimited comma-separated tokens; each is tried against the database in turn | Partial |
|
||
| 10 | **High** | security | Logout does not invalidate the cached session when the token carries a `Bearer ` prefix — the key used to delete differs from the key used to store | Open |
|
||
| 11 | **High** | security | Unauthenticated dynamic client registration: any caller registers a client with self-chosen scopes, and the `clients` map is never pruned | Open |
|
||
| 12 | **High** | security | `JWTAuthenticator.Login` issues `token_<userid>_<expiry>` as a bearer token — trivially forgeable — and never verifies the password | Open |
|
||
| 34 | **High** | security | `lookupOrFetchClient` rehydrates a persisted client without its secret hash, so after a restart every confidential client is treated as public and client authentication is skipped | Open |
|
||
| 13 | **Medium** | security | `refresh_token` grant authenticates no client and binds the token to none | Open |
|
||
| 14 | **Medium** | security | Both `SecurityList` maps grow without bound, and their keys embed session tokens and JWT claims | Partial |
|
||
| 15 | **Medium** | security | Model rules fail open: unregistered models allow update and delete; `CheckModelAuthAllowed` grants every authenticated user every operation | Open |
|
||
| 16 | **Medium** | security / logging | `logDataAccess` is a `logger.Info` line with a TODO — there is no audit trail | Open |
|
||
| 17 | **Medium** | security | `ValidateSQLNames` / `ValidateTableNames` exist but are never called; names are interpolated into SQL unvalidated | Open |
|
||
| 18 | **Medium** | security | Internal error text echoed to unauthenticated clients on every auth failure | Open |
|
||
| 19 | **Medium** | security / panic handling | `probeFunctionExists` swallows a panic into `false`, silently downgrading the whole authenticator to Direct mode | Open |
|
||
| 20 | **Medium** | thread locking | `go a.updateSessionActivity(r.Context(), …)` — unbounded goroutine per request, no recover, and the context is already cancelled | Fixed |
|
||
| 21 | **Medium** | thread locking | `cleanupStates` and `cleanupExpired` goroutines: one unstoppable and leaked per `WithOAuth2` call | Fixed |
|
||
| 22 | **Medium** | security | `ApplyColumnSecurity` returns an error when a table has *no* rules, which `hooks.go:184` logs at `Warn` — one Sentry event per read | Open |
|
||
| 23 | **Medium** | security | Ephemeral RS256 signing key generated per process; `id_token`s break on restart and across replicas | Open |
|
||
| 24 | **Medium** | correctness | `contains` is prefix-or-suffix, not substring — primary-key detection misses `bun:"id,pk"` and `extractSQLName` returns `"column:name"` verbatim | Open |
|
||
| 25 | **Medium** | correctness | `maskString` masks one character too many at each end and indexes runes by byte offset (verified) | Open |
|
||
| 26 | **Low** | thread locking | Unsynchronised nil-map reads outside the lock in `ApplyColumnSecurity` and `GetRowSecurityTemplate` — a real race under `-race` | Fixed |
|
||
| 27 | **Low** | security | `SecurityList`'s maps and mutexes are exported, so any importer can mutate the security cache | Open |
|
||
| 28 | **Low** | correctness | `ColumSecurityApplyOnRecord` shadows `i` three times and indexes one slice with another's index | Open |
|
||
| 29 | **Low** | correctness | `ClearSecurity`'s filter condition is `&&` where it must be `||` (dead code — zero callers) | Open |
|
||
| 30 | **Low** | slowness | `splitTag` builds strings with `+=` inside a rune loop — O(n²) per struct tag, on every secured read | Fixed |
|
||
| 31 | **Low** | correctness | `registerDirect` uses `LastInsertId`, unsupported on Postgres, and checks uniqueness outside a transaction | Open |
|
||
| 32 | **Low** | security | `Authenticate` may return `(nil, nil)` through the callback, and the caller dereferences it | Open |
|
||
| 33 | **Low** | security | `requestPasswordReset` returns the raw reset token to its caller | Open |
|
||
|
||
## Resolution status (2026-09-30)
|
||
|
||
Addressed so far: the thread-locking, data-race and slowness findings (#6, #9, #20, #21,
|
||
#26, #30) and the three Critical findings (#1, #2, #3). Every other finding is untouched.
|
||
|
||
- **#1** — Fixed. Login now verifies the password in Direct mode (`loginDirect`, `jwtLoginDirect`) and in the shipped procedures (`resolvespec_login`, `resolvespec_jwt_login`), with `password.go` providing bcrypt hash/verify, a 72-byte limit (longer is rejected, not truncated) and a dummy comparison for unknown users. Passwords are hashed on write (`registerDirect`, `completePasswordResetDirect`, `resolvespec_register`, `resolvespec_password_reset`). Registration ignores client-supplied `user_level`/`roles` (and `program_user_*` in the procedure): new users are level 0 with no roles. `resolvespec_jwt_login` no longer returns the password and the Go struct field is gone. `database_schema.sql` now creates the `pgcrypto` extension. Legacy cleartext rows are still accepted at login, but rewriting them as bcrypt is **opt-in and off by default**: `DatabaseAuthenticatorOptions.UpgradePasswordHash`, `JWTAuthenticator.WithPasswordHashUpgrade(true)`, and for the procedures `ALTER DATABASE <db> SET resolvespec.upgrade_password_hash = 'on'`. Until enabled, cleartext passwords stay in the table. The procedures were exercised against a scratch PostgreSQL (wrong/empty/legacy/bcrypt passwords, registration privileges, payload) and Go-generated hashes verify with `crypt()`; the procedure path has no automated test in the repo. Not done: `JWTAuthenticator` still issues the forgeable `token_<id>_<exp>` (#12); the not-found branch of the procedures is not timing-equalised
|
||
- **#2** — Fixed. `RowSecurity.GetTemplate` now returns `(clause, args, error)`: `{UserID}` becomes a `?` placeholder with the user reduced to a scalar (`*UserContext` becomes its `UserID`; other structured values are rejected) and `{PrimaryKeyName}`/`{TableName}`/`{SchemaName}` must be plain identifiers. `applyRowSecurity` asserts `common.SelectQuery`, passes the bind args, and returns an error if the filter cannot be attached (previously it was silently dropped). The "Applying row security filter" line is now `Debug` and no longer prints the user. Tests use a fake `SelectQuery` that records `Where` calls. Not done: the `modelType.Kind()` check before `NumField()`
|
||
- **#3** — Fixed. `ApplyColumnSecurity` and `GetRowSecurityTemplate` have named results and convert a panic into an error via `logger.HandlePanic`. New sentinels `ErrNoColumnSecurity`/`ErrNoRowSecurity` mean "no rules"; the hooks treat those as success and fail the request on any other error (including a recovered panic). Not done: `loadSecurityRules` still ignores provider load errors (#5), so a failed load still reads as "no rules"; `setColSecValue` still lacks `CanSet()` guards (#4)
|
||
- Found while testing, not caused by this work: `database_schema.sql` fails to create one function (`parameter name "p_data" used more than once`, around line 1695); the same error occurs on the previously committed file
|
||
|
||
- **#6** — Fixed: `LoadColumnSecurity`/`LoadRowSecurity` no longer hold a mutex across the provider call (load first, then publish under the lock), the provider call gets a 10 s deadline derived from the request context, and `pOverwrite` is honoured. Results are cached for 30 s (`securityCacheTTL`; revocations take up to that long to apply) and expired entries are pruned on write after a further 30 s grace. Expiry is tracked in side maps, so the exported `ColumnSecurity`/`RowSecurity` maps keep their shape. Duplicate cold-key queries are not collapsed (no singleflight)
|
||
- **#9** — Partly fixed: `Authenticate` rejects more than 4 comma-separated tokens (`maxAuthTokens`) with `too many authorization tokens`, and splits with `SplitN` so a huge header is not fully split. Not done: aborting the loop on the first hard failure, per-token rate limiting, and dropping the header from the `Warn` (finding 7)
|
||
- **#14** — Partly fixed as a side effect of #6: entries now expire and are pruned. The unstable `%v` row-security key (session token and claims inside the key) is unchanged
|
||
- **#20** — Fixed: the activity update runs on `context.WithoutCancel(r.Context())` with a 5 s timeout and recovers panics; the existing `activityWG` tracks it. Not done: coalescing into one batched flush
|
||
- **#21** — Fixed: each `OAuth2Provider` has a stop channel and `cleanupStates` exits on it (and recovers panics); replacing a provider stops the old one; new `DatabaseAuthenticator.Close()` stops all of them and waits for in-flight activity updates. `Close` is not yet called from the server shutdown path
|
||
- **#26** — Fixed: the nil-map checks in `ApplyColumnSecurity`, `ColumSecurityApplyOnRecord` and `GetRowSecurityTemplate` now run inside the lock. Error messages are unchanged
|
||
- **#30** — Fixed: `splitTag` uses `strings.FieldsFunc`; `maskString` uses a `strings.Builder` (its off-by-one offsets, finding 25, are unchanged)
|
||
- Tests: `pkg/security/concurrency_test.go`, `direct_mode_test.go`, `hooks_test.go` (run with `-race`).
|
||
|
||
---
|
||
|
||
## 1. Critical — the password is never verified, in either query mode
|
||
|
||
`loginDirect` reads exactly one thing from the request: the username.
|
||
|
||
```go
|
||
// providers_direct.go:30-46
|
||
func (a *DatabaseAuthenticator) loginDirect(ctx context.Context, req LoginRequest) (*LoginResponse, error) {
|
||
var userID int
|
||
var email, roles, programUserTable sql.NullString
|
||
var userLevel, programUserID sql.NullInt64
|
||
|
||
err := a.runDBOpWithReconnect(func(db *sql.DB) error {
|
||
query := rewritePlaceholders(db, fmt.Sprintf(
|
||
`SELECT id, email, user_level, roles, program_user_id, program_user_table FROM %s WHERE username = ? AND is_active = ?`,
|
||
a.tableNames.Users))
|
||
return db.QueryRowContext(ctx, query, req.Username, true).Scan(&userID, &email, &userLevel, &roles, &programUserID, &programUserTable)
|
||
})
|
||
```
|
||
|
||
`req.Password` appears nowhere in the function. Execution continues straight to
|
||
`generateSessionToken()` at `:48` and the session row is inserted at `:60`. Any
|
||
request naming an existing active user receives a valid 24-hour session token.
|
||
|
||
This is not an oversight in one code path. The file header states it as policy:
|
||
|
||
```go
|
||
// providers_direct.go:20-23
|
||
// Password verification is intentionally not implemented here: the stored
|
||
// procedures never verify the password hash either (see the TODOs in
|
||
// database_schema.sql), so Direct mode matches that behavior exactly rather
|
||
// than introducing a mismatch between modes.
|
||
```
|
||
|
||
And the stored procedures confirm it:
|
||
|
||
```sql
|
||
-- database_schema.sql:119-129 (resolvespec_login)
|
||
IF NOT FOUND THEN
|
||
RETURN QUERY SELECT false, 'Invalid credentials'::text, NULL::jsonb;
|
||
RETURN;
|
||
END IF;
|
||
|
||
-- TODO: Verify password hash using pgcrypto extension
|
||
-- Enable pgcrypto: CREATE EXTENSION IF NOT EXISTS pgcrypto;
|
||
-- IF NOT (crypt(p_request->>'password', v_password_hash) = v_password_hash) THEN
|
||
-- RETURN QUERY SELECT false, 'Invalid credentials'::text, NULL::jsonb;
|
||
-- RETURN;
|
||
-- END IF;
|
||
```
|
||
|
||
`resolvespec_jwt_login` is the same (`database_schema.sql:353-357`), and
|
||
`JWTAuthenticator.Login` repeats it a third time in Go
|
||
(`providers.go:687-690`).
|
||
|
||
Three consequences compound it:
|
||
|
||
**Passwords are stored in cleartext.** The hashing step is commented out in the
|
||
procedure:
|
||
|
||
```sql
|
||
-- database_schema.sql:506-512
|
||
-- TODO: Hash password using pgcrypto extension
|
||
-- Enable pgcrypto: CREATE EXTENSION IF NOT EXISTS pgcrypto;
|
||
-- v_password := crypt(v_password, gen_salt('bf'));
|
||
|
||
-- Create new user
|
||
INSERT INTO users (username, email, password, user_level, roles, is_active, created_at, updated_at, program_user_id, program_user_table)
|
||
VALUES (v_username, v_email, v_password, v_user_level, v_roles, true, now(), now(), v_program_user_id, v_program_user_table)
|
||
```
|
||
|
||
and Direct mode inserts `req.Password` verbatim (`providers_direct.go:125`), as
|
||
does password reset (`providers_direct.go:392`, `UPDATE … SET password = ?` with
|
||
`req.NewPassword`). The column comment still says "bcrypt hashed password"
|
||
(`database_schema.sql:9`), so a reader of the schema would not notice.
|
||
|
||
**The stored password is returned to the caller.** `resolvespec_jwt_login`
|
||
includes it in its result payload:
|
||
|
||
```sql
|
||
-- database_schema.sql:360-370
|
||
RETURN QUERY SELECT
|
||
true,
|
||
NULL::text,
|
||
jsonb_build_object(
|
||
'id', v_user_id,
|
||
'username', v_username,
|
||
'email', v_email,
|
||
'password', v_password,
|
||
...
|
||
```
|
||
|
||
and `JWTAuthenticator.Login` unmarshals it into a struct field
|
||
(`providers.go:678`). It is not placed in `LoginResponse`, so it is not returned
|
||
over the wire — but it is now in a live Go value that any `%+v` log line or
|
||
error-tracker breadcrumb would capture.
|
||
|
||
**Self-registration chooses its own privileges.** `registerDirect` writes
|
||
`req.UserLevel` and `strings.Join(req.Roles, ",")` straight from the request
|
||
(`providers_direct.go:100`, `:125`), and the procedure takes the same fields from
|
||
`p_request` (`database_schema.sql:445` documents
|
||
`{username, password, email, user_level, roles, …}`). A client that can reach
|
||
`Register` becomes `user_level: 99, roles: ["admin"]`.
|
||
|
||
Finally, note that this reaches the OAuth server: `authorizePost` calls the same
|
||
login (`oauth_server.go:605-620`), so `/oauth/authorize` issues authorization
|
||
codes to anyone who names a valid username.
|
||
|
||
### Remediation
|
||
|
||
The fix is not a patch, it is the missing feature. In order:
|
||
|
||
1. Add a real verifier. In Go, `golang.org/x/crypto/bcrypt`:
|
||
```go
|
||
var hash string
|
||
// …SELECT id, password, … FROM users WHERE username = ? AND is_active = ?
|
||
if err := bcrypt.CompareHashAndPassword([]byte(hash), []byte(req.Password)); err != nil {
|
||
return nil, fmt.Errorf("invalid credentials") // same text as the not-found path
|
||
}
|
||
```
|
||
Keep the error text and the timing identical to the user-not-found branch
|
||
(`providers_direct.go:42-43`) — compare against a dummy hash when the user does
|
||
not exist, so the two paths cost the same.
|
||
2. In the procedures, enable `pgcrypto` and uncomment the three `crypt()` blocks.
|
||
3. Hash on write: `crypt(v_password, gen_salt('bf'))` in `resolvespec_register`,
|
||
and `bcrypt.GenerateFromPassword` in `registerDirect` and
|
||
`completePasswordResetDirect`.
|
||
4. Remove `'password', v_password` from `resolvespec_jwt_login`'s payload and the
|
||
`Password` field from the struct at `providers.go:678`.
|
||
5. Do not let a registration request set `user_level` or `roles`. Take them from
|
||
server-side policy; if a deployment genuinely needs client-supplied roles,
|
||
intersect against an allow-list.
|
||
6. Ship a migration for existing rows. Cleartext passwords cannot be converted —
|
||
rehash on next successful login, or force a reset.
|
||
7. Add a test that asserts login **fails** with a wrong password, for both modes.
|
||
The absence of that test is what let this ship: `providers_test.go` covers the
|
||
success path only.
|
||
|
||
Until step 1 lands, no other finding in this file can be assessed as mitigated,
|
||
because an attacker does not need to bypass column masking or row filters if they
|
||
can log in as the data's owner.
|
||
|
||
---
|
||
|
||
## 2. Critical — row-level security is silently inert everywhere
|
||
|
||
`applyRowSecurity` does all the work and then throws it away.
|
||
|
||
```go
|
||
// hooks.go:126-141
|
||
// Generate the WHERE clause from template
|
||
whereClause := rowSec.GetTemplate(pkName, modelType)
|
||
|
||
logger.Info("Applying row security filter for user %v on %s.%s: %s",
|
||
userRef, schema, tablename, whereClause)
|
||
|
||
// Apply the WHERE clause to the query
|
||
query := secCtx.GetQuery()
|
||
if selectQuery, ok := query.(interface {
|
||
Where(string, ...interface{}) interface{}
|
||
}); ok {
|
||
secCtx.SetQuery(selectQuery.Where(whereClause))
|
||
} else {
|
||
logger.Debug("Query doesn't support Where method, skipping row security")
|
||
}
|
||
```
|
||
|
||
The asserted method must return `interface{}`. The query that
|
||
`restheadspec/handler.go:924` places in the hook context is a
|
||
`common.SelectQuery`, whose `Where` returns `SelectQuery`:
|
||
|
||
```go
|
||
// pkg/common/interfaces.go:47
|
||
Where(query string, args ...interface{}) SelectQuery
|
||
```
|
||
|
||
Go interface satisfaction requires the method signature to match exactly,
|
||
including the result type, so the assertion fails for every query type in this
|
||
repository. I confirmed there is no other candidate: grepping for
|
||
`Where(string, ...interface{}) interface{}` finds only the assertion itself, and
|
||
`UpdateQuery.Where` (`pkg/common/interfaces.go:87`) and `DeleteQuery.Where`
|
||
(`:98`) return their own concrete interfaces too. I then compiled a minimal
|
||
program with a `SelectQuery`-shaped type and verified the assertion fails while
|
||
the corrected `Where(string, ...interface{}) SelectQuery` form succeeds.
|
||
|
||
Two things make this hard to notice:
|
||
|
||
* The `Info` line at `:129` is emitted **before** the assertion and says
|
||
"Applying row security filter … `<clause>`". The only record of the failure is
|
||
the `Debug` at `:139`, which is off in production
|
||
(`zap.NewProductionConfig()`, `logger/logger.go:24`). The logs assert that
|
||
row security is working.
|
||
* The deny path still works. `HasBlock` returns an error at `hooks.go:92-95`
|
||
before any of this, so "block user X from table Y" behaves correctly. A
|
||
reviewer who tests blocking concludes row security is live.
|
||
|
||
This affects every spec: `restheadspec/security_hooks.go:31-34`,
|
||
`resolvespec/security_hooks.go`, `websocketspec/security_hooks.go`,
|
||
`mqttspec/security_hooks.go`, `resolvemcp/security_hooks.go` and
|
||
`funcspec/security_adapter.go` all register `ApplyRowSecurity` against the same
|
||
`hooks.go` implementation.
|
||
|
||
### Remediation — and why the obvious fix is dangerous
|
||
|
||
The naive correction is to assert the real interface:
|
||
|
||
```go
|
||
if selectQuery, ok := query.(common.SelectQuery); ok {
|
||
secCtx.SetQuery(selectQuery.Where(whereClause))
|
||
}
|
||
```
|
||
|
||
Do not ship that alone. It would turn a dead code path into a SQL injection
|
||
sink, because of how the clause is built:
|
||
|
||
```go
|
||
// provider.go:43-50
|
||
func (m *RowSecurity) GetTemplate(pPrimaryKeyName string, pModelType reflect.Type) string {
|
||
str := m.Template
|
||
str = strings.ReplaceAll(str, "{PrimaryKeyName}", pPrimaryKeyName)
|
||
str = strings.ReplaceAll(str, "{TableName}", m.Tablename)
|
||
str = strings.ReplaceAll(str, "{SchemaName}", m.Schema)
|
||
str = strings.ReplaceAll(str, "{UserID}", fmt.Sprintf("%v", m.UserID))
|
||
return str
|
||
}
|
||
```
|
||
|
||
`m.UserID` is typed `any` (`provider.go:40`) and for every spec adapter it is the
|
||
whole `*UserContext` (`restheadspec/security_hooks.go:83-89`), so
|
||
`fmt.Sprintf("%v", …)` renders `&{1 alice 0 sess_… [user] alice@example.com
|
||
map[…] …}` — including JWT-derived `UserName`, `Email` and `Claims` — directly
|
||
into a WHERE clause that `hooks.go:137` passes with **no bind arguments**. A
|
||
template of `user_id = {UserID}` plus an attacker-influenced claim yields
|
||
arbitrary SQL. The two defects currently mask each other.
|
||
|
||
Fix both together:
|
||
|
||
1. Change the template contract to emit a placeholder and a bind value:
|
||
```go
|
||
func (m *RowSecurity) GetTemplate(pk string, t reflect.Type) (string, []any) {
|
||
str := m.Template
|
||
str = strings.ReplaceAll(str, "{PrimaryKeyName}", quoteIdent(pk))
|
||
str = strings.ReplaceAll(str, "{TableName}", quoteIdent(m.Tablename))
|
||
str = strings.ReplaceAll(str, "{SchemaName}", quoteIdent(m.Schema))
|
||
if !strings.Contains(str, "{UserID}") {
|
||
return str, nil
|
||
}
|
||
return strings.ReplaceAll(str, "{UserID}", "?"), []any{m.userIDScalar()}
|
||
}
|
||
```
|
||
where `userIDScalar()` returns an `int`/`string` — never a struct. Reject a
|
||
`*UserContext` here outright; a provider that needs a claim should put that
|
||
claim in `RowSecurity.UserID` itself.
|
||
2. Then assert `common.SelectQuery` and pass the args:
|
||
`selectQuery.Where(clause, args...)`.
|
||
3. Make the failure loud. A security filter that cannot be attached must fail the
|
||
request, not log at `Debug`:
|
||
```go
|
||
selectQuery, ok := query.(common.SelectQuery)
|
||
if !ok {
|
||
return fmt.Errorf("row security: query type %T does not support Where", query)
|
||
}
|
||
```
|
||
4. Add a test that asserts the generated SQL contains the filter. A unit test on
|
||
`applyRowSecurity` with a fake `SelectQuery` recording its calls would have
|
||
caught this on day one.
|
||
|
||
Note also that `hooks.go:113-124` walks `modelType.NumField()` without checking
|
||
`modelType.Kind() == reflect.Struct` first; a non-struct model panics there. The
|
||
spec handlers recover (`restheadspec/handler.go:396-400`), so it surfaces as a
|
||
500 rather than a crash.
|
||
|
||
---
|
||
|
||
## 3. Critical — `CatchPanic` on unnamed results converts a panic into "allowed"
|
||
|
||
Two functions in `provider.go` defer `logger.CatchPanic`:
|
||
|
||
```go
|
||
// provider.go:301-306
|
||
func (m *SecurityList) ApplyColumnSecurity(records reflect.Value, modelType reflect.Type, pUserID int, pSchema, pTablename string) (reflect.Value, error) {
|
||
defer logger.CatchPanic("ApplyColumnSecurity")()
|
||
|
||
if m.ColumnSecurity == nil {
|
||
return records, fmt.Errorf("security not initialized")
|
||
}
|
||
```
|
||
|
||
```go
|
||
// provider.go:442-447
|
||
func (m *SecurityList) GetRowSecurityTemplate(pUserRef any, pSchema, pTablename string) (RowSecurity, error) {
|
||
defer logger.CatchPanic("GetRowSecurityTemplate")()
|
||
|
||
if m.RowSecurity == nil {
|
||
return RowSecurity{}, fmt.Errorf("security not initialized")
|
||
}
|
||
```
|
||
|
||
`CatchPanic` returns a closure that calls `recover()`
|
||
(`logger/logger.go:153-186`). Recovering in a deferred function on a function
|
||
whose results are **unnamed** means the function returns its results' zero
|
||
values — here `(reflect.Value{}, nil)` and `(RowSecurity{}, nil)`. The `nil`
|
||
error is the problem: both callers treat it as success.
|
||
|
||
For column masking:
|
||
|
||
```go
|
||
// hooks.go:182-192
|
||
maskedResult, err := securityList.ApplyColumnSecurity(resultValue, modelType, userID, schema, tablename)
|
||
if err != nil {
|
||
logger.Warn("Column security error: %v", err)
|
||
// Don't fail the request, just log the issue
|
||
return nil
|
||
}
|
||
|
||
// Update the result with masked data
|
||
if maskedResult.IsValid() && maskedResult.CanInterface() {
|
||
secCtx.SetResult(maskedResult.Interface())
|
||
}
|
||
```
|
||
|
||
`err` is nil, `maskedResult.IsValid()` is false, so nothing happens and
|
||
`applyColumnSecurity` returns nil — "masking applied successfully". The handler
|
||
then serialises the untouched records (`restheadspec/handler.go:1006` sends
|
||
`modelPtr`, the same backing array `ApplyColumnSecurity` was supposed to mutate in
|
||
place). The client receives **unmasked** data and the response is a 200.
|
||
|
||
For row security, `GetRowSecurityTemplate` returning `(RowSecurity{}, nil)` gives
|
||
`HasBlock == false` and `Template == ""`, so `applyRowSecurity` returns nil at
|
||
`hooks.go:143` — "this table has no row security". A panic while looking up a
|
||
blocking rule therefore **unblocks** the user.
|
||
|
||
This is reachable. `setColSecValue` calls `fieldval.SetString`
|
||
(`provider.go:275`, `:277`), `SetZero` (`:271`) and `SetBytes` (`:295`) with no
|
||
`CanSet()` guard — only the integer branch checks (`:266`). A rule naming an
|
||
unexported field, or a `[]byte` branch reached on a non-byte slice, panics. One
|
||
such rule aborts masking for **all** columns and **all** records in the response,
|
||
silently.
|
||
|
||
The panic is logged (`Error` + Sentry via `CatchPanicCallback`), so there is a
|
||
trace — but the request still succeeds with unprotected data.
|
||
|
||
### Remediation
|
||
|
||
Name the results so the recovered value can be converted into a failure, and make
|
||
the caller fail closed:
|
||
|
||
```go
|
||
func (m *SecurityList) ApplyColumnSecurity(records reflect.Value, modelType reflect.Type,
|
||
pUserID int, pSchema, pTablename string) (out reflect.Value, err error) {
|
||
defer func() {
|
||
if r := recover(); r != nil {
|
||
out = reflect.Value{}
|
||
err = logger.HandlePanic("ApplyColumnSecurity", r) // logs + returns an error
|
||
}
|
||
}()
|
||
...
|
||
}
|
||
```
|
||
|
||
`logger.HandlePanic` (`logger/logger.go:197-211`) already does exactly this and is
|
||
the right helper here; `CatchPanic` is only appropriate on functions that return
|
||
nothing. Same change at `provider.go:443`.
|
||
|
||
Then close the caller:
|
||
|
||
```go
|
||
// hooks.go:183
|
||
if err != nil {
|
||
return fmt.Errorf("column security failed for %s.%s: %w", schema, tablename, err)
|
||
}
|
||
```
|
||
|
||
— see finding 22 for why that requires fixing the "no rules" error first, and
|
||
finding 5 for the same change on the load path. Separately, guard the reflect
|
||
writes in `setColSecValue` with `CanSet()` so the panic does not arise at all, and
|
||
have it return an error the caller aggregates rather than the ignored
|
||
`(int, reflect.Value)` pair.
|
||
|
||
`_CROSS-CUTTING` finding X8 covers the general `CatchPanic`-on-unnamed-results
|
||
hazard; this package is where it has the highest cost.
|
||
|
||
---
|
||
|
||
## 4. High — four common column types are never masked
|
||
|
||
`setColSecValue` decides what to do by string-matching the reflect *kind*:
|
||
|
||
```go
|
||
// provider.go:256-299 (abridged)
|
||
fieldKindLower := strings.ToLower(fieldval.Kind().String())
|
||
switch {
|
||
case strings.Contains(fieldKindLower, "int") && (mask || hide):
|
||
if fieldval.CanInt() && fieldval.CanSet() {
|
||
fieldval.SetInt(0)
|
||
}
|
||
case (strings.Contains(fieldKindLower, "time") || strings.Contains(fieldKindLower, "date")) && (mask || hide):
|
||
fieldval.SetZero()
|
||
case strings.Contains(fieldKindLower, "string"):
|
||
...
|
||
case strings.Contains(fieldTypeName, "json") && (mask || hide):
|
||
...
|
||
}
|
||
```
|
||
|
||
Working through the kinds a model actually uses:
|
||
|
||
| Go type | `Kind().String()` | Matches? | Result |
|
||
|---|---|---|---|
|
||
| `int`, `int64`, `uint8` | `int`, `int64`, `uint8` | yes (`"uint8"` contains `"int"`) | zeroed |
|
||
| `string` | `string` | yes | masked |
|
||
| **`float64`, `float32`** | `float64`, `float32` | **no** — `"float64"` does not contain `"int"` | **returned in full** |
|
||
| **`bool`** | `bool` | **no** | **returned in full** |
|
||
| **`time.Time`** | `struct` | **no** — `"struct"` contains neither `"time"` nor `"date"` | **returned in full** |
|
||
| **`[]byte` / JSONB** | `slice` | **no** — see below | **returned in full** |
|
||
| `*string` | (dereferenced at `:258-260`) | yes | masked |
|
||
|
||
So `salary float64`, `account_balance`, `is_vip bool`, `date_of_birth time.Time`
|
||
and every JSONB column pass through a "mask" rule untouched. I verified the string
|
||
matching: `strings.Contains("float64", "int")` is false, `strings.Contains("uint8",
|
||
"int")` is true, and `reflect.TypeOf(time.Time{}).Kind().String()` is `"struct"`.
|
||
|
||
The JSON branch cannot fire for the intended reason. It tests `fieldTypeName`, and
|
||
the caller passes the **column name**:
|
||
|
||
```go
|
||
// provider.go:353-356
|
||
if i == pathLen-1 {
|
||
if nameType == "sql" || nameType == "struct" {
|
||
setColSecValue(field, *colsec, fieldName)
|
||
}
|
||
```
|
||
|
||
where `fieldName` was assigned from `cols.SQLName` or `cols.Name`
|
||
(`provider.go:341`, `:347`). The branch therefore fires only when the column
|
||
happens to be *named* something containing "json" — and when it does fire on a
|
||
non-`[]byte` field, `fieldval.Bytes()` at `:285` panics, which finding 3 turns
|
||
into a silent unmasked response. Conversely a JSONB column mapped to `string`
|
||
matches the earlier `"string"` case and has its **entire document** masked instead
|
||
of the configured path.
|
||
|
||
Also note the switch has no default: an unmatched kind returns `(0, fieldsrc)`
|
||
indistinguishably from success, and both return values are discarded at the call
|
||
site.
|
||
|
||
### Remediation
|
||
|
||
Dispatch on the type, not on a substring of the kind's name, and report what could
|
||
not be handled:
|
||
|
||
```go
|
||
func setColSecValue(fieldsrc reflect.Value, colsec ColumnSecurity, sqlName string) error {
|
||
fieldval := fieldsrc
|
||
for fieldval.Kind() == reflect.Pointer || fieldval.Kind() == reflect.Interface {
|
||
if fieldval.IsNil() {
|
||
return nil
|
||
}
|
||
fieldval = fieldval.Elem()
|
||
}
|
||
if !fieldval.CanSet() {
|
||
return fmt.Errorf("column %s: field not settable", sqlName)
|
||
}
|
||
hide := strings.EqualFold(colsec.Accesstype, "hide")
|
||
|
||
// Concrete types first — before any Kind switch.
|
||
switch fieldval.Interface().(type) {
|
||
case time.Time:
|
||
fieldval.SetZero()
|
||
return nil
|
||
case []byte:
|
||
return maskJSONPath(fieldval, colsec) // the gjson/sjson path
|
||
}
|
||
|
||
switch fieldval.Kind() {
|
||
case reflect.String:
|
||
if hide { fieldval.SetString(""); return nil }
|
||
fieldval.SetString(maskString(...))
|
||
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
|
||
fieldval.SetInt(0)
|
||
case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64:
|
||
fieldval.SetUint(0)
|
||
case reflect.Float32, reflect.Float64:
|
||
fieldval.SetFloat(0)
|
||
case reflect.Bool:
|
||
fieldval.SetBool(false)
|
||
default:
|
||
return fmt.Errorf("column %s: unsupported kind %s for %s",
|
||
sqlName, fieldval.Kind(), colsec.Accesstype)
|
||
}
|
||
return nil
|
||
}
|
||
```
|
||
|
||
Decide the JSON case on the field's type (`[]byte`, `json.RawMessage`, or a
|
||
driver-specific JSONB wrapper), never on its name. `ApplyColumnSecurity` should
|
||
collect the returned errors and propagate them; with finding 3 fixed, an
|
||
unmaskable column then fails the request instead of leaking.
|
||
|
||
Test matrix worth adding: one model with `string`, `*string`, `int64`, `float64`,
|
||
`bool`, `time.Time`, `[]byte` and an unexported field, each with `mask` and `hide`,
|
||
asserting the value actually changed.
|
||
|
||
---
|
||
|
||
## 5. High — `loadSecurityRules` fails open on every path
|
||
|
||
```go
|
||
// hooks.go:32-67
|
||
func loadSecurityRules(secCtx SecurityContext, securityList *SecurityList) error {
|
||
userID, ok := secCtx.GetUserID()
|
||
if !ok {
|
||
logger.Warn("No user ID in context for security check")
|
||
return nil
|
||
}
|
||
...
|
||
err := securityList.LoadColumnSecurity(secCtx.GetContext(), userID, schema, tablename, false)
|
||
if err != nil {
|
||
logger.Warn("Failed to load column security: %v", err)
|
||
// Don't fail the request if no security rules exist
|
||
// return err
|
||
}
|
||
...
|
||
_, err = securityList.LoadRowSecurity(secCtx.GetContext(), userRef, schema, tablename, false)
|
||
if err != nil {
|
||
logger.Warn("Failed to load row security: %v", err)
|
||
// Don't fail the request if no security rules exist
|
||
// return err
|
||
}
|
||
|
||
return nil
|
||
}
|
||
```
|
||
|
||
Three fail-open points, with the `return err` commented out at two of them. The
|
||
stated rationale — "don't fail the request if no security rules exist" — conflates
|
||
*no rules* with *could not determine the rules*. The consequences differ
|
||
completely:
|
||
|
||
* A provider timeout, a closed connection, a missing stored procedure, a
|
||
permissions error on the security tables: `ColumnSecurity[key]` is left empty
|
||
and `RowSecurity[key]` is not written, so the subsequent `ApplyColumnSecurity`
|
||
finds no rules and `GetRowSecurityTemplate` reports "no row security". The
|
||
request proceeds **unmasked and unfiltered**. An attacker who can induce load on
|
||
the security database gets a window of unrestricted reads.
|
||
* `GetUserID` is `ctx.Value(UserIDKey).(int)` (`middleware.go:402-405`). Any
|
||
deployment whose user identifier is not an `int` — a UUID subject from an OIDC
|
||
provider, for example — gets `ok == false` and **no rules are loaded at all**,
|
||
for every request. The `Warn` at `:36` is the only signal, and per X8 it floods
|
||
Sentry with one event per request.
|
||
|
||
Note `GetUserRef` (`:55-58`) was added to handle non-integer identifiers, but
|
||
`loadSecurityRules` still returns early on the `GetUserID` check before reaching
|
||
it, and `LoadColumnSecurity`'s signature is `int`-only
|
||
(`provider.go:370`), so column security cannot be loaded for a non-integer user
|
||
at all.
|
||
|
||
### Remediation
|
||
|
||
Distinguish "no rules" from "unknown". Have the provider contract say so — e.g.
|
||
a sentinel `ErrNoSecurityRules` that the loader treats as success and everything
|
||
else as failure:
|
||
|
||
```go
|
||
if err := securityList.LoadColumnSecurity(ctx, userID, schema, tablename, false); err != nil {
|
||
if !errors.Is(err, ErrNoSecurityRules) {
|
||
return fmt.Errorf("column security unavailable for %s.%s: %w", schema, tablename, err)
|
||
}
|
||
}
|
||
```
|
||
|
||
and register the hook so a returned error aborts the request. For the user-ID
|
||
path, widen `LoadColumnSecurity` to take the same `any` ref `LoadRowSecurity`
|
||
takes, and drop the `GetUserID` gate in favour of `GetUserRef`:
|
||
|
||
```go
|
||
userRef, ok := secCtx.GetUserRef()
|
||
if !ok {
|
||
return fmt.Errorf("no user identity in context") // fail closed
|
||
}
|
||
```
|
||
|
||
If a deployment genuinely wants unauthenticated reads, that is what
|
||
`ModelRules.CanPublicRead` is for (`hooks.go:254`) — an explicit opt-in per model,
|
||
not an implicit consequence of a missing context value.
|
||
|
||
---
|
||
|
||
## 6. High — two global mutexes, each held across a database call, with no caching
|
||
|
||
```go
|
||
// provider.go:370-395
|
||
func (m *SecurityList) LoadColumnSecurity(ctx context.Context, pUserID int, pSchema, pTablename string, pOverwrite bool) error {
|
||
if m.provider == nil {
|
||
return fmt.Errorf("security provider not set")
|
||
}
|
||
|
||
m.ColumnSecurityMutex.Lock()
|
||
defer m.ColumnSecurityMutex.Unlock()
|
||
...
|
||
if pOverwrite || m.ColumnSecurity[secKey] == nil {
|
||
m.ColumnSecurity[secKey] = make([]ColumnSecurity, 0)
|
||
}
|
||
|
||
// Call the provider to load security rules
|
||
colSecList, err := m.provider.GetColumnSecurity(ctx, pUserID, pSchema, pTablename)
|
||
if err != nil {
|
||
return fmt.Errorf("GetColumnSecurity failed: %v", err)
|
||
}
|
||
|
||
m.ColumnSecurity[secKey] = colSecList
|
||
return nil
|
||
}
|
||
```
|
||
|
||
```go
|
||
// provider.go:419-440 (abridged)
|
||
m.RowSecurityMutex.Lock()
|
||
defer m.RowSecurityMutex.Unlock()
|
||
...
|
||
record, err := m.provider.GetRowSecurity(ctx, pUserRef, pSchema, pTablename)
|
||
...
|
||
m.RowSecurity[secKey] = record
|
||
```
|
||
|
||
Three problems, compounding.
|
||
|
||
**The lock spans the round trip.** `GetColumnSecurity` and `GetRowSecurity` are
|
||
the provider interface (`interfaces.go:133-137`); the shipped implementations are
|
||
`DatabaseColumnSecurityProvider` / `DatabaseRowSecurityProvider`, which issue SQL.
|
||
The write lock is therefore held for the full database latency. Every secured
|
||
request calls both loaders (`hooks.go:46`, `:59`), so the ceiling on the whole
|
||
API's throughput is `1 / (2 × provider_latency)` — with a 5 ms security query,
|
||
about 100 req/s regardless of how many cores or connections are available.
|
||
|
||
**A slow database becomes a total stall.** `sync.Mutex.Lock` has no timeout and
|
||
ignores `ctx`. If the security provider's connection pool saturates, every
|
||
in-flight and future request blocks in `Lock()` rather than failing with the
|
||
request's deadline. Cancelled clients still hold their place in the queue.
|
||
|
||
**There is no caching, so this happens on every request.** Both functions take a
|
||
`pOverwrite` parameter and both effectively ignore it: `LoadColumnSecurity`
|
||
consults it only to reset the slice at `:383` and then unconditionally queries at
|
||
`:388` and overwrites at `:393`; `LoadRowSecurity` does not reference `pOverwrite`
|
||
at all. `hooks.go:46` and `:59` both pass `false` — expecting a cache — and get a
|
||
database call anyway. The two maps are thus write-only caches: they accumulate
|
||
entries (finding 14) that are never read as a hit.
|
||
|
||
`GetRowSecurityTemplate` (`:442-458`) does take only `RLock`, so the read side is
|
||
concurrent — but it reads the entry that was just overwritten microseconds
|
||
earlier under the write lock, so the read lock buys nothing.
|
||
|
||
### Remediation
|
||
|
||
1. **Never call the provider under the lock.** Load first, then publish:
|
||
```go
|
||
func (m *SecurityList) LoadColumnSecurity(ctx context.Context, ref any, schema, table string, overwrite bool) error {
|
||
key := colSecKey(schema, table, ref)
|
||
|
||
if !overwrite {
|
||
m.mu.RLock()
|
||
_, hit := m.columnSecurity[key]
|
||
m.mu.RUnlock()
|
||
if hit {
|
||
return nil
|
||
}
|
||
}
|
||
|
||
list, err := m.provider.GetColumnSecurity(ctx, ref, schema, table) // no lock held
|
||
if err != nil {
|
||
return fmt.Errorf("GetColumnSecurity failed: %w", err)
|
||
}
|
||
|
||
m.mu.Lock()
|
||
m.columnSecurity[key] = entry{rules: list, loadedAt: time.Now()}
|
||
m.mu.Unlock()
|
||
return nil
|
||
}
|
||
```
|
||
Two callers may duplicate one query on a cold key; that is strictly better than
|
||
serialising all of them. If duplicate work matters, collapse it with
|
||
`golang.org/x/sync/singleflight` keyed on `key` — that also keeps a thundering
|
||
herd off the security database.
|
||
2. **Honour `pOverwrite` and add a TTL.** Security rules that never expire are
|
||
also a correctness problem: revoking a mask takes effect only on restart. A
|
||
short TTL (30–60 s) plus an explicit invalidate is the usual shape. The
|
||
`Cacheable` interface the composite provider already probes for
|
||
(`composite.go:99-120`) is the natural hook.
|
||
3. **Consider the existing cache package.** `pkg/cache` already provides TTL,
|
||
tag-based invalidation and a size cap; `DatabaseAuthenticator` uses it for
|
||
sessions (`providers.go:402`). Reusing it here would fix caching, bounding and
|
||
invalidation in one move — and see `cache.audit.md` findings 1, 2 and 7-9 for
|
||
the caveats to apply when doing so.
|
||
4. **Wrap the provider call with a deadline** derived from the request context so a
|
||
hung security query fails the request rather than occupying a slot.
|
||
|
||
Note that fixing the lock is what *enables* finding 5's fail-closed change to be
|
||
safe: failing closed on a provider error is only acceptable once a provider stall
|
||
cannot take the whole process down with it.
|
||
|
||
---
|
||
|
||
## 7. High — session tokens and `Authorization` headers are written to logs and Sentry
|
||
|
||
Three places log credentials.
|
||
|
||
**The raw `Authorization` header, at `Warn`:**
|
||
|
||
```go
|
||
// providers.go:389-392
|
||
// Log warning if multiple tokens are provided
|
||
if len(tokens) > 1 {
|
||
logger.Warn("Multiple authentication tokens provided in Authorization header (%d tokens). This is unusual and may indicate a misconfigured client. Header: %s", len(tokens), sessionToken)
|
||
}
|
||
```
|
||
|
||
`sessionToken` here is the unparsed header value (`:354`). Any client can trigger
|
||
this by sending `Authorization: Bearer abc, Bearer def` — so an attacker can
|
||
choose what gets written, and a legitimate client with a quirk writes its real
|
||
token. Per `_CROSS-CUTTING` X8 every `logger.Warn` is forwarded to the error
|
||
tracker (`logger/logger.go:118-122`), so the token leaves the host entirely and
|
||
lands in Sentry's retention.
|
||
|
||
**The whole `UserContext`, including `SessionID`, at `Warn` and `Info`:**
|
||
|
||
```go
|
||
// hooks.go:92-95
|
||
if rowSec.HasBlock {
|
||
logger.Warn("User %v blocked from accessing %s.%s", userRef, schema, tablename)
|
||
return fmt.Errorf("access denied to %s", tablename)
|
||
}
|
||
```
|
||
|
||
```go
|
||
// hooks.go:129-130
|
||
logger.Info("Applying row security filter for user %v on %s.%s: %s",
|
||
userRef, schema, tablename, whereClause)
|
||
```
|
||
|
||
`userRef` comes from `GetUserRef`, which returns the full `*UserContext`
|
||
(`restheadspec/security_hooks.go:83-89`, and identically in the websocketspec,
|
||
mqttspec, resolvespec, resolvemcp and funcspec adapters). `fmt`'s `%v` on a
|
||
pointer-to-struct prints every field:
|
||
|
||
```go
|
||
// interfaces.go:9-23
|
||
type UserContext struct {
|
||
UserID int `json:"user_id"`
|
||
UserName string `json:"user_name"`
|
||
UserLevel int `json:"user_level"`
|
||
SessionID string `json:"session_id"`
|
||
...
|
||
Claims map[string]any `json:"claims"`
|
||
Meta map[string]any `json:"meta"`
|
||
```
|
||
|
||
`SessionID` holds the live session token — `loginDirect` assigns
|
||
`SessionID: sessionToken` (`providers_direct.go:77`), as does `registerDirect`
|
||
(`:169`). So `hooks.go:93` sends a working session token to Sentry, and
|
||
`hooks.go:129` writes one to the log file on **every secured read of every table
|
||
that has a row-security template**. `Claims` is the full OIDC userinfo map when an
|
||
external provider is used (`oauth2_methods.go:356`), so JWT claims go with it.
|
||
|
||
`hooks.go:87` (`logger.Debug("No row security for %s.%s@%v: %v", …, userRef, err)`)
|
||
has the same content but at `Debug`, which production configuration suppresses and
|
||
which X8 does not forward — lower risk, same fix.
|
||
|
||
### Remediation
|
||
|
||
1. Never format a `UserContext` with `%v`. Give it a redacting `String()`:
|
||
```go
|
||
func (u *UserContext) String() string {
|
||
if u == nil { return "<nil user>" }
|
||
return fmt.Sprintf("user{id=%d name=%q level=%d roles=%v}", u.UserID, u.UserName, u.UserLevel, u.Roles)
|
||
}
|
||
```
|
||
`fmt` will use it for both `%v` and `%s`, fixing all three call sites at once
|
||
— and also shrinking the `SecurityList` map keys in finding 14. Pair it with a
|
||
`MarshalJSON` that omits `SessionID`, so the struct cannot leak through a JSON
|
||
log encoder either.
|
||
2. At `providers.go:391`, drop the header from the message. The count is the
|
||
actionable part:
|
||
`logger.Warn("Authorization header carried %d tokens; using the first that validates", len(tokens))`.
|
||
Better still, make it `Debug` — a multi-token header is client-triggerable, so
|
||
at `Warn` it is also an attacker-controlled Sentry flood (X8).
|
||
3. Lower `hooks.go:129` to `Debug`. "Row security applied" on every read is not a
|
||
`Warn`-worthy event, and at `Info` it doubles the log volume of the read path.
|
||
4. Repo-wide, grep for `%v` applied to anything reaching a `UserContext`,
|
||
`LoginRequest` (which has a `Password` field, `interfaces.go:28`) or
|
||
`LoginResponse` before considering this closed.
|
||
|
||
---
|
||
|
||
## 8. High — `HeaderAuthenticator` trusts the client, and the README recommends it
|
||
|
||
```go
|
||
// providers.go:18-23
|
||
// Production-Ready Authenticators
|
||
// =================================
|
||
|
||
// HeaderAuthenticator provides simple header-based authentication
|
||
// Expects: X-User-ID, X-User-Name, X-User-Level, X-Session-ID, X-Remote-ID, X-User-Roles, X-User-Email
|
||
type HeaderAuthenticator struct{}
|
||
```
|
||
|
||
```go
|
||
// providers.go:45-64
|
||
func (a *HeaderAuthenticator) Authenticate(r *http.Request) (*UserContext, error) {
|
||
userIDStr := r.Header.Get("X-User-ID")
|
||
if userIDStr == "" {
|
||
return nil, fmt.Errorf("X-User-ID header required")
|
||
}
|
||
|
||
userID, err := strconv.Atoi(userIDStr)
|
||
if err != nil {
|
||
return nil, fmt.Errorf("invalid user ID: %w", err)
|
||
}
|
||
|
||
return &UserContext{
|
||
UserID: userID,
|
||
UserName: r.Header.Get("X-User-Name"),
|
||
UserLevel: parseIntHeader(r, "X-User-Level", 0),
|
||
...
|
||
Roles: parseRoles(r.Header.Get("X-User-Roles")),
|
||
}, nil
|
||
}
|
||
```
|
||
|
||
Every field comes from a request header. `curl -H 'X-User-ID: 1' -H
|
||
'X-User-Roles: admin' -H 'X-User-Level: 99'` authenticates as user 1 with
|
||
administrative roles. There is no shared secret, no signature, no trusted-proxy
|
||
list, and nothing that distinguishes a header injected by a gateway from one sent
|
||
by the client.
|
||
|
||
This pattern is legitimate — but only behind a proxy that strips the headers from
|
||
inbound requests and re-adds them after authenticating. Nothing in this package
|
||
says so. It sits under a heading that says "Production-Ready", and it is the
|
||
authenticator in the README's end-to-end wiring example:
|
||
|
||
```go
|
||
// pkg/security/README.md:655-666
|
||
"public.orders": "user_id = {UserID}",
|
||
}
|
||
|
||
// Create providers
|
||
auth := security.NewHeaderAuthenticator()
|
||
colSec := security.NewConfigColumnSecurityProvider(columnRules)
|
||
rowSec := security.NewConfigRowSecurityProvider(rowTemplates, nil)
|
||
|
||
// Combine providers and register hooks
|
||
provider := security.NewCompositeSecurityProvider(auth, colSec, rowSec)
|
||
securityList := security.NewSecurityList(provider)
|
||
restheadspec.RegisterSecurityHooks(handler, securityList)
|
||
```
|
||
|
||
A developer following that example ships an API where authentication is a header.
|
||
`README.md:298` does mark `DatabaseAuthenticator` as "(Recommended)", but the
|
||
complete example contradicts it.
|
||
|
||
Reachability: `NewHeaderAuthenticator` has **no non-test callers** in this
|
||
repository (only its own constructor at `providers.go:25-26`), so nothing here is
|
||
currently exposed. This is a hazard shipped to consumers, not a live
|
||
vulnerability — which is why it is High rather than Critical.
|
||
|
||
### Remediation
|
||
|
||
Do not delete it; make misuse hard.
|
||
|
||
1. Require the trust boundary to be declared. Give the constructor a mandatory
|
||
configuration:
|
||
```go
|
||
type HeaderAuthenticatorConfig struct {
|
||
TrustedProxies []netip.Prefix // required, no default
|
||
RequiredSecret string // optional shared secret in X-Auth-Proxy-Secret
|
||
}
|
||
func NewHeaderAuthenticator(cfg HeaderAuthenticatorConfig) (*HeaderAuthenticator, error) {
|
||
if len(cfg.TrustedProxies) == 0 {
|
||
return nil, errors.New("HeaderAuthenticator requires TrustedProxies: these headers are client-controlled")
|
||
}
|
||
...
|
||
}
|
||
```
|
||
and have `Authenticate` reject any request whose direct peer
|
||
(`r.RemoteAddr`, **not** `X-Forwarded-For` — see `middleware.audit.md` finding 2)
|
||
is outside the list. Compare the secret with `subtle.ConstantTimeCompare`.
|
||
2. Move it out from under "Production-Ready Authenticators", and put the warning in
|
||
the doc comment rather than only in prose: *"trusts unauthenticated request
|
||
headers; safe only behind a reverse proxy that strips `X-User-*` from inbound
|
||
requests."*
|
||
3. Change the README's complete example to `NewDatabaseAuthenticator(db)`. If a
|
||
header example is wanted, show it with the trusted-proxy configuration and the
|
||
nginx/envoy `proxy_set_header` lines that make it sound.
|
||
|
||
---
|
||
|
||
## 9. High — one request can brute-force unlimited session tokens
|
||
|
||
```go
|
||
// providers.go:365-396 (abridged)
|
||
} else {
|
||
// Parse Authorization header which may contain multiple comma-separated tokens
|
||
// Format: "Token abc, Token def" or "Bearer abc" or just "abc"
|
||
rawTokens := strings.Split(sessionToken, ",")
|
||
for _, token := range rawTokens {
|
||
token = strings.TrimSpace(token)
|
||
token = strings.TrimPrefix(token, "Bearer ")
|
||
token = strings.TrimPrefix(token, "Token ")
|
||
token = strings.TrimSpace(token)
|
||
if token != "" {
|
||
tokens = append(tokens, token)
|
||
}
|
||
}
|
||
}
|
||
...
|
||
// Try each token until one succeeds
|
||
var lastErr error
|
||
for _, token := range tokens {
|
||
```
|
||
|
||
`strings.Split` on an attacker-supplied header with no cap on the number of
|
||
elements, feeding a loop that performs one cache lookup and, on a miss, one
|
||
database round trip per element (`:402-438`).
|
||
|
||
`Authorization: a,b,c,…` with 10 000 elements is one HTTP request that issues
|
||
10 000 session lookups. Two consequences:
|
||
|
||
* **Rate-limit bypass for credential stuffing.** Any per-request throttle — and
|
||
per `middleware.audit.md` finding 1 there is currently none mounted at all —
|
||
counts this as a single request while it tests 10 000 candidate tokens.
|
||
* **Amplified load.** Each miss is a `QueryRowContext` against the session
|
||
procedure. The request body can be empty; the header does the work. Go's default
|
||
`MaxHeaderBytes` is 1 MiB, which at ~4 bytes per element allows on the order of
|
||
200 000 lookups per request.
|
||
|
||
The loop also stops at the *first* token that validates, so a valid token placed
|
||
last still works after thousands of failures — there is no early abort.
|
||
|
||
### Remediation
|
||
|
||
1. Cap the count, and reject rather than truncate:
|
||
```go
|
||
const maxAuthTokens = 4
|
||
if len(rawTokens) > maxAuthTokens {
|
||
return nil, fmt.Errorf("too many authorization tokens")
|
||
}
|
||
```
|
||
Four is generous; RFC 7235 expects one credential per header. If the
|
||
multi-token behaviour exists for a specific client, one is the right number
|
||
and that client should be fixed.
|
||
2. Abort the loop on the first *hard* failure. "Invalid or expired session"
|
||
(`:424`) means this credential is wrong — there is no reason to keep trying
|
||
unless the caller legitimately holds several. Distinguish it from a transport
|
||
error, which should fail the request rather than fall through to the next token.
|
||
3. Charge the rate limiter per token, not per request, once a limiter is mounted
|
||
(`middleware.audit.md` finding 1).
|
||
4. Drop the `Warn` on the multi-token path (finding 7) — with a cap in place, the
|
||
log line loses its purpose and its Sentry cost.
|
||
|
||
---
|
||
|
||
## 10. High — logout does not invalidate a cached `Bearer ` session
|
||
|
||
`Authenticate` strips the scheme prefix **before** building the cache key:
|
||
|
||
```go
|
||
// providers.go:371-374, 398
|
||
token = strings.TrimPrefix(token, "Bearer ")
|
||
token = strings.TrimPrefix(token, "Token ")
|
||
token = strings.TrimSpace(token)
|
||
...
|
||
cacheKey := fmt.Sprintf("auth:session:%s", token)
|
||
```
|
||
|
||
`Logout` builds the key from the **unstripped** request field:
|
||
|
||
```go
|
||
// providers.go:316-320
|
||
// Clear cache for this token
|
||
if req.Token != "" {
|
||
cacheKey := fmt.Sprintf("auth:session:%s", req.Token)
|
||
_ = a.cache.Delete(ctx, cacheKey)
|
||
}
|
||
```
|
||
|
||
`logoutDirect` has the same split — it strips for the SQL but not for the cache:
|
||
|
||
```go
|
||
// providers_direct.go:182-184, 203-206
|
||
token := req.Token
|
||
token = strings.TrimPrefix(token, "Bearer ")
|
||
token = strings.TrimPrefix(token, "bearer ")
|
||
...
|
||
if req.Token != "" {
|
||
cacheKey := fmt.Sprintf("auth:session:%s", req.Token)
|
||
_ = a.cache.Delete(ctx, cacheKey)
|
||
}
|
||
```
|
||
|
||
So a client that logs out with `{"token": "Bearer sess_abc…"}` — the natural
|
||
thing to send back, and exactly what `logoutDirect:183` anticipates — deletes
|
||
`auth:session:Bearer sess_abc…`, a key that was never written. The live entry
|
||
`auth:session:sess_abc…` survives. The session row is deleted from the database,
|
||
but `Authenticate` serves the cached `UserContext` without consulting the
|
||
database:
|
||
|
||
```go
|
||
// providers.go:402-403
|
||
err := a.cache.GetOrSet(r.Context(), cacheKey, &userCtx, a.cacheTTL, func() (any, error) {
|
||
// This function is called only if cache miss
|
||
```
|
||
|
||
The token therefore keeps working until `cacheTTL` expires — five minutes by
|
||
default (`providers.go:134`, `:140`). On a shared or stolen device, "log out"
|
||
does not end the session.
|
||
|
||
`logoutDirect:184` also strips only `"Bearer "` and `"bearer "`, while
|
||
`Authenticate:373` strips `"Token "` as well, so a `Token …`-prefixed logout
|
||
fails to match the database row either (`rows == 0` → `"session not found"` at
|
||
`:199-201`) while the cache entry also survives.
|
||
|
||
### Remediation
|
||
|
||
Normalise once, in one place, and use it everywhere:
|
||
|
||
```go
|
||
// normalizeSessionToken strips any supported scheme prefix. The result is the
|
||
// value used both as the cache key suffix and as the stored session_token.
|
||
func normalizeSessionToken(s string) string {
|
||
s = strings.TrimSpace(s)
|
||
for _, p := range []string{"Bearer ", "bearer ", "Token ", "token "} {
|
||
if rest, ok := cutPrefixFold(s, p); ok {
|
||
return strings.TrimSpace(rest)
|
||
}
|
||
}
|
||
return s
|
||
}
|
||
|
||
func sessionCacheKey(token string) string {
|
||
return "auth:session:" + normalizeSessionToken(token)
|
||
}
|
||
```
|
||
|
||
Replace all four sites (`providers.go:398`, `:466`, `:318`;
|
||
`providers_direct.go:183`, `:204`) with these helpers.
|
||
|
||
Two hardening steps beyond the bug:
|
||
|
||
* Have `Logout` delete the cache entry **before** the database call, and again
|
||
after, so a failed database logout still drops the cached credential.
|
||
* Consider whether a 5-minute positive cache on sessions is the right default at
|
||
all. It is a 5-minute window on every revocation, not just logout — password
|
||
reset (`providers_direct.go:395`) deletes session rows but nothing clears their
|
||
cache entries, so a reset password also leaves sessions live for `cacheTTL`.
|
||
`ClearCache(token)` exists (`providers.go:463-471`) but the reset path does not
|
||
call it.
|
||
|
||
Separately: session tokens are stored in the database in plaintext
|
||
(`providers_direct.go:60` inserts `sessionToken` directly, and `sessionDirect:219`
|
||
matches on equality), and they are also cache keys — so both the cache keyspace
|
||
and a database read expose usable credentials. Storing `sha256(token)` and
|
||
matching on the hash would remove both exposures, at the cost of a migration.
|
||
|
||
|
||
---
|
||
|
||
## 11. High — unauthenticated client registration, with self-chosen scopes and no pruning
|
||
|
||
`/oauth/register` is mounted unconditionally:
|
||
|
||
```go
|
||
// oauth_server.go:240
|
||
mux.HandleFunc("/oauth/register", s.registerHandler)
|
||
```
|
||
|
||
and the handler requires nothing but a POST:
|
||
|
||
```go
|
||
// oauth_server.go:406-433 (abridged)
|
||
func (s *OAuthServer) registerHandler(w http.ResponseWriter, r *http.Request) {
|
||
if r.Method != http.MethodPost {
|
||
http.Error(w, "method not allowed", http.StatusMethodNotAllowed)
|
||
return
|
||
}
|
||
var req struct {
|
||
RedirectURIs []string `json:"redirect_uris"`
|
||
ClientName string `json:"client_name"`
|
||
GrantTypes []string `json:"grant_types"`
|
||
AllowedScopes []string `json:"allowed_scopes"`
|
||
TokenEndpointAuthMethod string `json:"token_endpoint_auth_method"`
|
||
}
|
||
if err := json.NewDecoder(r.Body).Decode(&req); err != nil { ... }
|
||
if len(req.RedirectURIs) == 0 { ... }
|
||
grantTypes := req.GrantTypes
|
||
if len(grantTypes) == 0 {
|
||
grantTypes = []string{"authorization_code"}
|
||
}
|
||
allowedScopes := req.AllowedScopes
|
||
if len(allowedScopes) == 0 {
|
||
allowedScopes = s.cfg.DefaultScopes
|
||
}
|
||
```
|
||
|
||
RFC 7591 permits open registration, so this is a defensible default for an MCP
|
||
server — but three specifics make it exploitable here.
|
||
|
||
**The client declares its own scopes.** `allowedScopes := req.AllowedScopes`
|
||
(`:430`) is stored verbatim into the client record (`:467`) with no intersection
|
||
against a server-side permitted set. `s.cfg.DefaultScopes` is used only when the
|
||
request omits the field. A registration asking for
|
||
`"allowed_scopes": ["admin", "write:all"]` gets them.
|
||
|
||
**Registered clients are never removed.** The cleanup goroutine prunes only the
|
||
two short-lived maps:
|
||
|
||
```go
|
||
// oauth_server.go:252-274 (abridged)
|
||
func (s *OAuthServer) cleanupExpired() {
|
||
ticker := time.NewTicker(5 * time.Minute)
|
||
defer ticker.Stop()
|
||
for {
|
||
select {
|
||
case <-s.done:
|
||
return
|
||
case <-ticker.C:
|
||
now := time.Now()
|
||
s.mu.Lock()
|
||
for k, p := range s.pending {
|
||
if now.After(p.ExpiresAt) { delete(s.pending, k) }
|
||
}
|
||
for k, p := range s.codes {
|
||
if now.After(p.ExpiresAt) { delete(s.codes, k) }
|
||
}
|
||
s.mu.Unlock()
|
||
}
|
||
}
|
||
}
|
||
```
|
||
|
||
`s.clients` (`:136`, initialised `:189`) has no expiry, no cap and no eviction.
|
||
Each POST to `/oauth/register` adds a permanent entry holding the client name, the
|
||
`redirect_uris` slice, the grant types and the scopes — all attacker-sized. This
|
||
is an unauthenticated, unbounded memory write: the classic shape of a slow
|
||
memory-exhaustion DoS, and there is no rate limiter mounted to slow it
|
||
(`middleware.audit.md` finding 1).
|
||
|
||
**The request body is unbounded.** `json.NewDecoder(r.Body)` at `:418` with no
|
||
`http.MaxBytesReader`. `RedirectURIs`, `GrantTypes` and `AllowedScopes` are
|
||
unbounded slices of unbounded strings, so one request can allocate as much as the
|
||
client is willing to send.
|
||
|
||
Persistence is available but off by default. `PersistClients` (`:37-39`) writes the
|
||
record through `OAuthRegisterClient` (`:472-486`) and `lookupOrFetchClient`
|
||
(`:1108-1135`) reads it back, so a registration can outlive the process — but with
|
||
the default `false`, every registration is lost on restart and invisible to other
|
||
replicas, and with it set the rehydrated record is incomplete in a
|
||
security-relevant way (finding 34).
|
||
|
||
### Remediation
|
||
|
||
1. **Gate registration.** Either require an initial access token (RFC 7591 §3.1) —
|
||
```go
|
||
if s.cfg.RegistrationAccessToken != "" {
|
||
if subtle.ConstantTimeCompare([]byte(bearerFrom(r)), []byte(s.cfg.RegistrationAccessToken)) != 1 {
|
||
writeOAuthError(w, "invalid_token", "", http.StatusUnauthorized)
|
||
return
|
||
}
|
||
}
|
||
```
|
||
— or keep it open and bound it hard: a per-IP registration rate limit plus a
|
||
global cap on `len(s.clients)`, rejecting with `503` once reached.
|
||
2. **Intersect the requested scopes** against what the server is willing to grant:
|
||
```go
|
||
allowedScopes = intersectScopes(req.AllowedScopes, s.cfg.GrantableScopes)
|
||
if len(allowedScopes) == 0 { allowedScopes = s.cfg.DefaultScopes }
|
||
```
|
||
Never store a scope the server did not sanction.
|
||
3. **Expire clients.** Give `oauthClient` a `CreatedAt` and a `LastUsedAt`, and
|
||
prune in `cleanupExpired` alongside `pending` and `codes`. For clients that must
|
||
outlive the process, persist them the way codes are persisted.
|
||
4. **Bound the body and the slices:**
|
||
`r.Body = http.MaxBytesReader(w, r.Body, 32<<10)`, plus explicit limits on the
|
||
number of redirect URIs (say 8) and their length, and `dec.DisallowUnknownFields()`
|
||
so a typo'd field is an error rather than a silent default.
|
||
5. Validate each `redirect_uri` at registration: absolute, no fragment, and
|
||
`https` unless the host is `localhost`. The exact-match check at `:562` makes
|
||
registration the only place this can be enforced.
|
||
|
||
---
|
||
|
||
## 12. High — `JWTAuthenticator` issues a forgeable bearer token and verifies nothing
|
||
|
||
```go
|
||
// providers.go:687-706
|
||
// TODO: Verify password
|
||
// if err := bcrypt.CompareHashAndPassword([]byte(user.Password), []byte(req.Password)); err != nil {
|
||
// return nil, fmt.Errorf("invalid credentials")
|
||
// }
|
||
|
||
// Generate token (placeholder - implement JWT signing when library is available)
|
||
expiresAt := time.Now().Add(24 * time.Hour)
|
||
tokenString := fmt.Sprintf("token_%d_%d", user.ID, expiresAt.Unix())
|
||
|
||
return &LoginResponse{
|
||
Token: tokenString,
|
||
...
|
||
```
|
||
|
||
and identically in Direct mode:
|
||
|
||
```go
|
||
// providers_direct.go:447-451
|
||
expiresAt := time.Now().Add(24 * time.Hour)
|
||
tokenString := fmt.Sprintf("token_%d_%d", userID, expiresAt.Unix())
|
||
|
||
return &LoginResponse{
|
||
Token: tokenString,
|
||
```
|
||
|
||
The "token" is `token_<user_id>_<unix_expiry>`. It is not signed, not random, and
|
||
contains no secret — anyone can construct `token_1_1790000000` for user 1. There
|
||
is no JWT anywhere in the type despite the name; the doc comment at `:692` calls
|
||
it a placeholder.
|
||
|
||
The counterpart is unimplemented:
|
||
|
||
```go
|
||
// providers.go:741-754
|
||
func (a *JWTAuthenticator) Authenticate(r *http.Request) (*UserContext, error) {
|
||
authHeader := r.Header.Get("Authorization")
|
||
if authHeader == "" {
|
||
return nil, fmt.Errorf("authorization header required")
|
||
}
|
||
|
||
tokenString := strings.TrimPrefix(authHeader, "Bearer ")
|
||
if tokenString == authHeader {
|
||
return nil, fmt.Errorf("bearer token required")
|
||
}
|
||
|
||
// TODO: Implement JWT parsing when library is available
|
||
return nil, fmt.Errorf("JWT parsing not implemented - install github.com/golang-jwt/jwt/v5")
|
||
}
|
||
```
|
||
|
||
So this provider **cannot authenticate any request**: it fails closed, always.
|
||
That is what keeps this at High rather than Critical — a deployment wiring
|
||
`JWTAuthenticator` into `NewAuthMiddleware` returns 401 to everyone and the
|
||
problem is discovered immediately.
|
||
|
||
The exposure is the seam between the two. `Login` mints tokens that are accepted
|
||
by nothing in this package, so a deployment that wants JWT login has to pair
|
||
`JWTAuthenticator.Login` with a verifier of its own — and the tokens it is handed
|
||
carry no signature to verify. `jwtLogoutDirect` compounds the impression that the
|
||
flow is complete by inserting the token into a blacklist table
|
||
(`providers_direct.go:464-473`) that `Authenticate` never reads.
|
||
|
||
`Login` also does not verify the password (finding 1) and dereferences `a.getDB()`
|
||
at `providers.go:654` without a nil check, panicking rather than erroring when no
|
||
database is configured.
|
||
|
||
### Remediation
|
||
|
||
Pick one of two honest outcomes.
|
||
|
||
**Implement it.** Add `github.com/golang-jwt/jwt/v5`, sign with RS256 using a
|
||
configured key (the OAuth server already manages one — `oauth_server.go:140`,
|
||
`:175-197` — and exposes JWKS at `:343`, so reuse that key and key ID rather than
|
||
introducing a second):
|
||
|
||
```go
|
||
claims := jwt.MapClaims{
|
||
"sub": strconv.Itoa(user.ID), "iss": a.issuer, "aud": a.audience,
|
||
"iat": now.Unix(), "exp": now.Add(a.ttl).Unix(), "jti": newJTI(),
|
||
"roles": user.Roles,
|
||
}
|
||
tok := jwt.NewWithClaims(jwt.SigningMethodRS256, claims)
|
||
signed, err := tok.SignedString(a.signingKey)
|
||
```
|
||
|
||
and in `Authenticate` parse with an explicit algorithm allow-list
|
||
(`jwt.WithValidMethods([]string{"RS256"})`) — never trust the token's own `alg` —
|
||
validating `iss`, `aud` and `exp`, then check `jti` against the blacklist table
|
||
`jwtLogoutDirect` already writes.
|
||
|
||
**Or remove it.** If JWT is not a supported mode, delete `JWTAuthenticator`
|
||
rather than shipping a type whose `Login` hands out forgeable credentials. A
|
||
constructor that returns `nil, errors.New("JWT authentication is not
|
||
implemented")` is safer than one that returns a working-looking token.
|
||
|
||
Either way, do not leave `Login` issuing `token_<id>_<ts>`. If the type must stay
|
||
as a stub, make `Login` fail the same way `Authenticate` does.
|
||
|
||
---
|
||
|
||
## 34. High — a persisted client is rehydrated without its secret, so confidential clients become public
|
||
|
||
The token endpoint gates client authentication on whether the client has a stored
|
||
secret:
|
||
|
||
```go
|
||
// oauth_server.go:77-79
|
||
func (c *oauthClient) isConfidential() bool {
|
||
return c.ClientSecretHash != ""
|
||
}
|
||
```
|
||
|
||
```go
|
||
// oauth_server.go:805-812
|
||
// Confidential clients (those registered with a client_secret) must authenticate;
|
||
// public clients keep relying on PKCE alone, unchanged from prior behavior.
|
||
if client, ok := s.lookupOrFetchClient(r.Context(), clientID); ok && client.isConfidential() {
|
||
if _, err := s.authenticateClient(r); err != nil {
|
||
writeOAuthError(w, "invalid_client", err.Error(), http.StatusUnauthorized)
|
||
return
|
||
}
|
||
}
|
||
```
|
||
|
||
Registration stores the hash correctly — `ClientSecretHash: secretHash` at
|
||
`:468`, persisted through `OAuthRegisterClient` at `:472-486` — and the in-memory
|
||
copy in `s.clients` carries it. But the read-through path does not:
|
||
|
||
```go
|
||
// oauth_server.go:1108-1135
|
||
// lookupOrFetchClient checks in-memory first, then DB if PersistClients is enabled.
|
||
func (s *OAuthServer) lookupOrFetchClient(ctx context.Context, clientID string) (*oauthClient, bool) {
|
||
s.mu.RLock()
|
||
c, ok := s.clients[clientID]
|
||
s.mu.RUnlock()
|
||
if ok {
|
||
return c, true
|
||
}
|
||
|
||
if !s.cfg.PersistClients || s.auth == nil {
|
||
return nil, false
|
||
}
|
||
|
||
dbClient, err := s.auth.OAuthGetClient(ctx, clientID)
|
||
if err != nil {
|
||
return nil, false
|
||
}
|
||
|
||
c = &oauthClient{
|
||
ClientID: dbClient.ClientID,
|
||
RedirectURIs: dbClient.RedirectURIs,
|
||
ClientName: dbClient.ClientName,
|
||
GrantTypes: dbClient.GrantTypes,
|
||
AllowedScopes: dbClient.AllowedScopes,
|
||
}
|
||
s.mu.Lock()
|
||
s.clients[clientID] = c
|
||
s.mu.Unlock()
|
||
return c, true
|
||
}
|
||
```
|
||
|
||
`ClientSecretHash` and `TokenEndpointAuthMethod` are **not copied**, even though
|
||
`OAuthServerClient` carries both (`oauth_server_db.go:11-19`) and the registration
|
||
path writes both (`:479-480`). So the rehydrated client has an empty hash, `isConfidential()` returns
|
||
false, and the guard at `:807` skips client authentication entirely. The incomplete
|
||
record is then written back into `s.clients` (`:1134`), so the wrong answer is
|
||
cached for the remaining life of the process.
|
||
|
||
This triggers whenever the in-memory map does not already hold the client:
|
||
|
||
* **after any restart or deploy** — every confidential client registered before the
|
||
restart is treated as public from then on;
|
||
* **on every replica that did not handle the registration** — so in a
|
||
multi-instance deployment it is the normal case, not the exceptional one;
|
||
* **after the first eviction**, if the pruning recommended in finding 11 is added
|
||
without fixing this.
|
||
|
||
The practical effect is that a confidential client's `client_secret` stops being
|
||
required at the token endpoint. Two mitigations keep this at High rather than
|
||
Critical. PKCE is mandatory for every client — `code_challenge` is required at the
|
||
authorize endpoint (`:549-551`) and the verifier is checked on both the persisted
|
||
and in-memory code paths (`:831`, `:859`) — so an attacker still needs the
|
||
`code_verifier` that the legitimate client generated. And the code is bound to its
|
||
`client_id` and `redirect_uri` (`:851-857`). What is lost is the independent control
|
||
RFC 6749 §3.2.1 requires for confidential clients: possession of the secret. An
|
||
attacker who obtains a code through any channel that does not also yield the
|
||
verifier — a logged query string, a referrer leak, an open redirector on a
|
||
registered URI — no longer faces the secret as a second barrier.
|
||
|
||
There is a matching availability bug in the same defect: a *legitimate* confidential
|
||
client that presents the correct secret after a restart is rejected, because
|
||
`authenticateClient` refuses a client that is not confidential
|
||
(`:1255-1258`). It does not surface, only because `:807` never calls it.
|
||
|
||
### Remediation
|
||
|
||
Copy the whole record:
|
||
|
||
```go
|
||
c = &oauthClient{
|
||
ClientID: dbClient.ClientID,
|
||
RedirectURIs: dbClient.RedirectURIs,
|
||
ClientName: dbClient.ClientName,
|
||
GrantTypes: dbClient.GrantTypes,
|
||
AllowedScopes: dbClient.AllowedScopes,
|
||
ClientSecretHash: dbClient.ClientSecretHash,
|
||
TokenEndpointAuthMethod: dbClient.TokenEndpointAuthMethod,
|
||
}
|
||
```
|
||
|
||
Then make the omission structurally impossible and the gate fail closed:
|
||
|
||
1. **Convert, don't hand-copy.** Give `OAuthServerClient` a
|
||
`func (c *OAuthServerClient) toOAuthClient() *oauthClient` used by both the
|
||
registration and the fetch path, so a field added to one is added to both. Field
|
||
lists duplicated across a persistence boundary drift; this one already has.
|
||
2. **Do not infer confidentiality from a field that can be silently empty.**
|
||
Persist `TokenEndpointAuthMethod` and decide from it:
|
||
```go
|
||
func (c *oauthClient) isConfidential() bool {
|
||
return c.TokenEndpointAuthMethod != "" && c.TokenEndpointAuthMethod != "none"
|
||
}
|
||
```
|
||
and refuse to serve a client whose method requires a secret but whose hash is
|
||
empty, rather than downgrading it:
|
||
```go
|
||
if c.isConfidential() && c.ClientSecretHash == "" {
|
||
logger.Error("client %s loaded without a secret hash; refusing", c.ClientID)
|
||
return nil, false
|
||
}
|
||
```
|
||
3. **Add the regression test that would have caught it:** register a confidential
|
||
client with `PersistClients` enabled, construct a *new* `OAuthServer` over the
|
||
same authenticator, and assert that a code exchange without
|
||
`client_secret` is rejected with `invalid_client`. The existing OAuth tests all
|
||
run against a single in-process server, which is why the hydration path is
|
||
untested.
|
||
|
||
Fix this together with finding 11: registration and rehydration are the two halves
|
||
of one record's lifecycle, and finding 11's pruning recommendation makes this path
|
||
hot rather than restart-only.
|
||
|
||
---
|
||
|
||
## 13. Medium — the refresh-token grant authenticates no client
|
||
|
||
```go
|
||
// oauth_server.go:871-900 (abridged)
|
||
func (s *OAuthServer) handleRefreshGrant(w http.ResponseWriter, r *http.Request) {
|
||
refreshToken := r.FormValue("refresh_token")
|
||
providerName := r.FormValue("provider")
|
||
clientID := r.FormValue("client_id")
|
||
if refreshToken == "" {
|
||
writeOAuthError(w, "invalid_request", "refresh_token required", http.StatusBadRequest)
|
||
return
|
||
}
|
||
|
||
// Try external providers first, then fall back to DatabaseAuthenticator
|
||
provider := s.providerByName(providerName)
|
||
if provider != nil {
|
||
loginResp, err := provider.auth.OAuth2RefreshToken(r.Context(), refreshToken, providerName)
|
||
...
|
||
s.writeOAuthToken(w, r, loginResp.Token, loginResp.RefreshToken, clientID, nil, false)
|
||
return
|
||
}
|
||
|
||
if s.auth != nil {
|
||
loginResp, err := s.auth.RefreshToken(r.Context(), refreshToken)
|
||
...
|
||
```
|
||
|
||
Compare the authorization-code grant, which does authenticate confidential
|
||
clients:
|
||
|
||
```go
|
||
// oauth_server.go:805-812
|
||
// Confidential clients (those registered with a client_secret) must authenticate;
|
||
// public clients keep relying on PKCE alone, unchanged from prior behavior.
|
||
if client, ok := s.lookupOrFetchClient(r.Context(), clientID); ok && client.isConfidential() {
|
||
if _, err := s.authenticateClient(r); err != nil {
|
||
writeOAuthError(w, "invalid_client", err.Error(), http.StatusUnauthorized)
|
||
return
|
||
}
|
||
}
|
||
```
|
||
|
||
The refresh path has no equivalent. `clientID` is read from the form and used only
|
||
as a pass-through to `writeOAuthToken`; it is never looked up, never authenticated,
|
||
and never compared against the client the refresh token was issued to. RFC 6749
|
||
§6 requires client authentication for confidential clients on refresh, and §10.4
|
||
requires the refresh token be bound to the client it was issued to.
|
||
|
||
Consequence: a refresh token that leaks — from a log, a referrer, a compromised
|
||
public client — can be redeemed by anyone, including as though it belonged to a
|
||
different (confidential) client, yielding a fresh access token and a fresh refresh
|
||
token. There is also no rotation check: `writeOAuthToken` is handed whatever the
|
||
underlying authenticator returns, so a replayed refresh token is not detected.
|
||
|
||
Two smaller issues in the same function: `providerName` comes from the form and
|
||
selects which external provider handles the token (`:881`), so a caller chooses
|
||
the validation path for a credential; and `err.Error()` from the provider is
|
||
echoed to the client at `:885` and `:895` (finding 18).
|
||
|
||
### Remediation
|
||
|
||
```go
|
||
func (s *OAuthServer) handleRefreshGrant(w http.ResponseWriter, r *http.Request) {
|
||
refreshToken := r.FormValue("refresh_token")
|
||
clientID := r.FormValue("client_id")
|
||
if refreshToken == "" || clientID == "" {
|
||
writeOAuthError(w, "invalid_request", "refresh_token and client_id required", http.StatusBadRequest)
|
||
return
|
||
}
|
||
|
||
client, ok := s.lookupOrFetchClient(r.Context(), clientID)
|
||
if !ok {
|
||
writeOAuthError(w, "invalid_client", "", http.StatusUnauthorized)
|
||
return
|
||
}
|
||
if client.isConfidential() {
|
||
if _, err := s.authenticateClient(r); err != nil {
|
||
writeOAuthError(w, "invalid_client", "", http.StatusUnauthorized)
|
||
return
|
||
}
|
||
}
|
||
// … and verify the stored refresh token's client_id == clientID before use
|
||
```
|
||
|
||
That last line needs storage support: the refresh token must record its issuing
|
||
client. `OAuthExchangeCode` already persists `ClientID` alongside a code
|
||
(`:824`), so the same shape applies.
|
||
|
||
Also rotate on use — issue a new refresh token, invalidate the old one, and treat
|
||
reuse of an invalidated token as a compromise signal (RFC 6749 §10.4 / OAuth 2.1
|
||
§6.1). And bind the provider to the token rather than to a form field: store
|
||
`ProviderName` with the token and ignore `r.FormValue("provider")`.
|
||
|
||
---
|
||
|
||
## 14. Medium — both security maps grow without bound, keyed on tokens and claims
|
||
|
||
```go
|
||
// provider.go:54-61
|
||
type SecurityList struct {
|
||
provider SecurityProvider
|
||
|
||
ColumnSecurityMutex sync.RWMutex
|
||
ColumnSecurity map[string][]ColumnSecurity
|
||
RowSecurityMutex sync.RWMutex
|
||
RowSecurity map[string]RowSecurity
|
||
}
|
||
```
|
||
|
||
Entries are inserted at `provider.go:393` and `:438` and **never removed**. There
|
||
is no TTL, no size cap and no eviction; `ClearSecurity` (`:397-417`) is the only
|
||
delete path and it has zero callers (finding 29) and does not touch `RowSecurity`
|
||
at all.
|
||
|
||
Growth is driven by the key. Column security keys on the integer user ID:
|
||
|
||
```go
|
||
// provider.go:381
|
||
secKey := fmt.Sprintf("%s.%s@%d", pSchema, pTablename, pUserID)
|
||
```
|
||
|
||
so that map is bounded by `users × tables` — large but finite. Row security keys
|
||
on the opaque ref:
|
||
|
||
```go
|
||
// provider.go:430
|
||
secKey := fmt.Sprintf("%s.%s@%v", pSchema, pTablename, pUserRef)
|
||
```
|
||
|
||
and `pUserRef` is the whole `*UserContext` for every spec adapter
|
||
(`restheadspec/security_hooks.go:83-89`). `%v` on a pointer-to-struct renders
|
||
every field, so the key is a string like
|
||
|
||
```
|
||
public.orders@&{1 alice 0 sess_9f3c… 42 10.0.0.7 [user] alice@example.com map[email:… exp:1.76e+09 sub:…] map[] false 0 }
|
||
```
|
||
|
||
Three consequences:
|
||
|
||
* **Unbounded cardinality when claims vary.** Any field that changes per session
|
||
mints a new permanent entry. `SessionID` alone guarantees this: a user who logs
|
||
in twice has two keys; a user who logs in daily for a year has 365. With
|
||
external OIDC providers, `Claims` is the entire userinfo map
|
||
(`oauth2_methods.go:356`), so a refreshed `exp` or `iat` does the same. Map
|
||
iteration order does not matter here — `fmt` prints map keys sorted since Go
|
||
1.12 — but the values themselves vary, and that is enough.
|
||
* **Credentials held in memory indefinitely.** Each key string contains a session
|
||
token and the user's email. They persist for the process lifetime, survive
|
||
logout, and appear in any heap dump or core file.
|
||
* **Nothing is ever read from these maps anyway** — finding 6 shows both loaders
|
||
query the provider unconditionally, so this is pure accumulation with no
|
||
benefit.
|
||
|
||
### Remediation
|
||
|
||
The key and the lifetime both need fixing.
|
||
|
||
1. **Key on a stable scalar.** Row security needs a *user identity*, not a
|
||
snapshot of the request. Derive it explicitly:
|
||
```go
|
||
func securityKey(schema, table string, ref any) string {
|
||
switch v := ref.(type) {
|
||
case *UserContext:
|
||
return fmt.Sprintf("%s.%s@%d", schema, table, v.UserID) // or v.Claims["sub"]
|
||
case int:
|
||
return fmt.Sprintf("%s.%s@%d", schema, table, v)
|
||
case string:
|
||
return fmt.Sprintf("%s.%s@%s", schema, table, v)
|
||
default:
|
||
return fmt.Sprintf("%s.%s@%v", schema, table, v)
|
||
}
|
||
}
|
||
```
|
||
For deployments with non-integer identities, read the subject claim — that is
|
||
what `GetUserRef`'s doc comment (`hooks.go:17-21`) intends. A redacting
|
||
`String()` on `UserContext` (finding 7) also fixes the default branch.
|
||
2. **Bound the maps.** Once finding 6 introduces real caching, the entries need a
|
||
TTL and a cap. `pkg/cache` provides both (`MaxSize` with LRU eviction,
|
||
`cache/provider_memory.go:105-109`, `:307-326`) plus tag-based invalidation, which
|
||
would let a rule change invalidate `table:orders` across all users at once.
|
||
3. **Extend `ClearSecurity` to `RowSecurity`** and call it on logout and on rule
|
||
changes, or delete it (finding 29) once a TTL makes it redundant.
|
||
|
||
---
|
||
|
||
## 15. Medium — model rules fail open, and any authenticated user may do anything
|
||
|
||
Two layers of authorisation both default to "allow".
|
||
|
||
**Unregistered models allow writes.** `checkModelUpdateAllowed` and
|
||
`checkModelDeleteAllowed` are the `BeforeUpdate`/`BeforeDelete` hooks
|
||
(`restheadspec/security_hooks.go:49-58`):
|
||
|
||
```go
|
||
// hooks.go:274-294
|
||
func checkModelUpdateAllowed(secCtx SecurityContext) error {
|
||
rules, ok := GetModelRulesFromContext(secCtx.GetContext())
|
||
if !ok {
|
||
schema := secCtx.GetSchema()
|
||
entity := secCtx.GetEntity()
|
||
var err error
|
||
if schema != "" {
|
||
rules, err = modelregistry.GetModelRulesByName(fmt.Sprintf("%s.%s", schema, entity))
|
||
}
|
||
if err != nil || schema == "" {
|
||
rules, err = modelregistry.GetModelRulesByName(entity)
|
||
}
|
||
if err != nil {
|
||
return nil // model not registered, allow by default
|
||
}
|
||
}
|
||
if !rules.CanUpdate {
|
||
return fmt.Errorf("update not allowed for %s", secCtx.GetEntity())
|
||
}
|
||
return nil
|
||
}
|
||
```
|
||
|
||
`checkModelDeleteAllowed` is identical (`:298-318`, fail-open at `:311`). A model
|
||
served by the spec handler but absent from the registry — or present under a name
|
||
the two lookups do not produce — is fully writable. This is the consuming side of
|
||
`modelregistry.audit.md` finding 1 (*registry side fixed 2026-09-30: `checkModelUpdateAllowed`/`checkModelDeleteAllowed` now allow only on `ErrModelNotFound`*): the registry's lookup failure and this
|
||
`return nil` combine into "unknown model ⇒ permitted".
|
||
|
||
**Any authenticated user may perform any operation.** `CheckModelAuthAllowed` is
|
||
the `BeforeHandle` hook (`restheadspec/security_hooks.go:14-22`), and its final
|
||
step is:
|
||
|
||
```go
|
||
// hooks.go:356-365
|
||
if operation == "delete" && rules.CanPublicDelete {
|
||
return nil
|
||
}
|
||
|
||
userID, _ := secCtx.GetUserID()
|
||
if userID == 0 {
|
||
return fmt.Errorf("authentication required")
|
||
}
|
||
return nil
|
||
}
|
||
```
|
||
|
||
The only distinction it draws is authenticated vs. guest. `UserContext.Roles` and
|
||
`UserLevel` are populated by every authenticator and consulted nowhere in this
|
||
function. The doc comment is candid about it — *"8. Authenticated user → allow
|
||
(operation-specific checks remain in BeforeUpdate/BeforeDelete)"* (`:332`) — but
|
||
`BeforeUpdate`/`BeforeDelete` check only the model's global `CanUpdate`/`CanDelete`
|
||
flags, which are per-model, not per-role. So `rules.CanUpdate == true` means
|
||
*every* logged-in user may update *every* row, subject only to row security —
|
||
which finding 2 shows is inert. Read and create have no per-operation check at all.
|
||
|
||
Note also that `userID == 0` is the guest marker (`middleware.go:46`), so a real
|
||
user whose ID is genuinely 0 is treated as unauthenticated, and conversely any
|
||
authenticator that fails to populate `UserIDKey` yields `userID == 0` from the
|
||
ignored-error `GetUserID` at `:360` and is correctly denied — the one place the
|
||
fail-closed direction holds.
|
||
|
||
### Remediation
|
||
|
||
1. **Fail closed on unknown models.** An operation on a model with no rules should
|
||
be refused, not permitted:
|
||
```go
|
||
rules, ok := resolveModelRules(secCtx)
|
||
if !ok {
|
||
return fmt.Errorf("no security rules registered for %s.%s", secCtx.GetSchema(), secCtx.GetEntity())
|
||
}
|
||
```
|
||
If that is too strict for existing deployments, make it configurable and
|
||
default to closed — with a startup log listing every served model that has no
|
||
rules, so the gap is visible at deploy time rather than at exploit time.
|
||
`resolveModelRules` (`:369-390`) already centralises the lookup, so the change
|
||
is one place plus the two write checks.
|
||
2. **Add role checks.** `ModelRules` needs per-operation role requirements —
|
||
`ReadRoles`, `CreateRoles`, `UpdateRoles`, `DeleteRoles` — and
|
||
`CheckModelAuthAllowed` should require intersection with `userCtx.Roles`:
|
||
```go
|
||
required := rules.RolesFor(operation)
|
||
if len(required) > 0 && !hasAnyRole(userCtx.Roles, required) {
|
||
return fmt.Errorf("operation %s on %s requires one of %v", operation, entity, required)
|
||
}
|
||
```
|
||
Until that exists, the package's authorisation model is "logged in ⇒ trusted",
|
||
and row security is the only tenant boundary — which makes finding 2 the whole
|
||
of the access-control story.
|
||
3. `GetModelRulesFromContext` is populated by `NewModelAuthMiddleware`
|
||
(`middleware.go:225`), which is per-model. Verify that every route that reaches
|
||
these hooks goes through it; where it does not, the fallback registry lookup at
|
||
`:277-288` is the only path and its two name forms must both be correct.
|
||
|
||
---
|
||
|
||
## 16. Medium — there is no audit log
|
||
|
||
`logDataAccess` is registered as an `AfterRead` hook by every spec
|
||
(`restheadspec/security_hooks.go:43-46`) and is this:
|
||
|
||
```go
|
||
// hooks.go:198-218
|
||
func logDataAccess(secCtx SecurityContext) error {
|
||
userID, _ := secCtx.GetUserID()
|
||
|
||
logger.Info("AUDIT: User %d accessed %s.%s",
|
||
userID,
|
||
secCtx.GetSchema(),
|
||
secCtx.GetEntity(),
|
||
)
|
||
|
||
// TODO: Write to audit log table or external audit service
|
||
// auditLog := AuditLog{
|
||
// UserID: userID,
|
||
// Schema: secCtx.GetSchema(),
|
||
// Entity: secCtx.GetEntity(),
|
||
// Action: "READ",
|
||
// Timestamp: time.Now(),
|
||
// }
|
||
// db.Create(&auditLog)
|
||
|
||
return nil
|
||
}
|
||
```
|
||
|
||
What this is: a log line. What it is not, and what a control named `AUDIT` implies:
|
||
durable, queryable, tamper-evident, and complete. Specifically —
|
||
|
||
* It records reads only. There is no equivalent hook on create, update or delete,
|
||
so the operations that change data are not audited at all.
|
||
* It records no row identity, no filter, no column list and no result count, so it
|
||
cannot answer "which records did this user see?" — the question an audit log
|
||
exists to answer.
|
||
* It goes to the application log, which rotates and is not tamper-evident.
|
||
* `Info` is not forwarded to the error tracker (`logger/logger.go:100-106`), so
|
||
it exists in exactly one place, on the host.
|
||
* It fires on the read path of every request, so it doubles log volume for no
|
||
retrievable benefit.
|
||
|
||
For any deployment with a compliance obligation (SOC 2 CC7, PCI DSS 10, HIPAA
|
||
§164.312(b)), this does not satisfy it, and the `AUDIT:` prefix makes it look as
|
||
though it might.
|
||
|
||
### Remediation
|
||
|
||
Either build it or stop calling it an audit log.
|
||
|
||
To build it: write to a dedicated append-only table through the same
|
||
`SecurityProvider` abstraction, and cover writes as well as reads:
|
||
|
||
```go
|
||
type AuditEvent struct {
|
||
At time.Time
|
||
UserID int
|
||
SessionID string // hash, not the token — see finding 7
|
||
Operation string // read | create | update | delete
|
||
Schema string
|
||
Entity string
|
||
RecordIDs []string // primary keys touched
|
||
Filters string // normalised, no values
|
||
RowCount int
|
||
RequestID string
|
||
RemoteAddr string
|
||
Outcome string // allowed | denied
|
||
}
|
||
|
||
type AuditSink interface { Record(ctx context.Context, e AuditEvent) error }
|
||
```
|
||
|
||
Register it on `AfterRead`, `AfterCreate`, `AfterUpdate`, `AfterDelete` **and** on
|
||
the deny paths (`hooks.go:94`, `:291`, `:315`, `:339`, `:362`) — a denied attempt
|
||
is the event most worth recording. Write asynchronously through a bounded queue so
|
||
the sink cannot become the throughput ceiling the way finding 6's mutexes are, and
|
||
decide explicitly whether a full queue drops events or fails the request; for a
|
||
compliance audit log it must be the latter.
|
||
|
||
If building it is out of scope, rename the function and drop the `AUDIT:` prefix so
|
||
nothing reads as a control that is not one, and say plainly in
|
||
`SECURITY_FEATURES.md` that audit logging is the integrator's responsibility.
|
||
|
||
---
|
||
|
||
## 17. Medium — identifier validation exists but is never called
|
||
|
||
The package interpolates configurable names into SQL in dozens of places:
|
||
|
||
```go
|
||
// providers.go:413
|
||
query := fmt.Sprintf(`SELECT p_success, p_error, p_user::text FROM %s($1, $2)`, a.sqlNames.Session)
|
||
```
|
||
|
||
```go
|
||
// providers_direct.go:216-220
|
||
query := rewritePlaceholders(db, fmt.Sprintf(
|
||
`SELECT s.user_id, u.username, u.email, u.user_level, u.roles, u.program_user_id, u.program_user_table
|
||
FROM %s s JOIN %s u ON s.user_id = u.id
|
||
WHERE s.session_token = ? AND s.expires_at > ? AND u.is_active = ?`,
|
||
a.tableNames.UserSessions, a.tableNames.Users))
|
||
```
|
||
|
||
and validators for exactly this exist:
|
||
|
||
```go
|
||
// sql_names.go:9
|
||
var validSQLIdentifier = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_]*$`)
|
||
```
|
||
|
||
```go
|
||
// sql_names.go:242-258
|
||
// ValidateSQLNames checks that all non-empty fields in names are valid SQL identifiers.
|
||
// Returns an error if any field contains invalid characters.
|
||
func ValidateSQLNames(names *SQLNames) error {
|
||
v := reflect.ValueOf(names).Elem()
|
||
...
|
||
if val != "" && !validSQLIdentifier.MatchString(val) {
|
||
return fmt.Errorf("SQLNames.%s contains invalid characters: %q", typ.Field(i).Name, val)
|
||
}
|
||
```
|
||
|
||
Grepping the whole repository for `ValidateSQLNames` and `ValidateTableNames`
|
||
returns their own definitions and their tests — **no production caller**. The
|
||
resolvers that every constructor goes through skip them:
|
||
|
||
```go
|
||
// sql_names.go:262-267
|
||
func resolveSQLNames(override ...*SQLNames) *SQLNames {
|
||
if len(override) > 0 && override[0] != nil {
|
||
return MergeSQLNames(DefaultSQLNames(), override[0])
|
||
}
|
||
return DefaultSQLNames()
|
||
}
|
||
```
|
||
|
||
```go
|
||
// table_names.go:99-101
|
||
func resolveTableNames(override *TableNames) *TableNames {
|
||
return MergeTableNames(DefaultTableNames(), override)
|
||
}
|
||
```
|
||
|
||
So a `SQLNames`/`TableNames` override flows unvalidated into `fmt.Sprintf`. These
|
||
values come from application configuration, not from a request, which caps the
|
||
severity — this is not remote SQL injection. But it is injection-by-configuration:
|
||
a name assembled from an environment variable or a tenant identifier becomes SQL,
|
||
and the guard written to prevent it is inert. This is another instance of
|
||
`_CROSS-CUTTING` finding X10 — a control that is implemented, tested, and never
|
||
installed.
|
||
|
||
Note also that the regex rejects schema-qualified names: `public.users` fails
|
||
`^[a-zA-Z_][a-zA-Z0-9_]*$`. Since validation never runs, deployments using
|
||
`"myschema.users"` work today and would break the moment it is wired in — so the
|
||
fix needs the regex widened (or a per-part check) at the same time.
|
||
|
||
### Remediation
|
||
|
||
Call the validators where the names enter, and fail at construction rather than at
|
||
query time:
|
||
|
||
```go
|
||
func resolveSQLNames(override ...*SQLNames) (*SQLNames, error) {
|
||
n := DefaultSQLNames()
|
||
if len(override) > 0 && override[0] != nil {
|
||
n = MergeSQLNames(n, override[0])
|
||
}
|
||
if err := ValidateSQLNames(n); err != nil {
|
||
return nil, err
|
||
}
|
||
return n, nil
|
||
}
|
||
```
|
||
|
||
That changes the signature of the constructors that call it
|
||
(`NewDatabaseAuthenticatorWithOptions`, `NewDatabaseColumnSecurityProvider` at
|
||
`providers.go:771`, and the row-security and keystore equivalents). Several
|
||
already return only a value; those become `(T, error)`. Rejecting a bad
|
||
configuration at startup is worth the churn.
|
||
|
||
Widen the identifier rule to allow an optional schema qualifier, validating each
|
||
part:
|
||
|
||
```go
|
||
var validSQLIdentifierPart = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_]*$`)
|
||
|
||
func validIdentifier(s string) bool {
|
||
parts := strings.Split(s, ".")
|
||
if len(parts) > 2 { return false }
|
||
for _, p := range parts {
|
||
if !validSQLIdentifierPart.MatchString(p) { return false }
|
||
}
|
||
return true
|
||
}
|
||
```
|
||
|
||
and quote on interpolation (`pq.QuoteIdentifier` or the dialect equivalent) so the
|
||
regex is defence in depth rather than the only line.
|
||
|
||
---
|
||
|
||
## 18. Medium — internal error text is returned to unauthenticated clients
|
||
|
||
```go
|
||
// middleware.go:82-91
|
||
func authenticateRequest(w http.ResponseWriter, r *http.Request, provider SecurityProvider) (*http.Request, bool) {
|
||
// Call the provider's Authenticate method
|
||
userCtx, err := provider.Authenticate(r)
|
||
if err != nil {
|
||
http.Error(w, "Authentication failed: "+err.Error(), http.StatusUnauthorized)
|
||
return nil, false
|
||
}
|
||
```
|
||
|
||
The same pattern at `middleware.go:203` (`NewAuthMiddleware`) and `:266`
|
||
(`NewModelAuthMiddleware`), and in the OAuth server at `oauth_server.go:809`,
|
||
`:885` and `:895`.
|
||
|
||
`err` is whatever the provider produced, and the providers wrap freely:
|
||
|
||
* `"session query failed: %w"` (`providers.go:417`) — carries the driver's error,
|
||
so a malformed query or a missing procedure returns the PostgreSQL message,
|
||
including the procedure name and sometimes the SQL.
|
||
* `"%s", errorMsg.String` (`providers.go:422`) — the stored procedure's own text,
|
||
verbatim, to the client.
|
||
* `"failed to parse user context: %w"` (`:434`) — a JSON error quoting the
|
||
offending input, which is the session payload.
|
||
* `"invalid user ID: %w"` (`:53`) — `strconv`'s message including the value.
|
||
|
||
An unauthenticated attacker learns the database dialect, the procedure naming
|
||
scheme, whether a given procedure exists, and whether a failure was "no such
|
||
session" versus "database unreachable" — a clean oracle for probing
|
||
`SQLNames`/`TableNames` configuration and for distinguishing valid from invalid
|
||
tokens.
|
||
|
||
### Remediation
|
||
|
||
Separate what the client is told from what is recorded:
|
||
|
||
```go
|
||
func authenticateRequest(w http.ResponseWriter, r *http.Request, provider SecurityProvider) (*http.Request, bool) {
|
||
userCtx, err := provider.Authenticate(r)
|
||
if err != nil {
|
||
logger.Debug("authentication failed for %s %s: %v", r.Method, r.URL.Path, err)
|
||
http.Error(w, "Unauthorized", http.StatusUnauthorized)
|
||
return nil, false
|
||
}
|
||
if userCtx == nil {
|
||
logger.Error("authenticator returned nil user context with nil error")
|
||
http.Error(w, "Internal Server Error", http.StatusInternalServerError)
|
||
return nil, false
|
||
}
|
||
return setUserContext(r, userCtx), true
|
||
}
|
||
```
|
||
|
||
`Debug` rather than `Warn` on the log side is deliberate: failed authentication is
|
||
attacker-triggerable, so at `Warn` it becomes a Sentry flood (X8). Attach a
|
||
request ID to the log line and return it in the response if operators need to
|
||
correlate a specific 401.
|
||
|
||
Apply the same shape at `middleware.go:203`, `:266`, and to the OAuth handlers —
|
||
`writeOAuthError` should send the RFC 6749 error code (`invalid_client`,
|
||
`invalid_grant`) with an empty or fixed `error_description`, never `err.Error()`.
|
||
`oauth_server.go:825` and `:852` already do exactly that (`writeOAuthError(w,
|
||
"invalid_client", "", …)`), so the correct pattern is already in the file; `:809`,
|
||
`:885` and `:895` are the outliers.
|
||
|
||
The `userCtx == nil` guard also closes finding 32.
|
||
|
||
---
|
||
|
||
## 19. Medium — a panicking driver silently downgrades the authenticator to Direct mode
|
||
|
||
```go
|
||
// query_mode.go:53-72
|
||
// probeFunctionExists checks, via a Postgres-specific system catalog query,
|
||
// whether a function named procName exists. Any error (wrong dialect,
|
||
// placeholder syntax rejected, relation missing, etc.) is treated as "does
|
||
// not exist" rather than propagated - the probe must never be able to panic
|
||
// or block resolution of the query mode.
|
||
func probeFunctionExists(ctx context.Context, db *sql.DB, procName string) bool {
|
||
if db == nil {
|
||
return false
|
||
}
|
||
var exists bool
|
||
defer func() {
|
||
// Guard against any unexpected panic from a misbehaving driver.
|
||
_ = recover()
|
||
}()
|
||
row := db.QueryRowContext(ctx, `SELECT EXISTS (SELECT 1 FROM pg_proc WHERE proname = $1 LIMIT 1)`, procName)
|
||
if err := row.Scan(&exists); err != nil {
|
||
return false
|
||
}
|
||
return exists
|
||
}
|
||
```
|
||
|
||
The result is unnamed, so the recovered panic makes the function return `false` —
|
||
and `false` is not a neutral value here. It means "the stored procedure does not
|
||
exist", which makes `ShouldUseProcedure` select **Direct mode**
|
||
(`providers.go:404`, `:483`, `:211`, `:250`, and every other call site). Direct
|
||
mode is the code path documented as never verifying passwords
|
||
(`providers_direct.go:20-23`).
|
||
|
||
So a transient fault in the driver silently switches the whole authenticator from
|
||
the stored-procedure implementation to the one with weaker behaviour, with no log
|
||
line at all — `_ = recover()` discards the value. The same applies to the
|
||
ordinary error path at `:68`: a permissions error reading `pg_proc`, or a
|
||
connection blip during the probe, is indistinguishable from "procedure absent".
|
||
|
||
The result is cached in a `sync.Map` (`query_mode.go:36`) and only cleared by
|
||
`reset()` (`:46-51`), so one bad probe pins the downgraded mode until a reconnect.
|
||
|
||
The intent — "the probe must never be able to panic or block resolution" — is
|
||
sound. The problem is that the fallback direction is the less safe one, and the
|
||
event is invisible.
|
||
|
||
### Remediation
|
||
|
||
1. **Log it.** A discarded panic in security-relevant code is never acceptable:
|
||
```go
|
||
defer func() {
|
||
if r := recover(); r != nil {
|
||
logger.Error("probeFunctionExists(%s) panicked: %v", procName, r)
|
||
}
|
||
}()
|
||
```
|
||
Same for the `Scan` error at `:68` — at `Debug` if it is expected on non-Postgres
|
||
dialects, but not silent.
|
||
2. **Do not cache a failed probe.** Distinguish the three outcomes rather than
|
||
collapsing them into a bool:
|
||
```go
|
||
type probeResult int
|
||
const (probeAbsent probeResult = iota; probePresent; probeUnknown)
|
||
```
|
||
Cache `probeAbsent`/`probePresent`; retry on `probeUnknown`.
|
||
3. **Make the mode explicit in production.** `ModeAuto` is convenient for tests and
|
||
ambiguous in deployment. Recommend — and document — setting
|
||
`QueryMode: ModeProcedure` explicitly where the procedures are installed, so a
|
||
probe failure produces an error instead of a silent behaviour change.
|
||
4. Log the resolved mode once at startup, per procedure name. "Using Direct mode
|
||
for resolvespec_login" is exactly the line an operator needs and it does not
|
||
exist today.
|
||
|
||
Most of this becomes moot once finding 1 is fixed and both modes verify passwords
|
||
— which is the real answer: the two modes should not differ in their security
|
||
properties, so that choosing between them cannot be a security event.
|
||
|
||
---
|
||
|
||
## 20. Medium — a fire-and-forget goroutine uses the request's context
|
||
|
||
```go
|
||
// providers.go:445-449
|
||
// Update last activity asynchronously (don't block the request)
|
||
if userCtx != nil {
|
||
go a.updateSessionActivity(r.Context(), token)
|
||
}
|
||
return userCtx, nil
|
||
```
|
||
|
||
Two problems in one line.
|
||
|
||
**The context dies with the request.** `r.Context()` is cancelled when the handler
|
||
returns, which is typically before the goroutine gets scheduled. So
|
||
`updateSessionActivity`'s `ExecContext` (`:482-502`) is racing the response: some
|
||
fraction of these writes fail with `context canceled`, and which fraction depends
|
||
on load. The intent is clearly the opposite of what the code does — the comment
|
||
says "don't block the request", but detaching the work from the request also
|
||
detaches it from the request's lifetime. `context.WithoutCancel(r.Context())` (Go
|
||
1.21+) or `context.Background()` with an explicit timeout is what is wanted:
|
||
|
||
```go
|
||
if userCtx != nil {
|
||
ctx, cancel := context.WithTimeout(context.WithoutCancel(r.Context()), 5*time.Second)
|
||
go func() { defer cancel(); a.updateSessionActivity(ctx, token) }()
|
||
}
|
||
```
|
||
|
||
**The goroutine is unbounded and untracked.** One per authenticated request, with
|
||
no semaphore, no wait group, and no panic recovery. `updateSessionActivity`
|
||
performs a DB write, so under load this spawns an unbounded number of concurrent
|
||
writers against the connection pool — the pool bounds the concurrency, but the
|
||
goroutines queue behind it without limit, and each holds a token string alive.
|
||
Nothing waits for them at shutdown, so in-flight activity updates are lost on a
|
||
restart; and any panic inside the goroutine (for example from a nil `a.getDB()`)
|
||
takes the whole process down, because a panic in a goroutine cannot be recovered
|
||
by the handler.
|
||
|
||
The pattern is worth fixing structurally, not per-call-site: session activity is a
|
||
write-heavy, low-value, idempotent update — exactly the workload for coalescing.
|
||
|
||
### Remediation
|
||
|
||
1. Use a detached context with a timeout, as above.
|
||
2. Recover inside the goroutine: `defer logger.CatchPanic("updateSessionActivity")()`
|
||
— this is one of the few places `CatchPanic` is the right helper, because the
|
||
goroutine has no results and no caller to mislead (contrast finding 3).
|
||
3. Better: coalesce. Keep a small in-memory map of `token → lastSeen`, updated
|
||
under a mutex by the request path, and have **one** background goroutine flush
|
||
it every 30 seconds in a single batched `UPDATE`. That turns N writes per
|
||
second into one, bounds the goroutine count at 1, and makes the shutdown story
|
||
trivial (flush once in `Close`). It also removes the write from the
|
||
authentication hot path entirely.
|
||
4. Whichever route, register the worker with the server's shutdown sequence so the
|
||
final flush happens before the process exits — see `server.audit.md` on the
|
||
shutdown budget.
|
||
|
||
---
|
||
|
||
## 21. Medium — `cleanupStates` leaks one goroutine per OAuth2 configuration
|
||
|
||
```go
|
||
// oauth2_methods.go:74-86 (abridged)
|
||
func (a *DatabaseAuthenticator) ConfigureOAuth2Provider(name string, cfg OAuth2Config) {
|
||
a.oauth2Mutex.Lock()
|
||
defer a.oauth2Mutex.Unlock()
|
||
if a.oauth2Configs == nil {
|
||
a.oauth2Configs = make(map[string]OAuth2Config)
|
||
}
|
||
a.oauth2Configs[name] = cfg
|
||
|
||
if !a.oauth2CleanupStarted {
|
||
a.oauth2CleanupStarted = true
|
||
go a.cleanupStates()
|
||
}
|
||
}
|
||
```
|
||
|
||
and the worker:
|
||
|
||
```go
|
||
// oauth2_methods.go:337-351
|
||
func (a *DatabaseAuthenticator) cleanupStates() {
|
||
ticker := time.NewTicker(5 * time.Minute)
|
||
defer ticker.Stop()
|
||
for range ticker.C {
|
||
a.oauth2Mutex.Lock()
|
||
for state, entry := range a.oauth2States {
|
||
if time.Now().After(entry.ExpiresAt) {
|
||
delete(a.oauth2States, state)
|
||
}
|
||
}
|
||
a.oauth2Mutex.Unlock()
|
||
}
|
||
}
|
||
```
|
||
|
||
`for range ticker.C` with no `ctx.Done()` case and no stop channel: the goroutine
|
||
runs until the process exits. The `oauth2CleanupStarted` flag correctly prevents
|
||
more than one per `DatabaseAuthenticator`, so this is not a per-request leak — but
|
||
there is no `Close` on `DatabaseAuthenticator` at all, so every authenticator ever
|
||
constructed keeps a live goroutine and keeps its entire `oauth2States` map, its
|
||
configs (including **client secrets**) and its DB handle reachable. Tests that
|
||
construct authenticators in a loop accumulate them; a long-running process that
|
||
rebuilds its authenticator on configuration reload accumulates them too.
|
||
|
||
Contrast `OAuthServer`, which does this correctly: a `done` channel created in the
|
||
constructor (`oauth_server.go:193`), selected on in the worker (`:256-257`), and
|
||
closed by an idempotent `Close` (`:204-211`).
|
||
|
||
A related overwrite hazard sits two lines up: `a.oauth2Configs[name] = cfg` at
|
||
`:80` replaces an existing provider configuration silently. Since `providerName`
|
||
is chosen by the client on the refresh path (finding 13), a mis-keyed
|
||
reconfiguration silently redirects token validation to a different provider.
|
||
|
||
### Remediation
|
||
|
||
Give `DatabaseAuthenticator` a lifecycle, mirroring `OAuthServer`:
|
||
|
||
```go
|
||
type DatabaseAuthenticator struct {
|
||
...
|
||
stopCh chan struct{}
|
||
wg sync.WaitGroup
|
||
}
|
||
|
||
func (a *DatabaseAuthenticator) startCleanup() {
|
||
a.wg.Add(1)
|
||
go func() {
|
||
defer a.wg.Done()
|
||
ticker := time.NewTicker(5 * time.Minute)
|
||
defer ticker.Stop()
|
||
for {
|
||
select {
|
||
case <-a.stopCh:
|
||
return
|
||
case <-ticker.C:
|
||
a.pruneStates()
|
||
}
|
||
}
|
||
}()
|
||
}
|
||
|
||
func (a *DatabaseAuthenticator) Close() error {
|
||
a.closeOnce.Do(func() { close(a.stopCh) })
|
||
a.wg.Wait()
|
||
return nil
|
||
}
|
||
```
|
||
|
||
Wire `Close` into the server's shutdown path, and add `defer logger.CatchPanic(…)()`
|
||
inside the worker so a panic in `pruneStates` does not kill the process.
|
||
|
||
Log an overwrite in `ConfigureOAuth2Provider` (`logger.Info`, not `Warn` —
|
||
reconfiguration is an operator action, not an error) so a duplicated provider name
|
||
is visible.
|
||
|
||
---
|
||
|
||
## 22. Medium — "no rules" is returned as an error, so a normal read is reported to Sentry
|
||
|
||
`ApplyColumnSecurity` treats an absent bucket as a failure:
|
||
|
||
```go
|
||
// provider.go:311-314
|
||
colsecList, ok := m.ColumnSecurity[fmt.Sprintf("%s.%s@%d", pSchema, pTablename, pUserID)]
|
||
if !ok || colsecList == nil {
|
||
return records, fmt.Errorf("nocolumn security data")
|
||
}
|
||
```
|
||
|
||
and the hook logs that failure at `Warn`:
|
||
|
||
```go
|
||
// hooks.go:182-187
|
||
maskedResult, err := securityList.ApplyColumnSecurity(resultValue, modelType, userID, schema, tablename)
|
||
if err != nil {
|
||
logger.Warn("Column security error: %v", err)
|
||
// Don't fail the request, just log the issue
|
||
return nil
|
||
}
|
||
```
|
||
|
||
`logger.Warn` is forwarded to the error tracker (`logger/logger.go:118-122`), so
|
||
**every read of every table that has no column-masking rules produces a Sentry
|
||
event**. The absence of rules is the normal case — most tables mask nothing — so
|
||
this is not an edge case but the steady state. At any real request rate it exhausts
|
||
the error-tracking quota and buries the events that matter; the `Warn` also cannot
|
||
be tuned away without losing the genuine failures, because both arrive with the
|
||
same level and the only distinguishing feature is the error string
|
||
`"nocolumn security data"` (note the missing space, which makes it harder to grep
|
||
for than it should be).
|
||
|
||
Two things amplify it.
|
||
|
||
**In Direct mode, this fires on every read of every table.** Both database
|
||
providers refuse to operate without the stored procedures:
|
||
|
||
```go
|
||
// providers.go:810-813
|
||
func (p *DatabaseColumnSecurityProvider) GetColumnSecurity(ctx context.Context, userID int, schema, table string) ([]ColumnSecurity, error) {
|
||
if !p.capability.ShouldUseProcedure(ctx, p.queryMode, p.getDB(), p.sqlNames.ColumnSecurity) {
|
||
return nil, ErrDirectModeUnsupported
|
||
}
|
||
```
|
||
|
||
```go
|
||
// providers.go:926-929
|
||
func (p *DatabaseRowSecurityProvider) GetRowSecurity(ctx context.Context, userRef any, schema, table string) (RowSecurity, error) {
|
||
if !p.capability.ShouldUseProcedure(ctx, p.queryMode, p.getDB(), p.sqlNames.RowSecurity) {
|
||
return RowSecurity{}, ErrDirectModeUnsupported
|
||
}
|
||
```
|
||
|
||
`ErrDirectModeUnsupported` is explicit about the consequence — *"direct mode does
|
||
not support column/row security; requires the
|
||
resolvespec_column_security/resolvespec_row_security stored procedures"*
|
||
(`table_names.go:15`) — and it is honest, documented behaviour. But it means that
|
||
when the procedures are not installed, **all column masking and all row filtering
|
||
are unavailable**, and `loadSecurityRules` converts that into two `Warn` lines per
|
||
request:
|
||
|
||
```go
|
||
// hooks.go:46-64 (abridged)
|
||
err := securityList.LoadColumnSecurity(secCtx.GetContext(), userID, schema, tablename, false)
|
||
if err != nil {
|
||
logger.Warn("Failed to load column security: %v", err)
|
||
// Don't fail the request if no security rules exist
|
||
// return err
|
||
}
|
||
...
|
||
_, err = securityList.LoadRowSecurity(secCtx.GetContext(), userRef, schema, tablename, false)
|
||
if err != nil {
|
||
logger.Warn("Failed to load row security: %v", err)
|
||
}
|
||
```
|
||
|
||
So a Direct-mode deployment emits three Sentry events per secured read — two from
|
||
the loader, one from `ApplyColumnSecurity` — while silently serving unmasked,
|
||
unfiltered data. Finding 19 makes this reachable by accident: a single panicking or
|
||
erroring `pg_proc` probe downgrades a procedure-mode deployment into exactly this
|
||
state.
|
||
|
||
**A guest request adds a fourth.** `hooks.go:36` logs `"No user ID in context for
|
||
security check"` at `Warn` and returns nil, so every unauthenticated request that
|
||
reaches the hook is also a Sentry event — and unauthenticated requests are the ones
|
||
an attacker can send without limit (`middleware.audit.md` finding 1).
|
||
|
||
The row-security path is better behaved: `hooks.go:85-89` logs its "no rules" case
|
||
at `Debug` and returns nil. That is the right level — but it is also the reason a
|
||
genuine row-security lookup failure is invisible, which is finding 5's fail-open
|
||
seen from the logging side.
|
||
|
||
### Remediation
|
||
|
||
Return the distinction in the type rather than in the error, so the caller can tell
|
||
"no rules" from "lookup failed":
|
||
|
||
```go
|
||
// ApplyColumnSecurity masks the records in place. ok is false when no column rules
|
||
// are configured for this user and table, which is not an error.
|
||
func (m *SecurityList) ApplyColumnSecurity(...) (result reflect.Value, ok bool, err error)
|
||
```
|
||
|
||
and in the hook:
|
||
|
||
```go
|
||
masked, ok, err := securityList.ApplyColumnSecurity(resultValue, modelType, userID, schema, tablename)
|
||
switch {
|
||
case err != nil:
|
||
logger.Error("column security failed for %s.%s: %v", schema, tablename, err)
|
||
return fmt.Errorf("unable to apply column security to %s.%s", schema, tablename) // fail closed
|
||
case !ok:
|
||
return nil // no rules configured: nothing to apply, nothing to log
|
||
}
|
||
if masked.IsValid() && masked.CanInterface() {
|
||
secCtx.SetResult(masked.Interface())
|
||
}
|
||
return nil
|
||
```
|
||
|
||
Two things happen at once here and both matter: the normal case stops logging, and
|
||
a genuine failure stops being ignored. That second half is finding 5 — the change
|
||
of return type is what makes failing closed expressible.
|
||
|
||
For Direct mode specifically:
|
||
|
||
1. **Decide the policy explicitly at startup, not per request.** If the provider
|
||
cannot supply rules, either refuse to start with security hooks registered, or
|
||
register a no-op provider and log the decision *once*:
|
||
```go
|
||
if errors.Is(probeErr, ErrDirectModeUnsupported) {
|
||
logger.Warn("column/row security unavailable: %v; hooks will not be registered", probeErr)
|
||
}
|
||
```
|
||
`Warn` is correct there — it is operator-facing, happens once, and is exactly
|
||
what should reach the error tracker.
|
||
2. **Make `errors.Is(err, ErrDirectModeUnsupported)` a distinct branch** in
|
||
`loadSecurityRules` so it is logged at `Debug` (or not at all, given the startup
|
||
line) rather than as a per-request warning.
|
||
3. Fix the `"nocolumn security data"` message while there.
|
||
---
|
||
|
||
## 23. Medium — the OAuth signing key is ephemeral, so tokens do not survive a restart
|
||
|
||
```go
|
||
// oauth_server.go:175-196
|
||
signingKey := cfg.SigningKey
|
||
if signingKey == nil {
|
||
var err error
|
||
signingKey, err = rsa.GenerateKey(rand.Reader, 2048)
|
||
if err != nil {
|
||
// Signing keys are only required for id_token issuance (OIDC "openid" scope);
|
||
// leaving signingKey nil degrades gracefully by omitting id_token/JWKS support.
|
||
signingKey = nil
|
||
}
|
||
}
|
||
...
|
||
if signingKey != nil {
|
||
s.signingKeyID = rsaKeyID(&signingKey.PublicKey)
|
||
}
|
||
```
|
||
|
||
The fallback is correct in construction — 2048-bit RSA from `crypto/rand`, key ID
|
||
derived from the public key (`rsaKeyID`, `:1271`) — and `cfg.SigningKey` is the
|
||
documented way to supply a stable one. Two problems follow from the default path.
|
||
|
||
First, a key-generation failure is swallowed: `signingKey = nil` with no log line
|
||
at all, and `NewOAuthServer` returns a server that silently omits `id_token` and
|
||
serves an empty JWKS. `rsa.GenerateKey` failing means `crypto/rand` failed, which is
|
||
not a condition to degrade quietly through — and the comment explains the *design*
|
||
without acknowledging that the operator is never told.
|
||
|
||
Second, and the reason this finding exists, the generated key is per process:
|
||
|
||
* **Every restart invalidates every issued token.** Clients holding valid,
|
||
unexpired access tokens get `invalid_token` after a deploy, with no way to
|
||
distinguish that from a revocation.
|
||
* **Replicas disagree.** Two instances behind a load balancer generate different
|
||
keys, so a token minted by one is rejected by the other — an intermittent,
|
||
load-balancer-dependent 401 that is painful to diagnose.
|
||
* **JWKS churns.** `/.well-known/jwks.json` (`:343`) publishes the current public
|
||
key only; a relying party that caches JWKS sees the key disappear rather than
|
||
rotate, because there is no overlap period and no second key in the set.
|
||
|
||
Registered clients (finding 11) and authorization codes share the restart problem,
|
||
though codes have `PersistCodes` (`:818`) as an opt-in remedy.
|
||
|
||
### Remediation
|
||
|
||
1. **Make the key a required configuration item in production.** Load from a file
|
||
or secret manager, and log loudly at startup when falling back to a generated
|
||
key:
|
||
```go
|
||
logger.Warn("OAuth signing key not configured; generated an ephemeral key. " +
|
||
"Tokens will not survive a restart and will not validate across replicas.")
|
||
```
|
||
(`Warn` is right here: it is not attacker-triggerable, it happens once, and it
|
||
is exactly what should reach the error tracker.)
|
||
2. **Support two keys** — a current signing key and a previous verification key —
|
||
and publish both in JWKS so rotation does not invalidate outstanding tokens.
|
||
The `keyID` derivation already supports this; the JWKS handler needs to emit a
|
||
set rather than a single entry.
|
||
3. Document the requirement next to `PersistCodes` in the OAuth section of the
|
||
README, since the two have the same "works in dev, breaks in HA" character.
|
||
|
||
---
|
||
|
||
## 24. Medium — `contains` is prefix-or-suffix, not substring, and it decides the primary key
|
||
|
||
```go
|
||
// hooks.go:404-407
|
||
func contains(s, substr string) bool {
|
||
return len(s) >= len(substr) && s[:len(substr)] == substr ||
|
||
len(s) > len(substr) && s[len(s)-len(substr):] == substr
|
||
}
|
||
```
|
||
|
||
This is `strings.HasPrefix(s, substr) || strings.HasSuffix(s, substr)`. It does not
|
||
search the middle. There are three callers, and each is wrong in a different way.
|
||
|
||
**Primary-key detection (`hooks.go:117`).** This is the consequential one, because
|
||
its output goes into a SQL `WHERE` clause:
|
||
|
||
```go
|
||
// hooks.go:112-125
|
||
pkName := "id" // default
|
||
for i := 0; i < modelType.NumField(); i++ {
|
||
field := modelType.Field(i)
|
||
if tag := field.Tag.Get("bun"); tag != "" {
|
||
// Check for primary key tag
|
||
if contains(tag, "pk") || contains(tag, "primary_key") {
|
||
if sqlName := extractSQLName(tag); sqlName != "" {
|
||
pkName = sqlName
|
||
}
|
||
break
|
||
}
|
||
}
|
||
}
|
||
```
|
||
|
||
`pkName` is substituted into the row-security template as `{PrimaryKeyName}`
|
||
(`provider.go:44`). The tag forms in this repository are:
|
||
|
||
| bun tag | `contains(tag,"pk")` | result |
|
||
| --- | --- | --- |
|
||
| `id,pk` | true (suffix) | detected |
|
||
| `pk,autoincrement` | true (prefix) | detected |
|
||
| `id,pk,autoincrement` | **false** | missed → `pkName` stays `"id"` |
|
||
| `column:user_id,pk` | true (suffix) | detected, but `extractSQLName` returns `"column:user_id"` |
|
||
| `pkid` | true (prefix) | **false positive** — a column named `pkid` is taken as the primary key |
|
||
|
||
The two failure rows both produce a wrong `{PrimaryKeyName}` in a row-security
|
||
filter. A missed primary key silently falls back to `"id"`, which is right often
|
||
enough to hide the bug and wrong exactly where it matters — a table whose key is
|
||
`uuid` or `tenant_id` gets a filter referencing a column that may not exist
|
||
(a SQL error, surfaced through finding 22's fail-open `Warn`) or, worse, one that
|
||
exists but is not the key.
|
||
|
||
**`extractSQLName`'s two uses (`hooks.go:414`, `:417`).**
|
||
|
||
```go
|
||
// hooks.go:409-422
|
||
func extractSQLName(tag string) string {
|
||
parts := splitTag(tag, ',')
|
||
for _, part := range parts {
|
||
if part != "" && !contains(part, ":") {
|
||
return part
|
||
}
|
||
if contains(part, "column:") {
|
||
return part[7:] // Skip "column:"
|
||
}
|
||
}
|
||
return ""
|
||
}
|
||
```
|
||
|
||
`contains(part, ":")` asks "does this part begin or end with a colon", not "does it
|
||
contain one". For `column:user_id` the colon is in the middle, so `contains` is
|
||
false, `!contains` is true, and the function returns `"column:user_id"` whole —
|
||
which then becomes `pkName`. The `column:` branch at `:417` is only reachable for a
|
||
part that *starts* with `column:`, which the branch above has already consumed, so
|
||
it is effectively dead.
|
||
|
||
This is live today: row security is inert because of finding 2, so a wrong `pkName`
|
||
has no effect yet. Fixing finding 2 without fixing this one turns a dormant
|
||
correctness bug into a live one, which is why finding 2's remediation and this one
|
||
should ship together.
|
||
|
||
### Remediation
|
||
|
||
Delete `contains`, `extractSQLName` and `splitTag`, and use the standard library
|
||
plus the helper that already parses these tags correctly
|
||
(`common/handler_utils.go:53-55`):
|
||
|
||
```go
|
||
func primaryKeyName(t reflect.Type) string {
|
||
for i := 0; i < t.NumField(); i++ {
|
||
tag := t.Field(i).Tag.Get("bun")
|
||
if tag == "" {
|
||
continue
|
||
}
|
||
opts := strings.Split(tag, ",")
|
||
isPK := false
|
||
for _, o := range opts[1:] {
|
||
if o == "pk" || o == "primary_key" {
|
||
isPK = true
|
||
break
|
||
}
|
||
}
|
||
if !isPK {
|
||
continue
|
||
}
|
||
if name := common.ExtractTagValue(tag, "column"); name != "" {
|
||
return name
|
||
}
|
||
if opts[0] != "" && !strings.Contains(opts[0], ":") {
|
||
return opts[0]
|
||
}
|
||
return strings.ToLower(t.Field(i).Name)
|
||
}
|
||
return "id"
|
||
}
|
||
```
|
||
|
||
Matching options exactly (`o == "pk"`) rather than by substring removes both the
|
||
missed-key and false-positive rows from the table above. Better still, take the
|
||
primary key from `pkg/reflection`, which already resolves it from the model
|
||
metadata rather than re-parsing tags — two implementations of tag parsing in one
|
||
repository is how they drift.
|
||
|
||
Add a test asserting `primaryKeyName` on a struct tagged `bun:"id,pk,autoincrement"`
|
||
and on one tagged `bun:"column:tenant_uuid,pk"`; both fail against the current code.
|
||
|
||
---
|
||
|
||
## 25. Medium — `maskString`'s offsets are off by one, and it masks at byte positions
|
||
|
||
```go
|
||
// provider.go:85-122 (abridged)
|
||
func maskString(pString string, maskStart, maskEnd int, maskChar string, invert bool) string {
|
||
strLen := len(pString)
|
||
middleIndex := (strLen / 2)
|
||
newStr := ""
|
||
if maskStart == 0 && maskEnd == 0 {
|
||
maskStart = strLen
|
||
maskEnd = strLen
|
||
}
|
||
...
|
||
for index, char := range pString {
|
||
if invert && index >= middleIndex-maskStart && index <= middleIndex { newStr += maskChar; continue }
|
||
if invert && index <= middleIndex+maskEnd && index >= middleIndex { newStr += maskChar; continue }
|
||
if !invert && index <= maskStart { newStr += maskChar; continue }
|
||
if !invert && index >= strLen-1-maskEnd { newStr += maskChar; continue }
|
||
newStr += string(char)
|
||
}
|
||
return newStr
|
||
}
|
||
```
|
||
|
||
Called from `setColSecValue` for string fields (`provider.go:275`) and for JSON
|
||
sub-fields (`:289`), with `colsec.MaskStart`, `colsec.MaskEnd`, `colsec.MaskChar`
|
||
and `colsec.MaskInvert` straight from the rule.
|
||
|
||
Measured behaviour, from a standalone run of this exact function:
|
||
|
||
| input | maskStart | maskEnd | invert | output |
|
||
| --- | --- | --- | --- | --- |
|
||
| `4111111111111234` | 4 | 0 | false | `*****1111111123*` |
|
||
| `4111111111111234` | 4 | 4 | false | `*****111111*****` |
|
||
| `4111111111111234` | 0 | 4 | false | `*1111111111*****` |
|
||
| `4111111111111234` | 0 | 0 | false | `****************` |
|
||
| `4111111111111234` | 4 | 4 | true | `4111*********234` |
|
||
| `alice@example.com` | 2 | 3 | false | `***ce@example****` |
|
||
| `Müller` | 2 | 1 | false | `**ll**` |
|
||
| `日本語テスト` | 2 | 1 | false | `*本語テスト` |
|
||
|
||
Three observations.
|
||
|
||
**The bounds are off by one, in the unsafe direction for `maskEnd` and the safe
|
||
direction for `maskStart`.** `index <= maskStart` masks `maskStart+1` characters
|
||
from the front, and `index >= strLen-1-maskEnd` masks `maskEnd+1` from the back —
|
||
so `MaskStart: 4` masks five characters, not four. Rows 1–3 show it: with
|
||
`maskEnd: 4`, five trailing digits are hidden; with `maskEnd: 0`, the final digit
|
||
is still masked even though zero was requested. A rule author calibrating "show
|
||
the last four digits of a card" gets the last three.
|
||
|
||
**`maskStart == 0 && maskEnd == 0` masks the entire string** (row 4), because both
|
||
are rewritten to `strLen`. That is the right default — an unconfigured rule hides
|
||
everything rather than revealing it — but it is undocumented and surprising, and
|
||
it means there is no way to express "mask nothing".
|
||
|
||
**The offsets are byte offsets, so non-ASCII text is masked at the wrong
|
||
positions — sometimes revealing data.** `for index, char := range pString` yields
|
||
the *byte* index of each rune, and `strLen` is `len()` in bytes. For `Müller` (7
|
||
bytes, 6 runes) the requested three-from-the-front only lands on two runes,
|
||
because byte index 2 is the continuation byte of `ü` and never appears as a loop
|
||
index. For `日本語テスト` (18 bytes, 6 runes, indices 0/3/6/9/12/15) the result is
|
||
worse: `index <= 2` matches only index 0, and `index >= 16` matches nothing, so
|
||
**one of six characters is masked** and five are returned in clear. A rule written
|
||
to hide most of a value hides one sixth of it as soon as the data is not Latin-1.
|
||
Any deployment masking names, addresses or free text in a non-English locale is
|
||
under-masking today.
|
||
|
||
The `newStr += maskChar` accumulation is also O(n²) in allocations, which is
|
||
irrelevant at field sizes but trivially avoidable.
|
||
|
||
### Remediation
|
||
|
||
Operate on runes, define the parameters as counts rather than indices, and fail
|
||
safe when they are contradictory:
|
||
|
||
```go
|
||
// maskString replaces characters in s with maskChar. maskStart is the number of
|
||
// leading characters to mask and maskEnd the number of trailing characters; when
|
||
// invert is set, the middle is masked and the edges are kept. Zero for both masks
|
||
// the whole string. If the masked regions meet or overlap, the whole string is
|
||
// masked.
|
||
func maskString(s string, maskStart, maskEnd int, maskChar string, invert bool) string {
|
||
r := []rune(s)
|
||
n := len(r)
|
||
if maskChar == "" { maskChar = "*" }
|
||
if maskStart <= 0 && maskEnd <= 0 { return strings.Repeat(maskChar, n) }
|
||
if maskStart < 0 { maskStart = 0 }
|
||
if maskEnd < 0 { maskEnd = 0 }
|
||
|
||
var b strings.Builder
|
||
b.Grow(len(s))
|
||
for i := range r {
|
||
masked := i < maskStart || i >= n-maskEnd
|
||
if invert { masked = !masked }
|
||
if masked {
|
||
b.WriteString(maskChar)
|
||
} else {
|
||
b.WriteRune(r[i])
|
||
}
|
||
}
|
||
return b.String()
|
||
}
|
||
```
|
||
|
||
Note that `invert` here means exactly "keep the edges, mask the middle", which is
|
||
what the current invert branches approximate via `middleIndex` — and the
|
||
`middleIndex` arithmetic has its own edge cases (`maskStart > middleIndex` makes
|
||
the lower bound negative, masking from index 0) that this formulation removes.
|
||
|
||
Two cautions for the rollout:
|
||
|
||
* This changes output for every configured rule, because of the off-by-one. Produce
|
||
a before/after table for the rules in each deployment's `column_security` data
|
||
and adjust `MaskStart`/`MaskEnd` by one where the old behaviour was the intended
|
||
one.
|
||
* `provider_test.go:130-150` covers ASCII only. Add the `Müller` and `日本語テスト`
|
||
cases above as fixtures so the rune handling cannot regress.
|
||
|
||
---
|
||
|
||
## 26. Low — both maps are read once before their mutex is taken
|
||
|
||
```go
|
||
// provider.go:301-311
|
||
func (m *SecurityList) ApplyColumnSecurity(records reflect.Value, modelType reflect.Type, pUserID int, pSchema, pTablename string) (reflect.Value, error) {
|
||
defer logger.CatchPanic("ApplyColumnSecurity")()
|
||
|
||
if m.ColumnSecurity == nil {
|
||
return records, fmt.Errorf("security not initialized")
|
||
}
|
||
|
||
m.ColumnSecurityMutex.RLock()
|
||
defer m.ColumnSecurityMutex.RUnlock()
|
||
|
||
colsecList, ok := m.ColumnSecurity[...]
|
||
```
|
||
|
||
The same shape at `provider.go:442-452` (`GetRowSecurityTemplate`, reading
|
||
`m.RowSecurity` at `:445` before locking at `:449`) and at `:125-136`
|
||
(`ColumSecurityApplyOnRecord`, reading at `:127` before locking at `:136`).
|
||
|
||
Reading the map header outside the lock is a data race under the Go memory model
|
||
whenever another goroutine can assign the field. `LoadColumnSecurity` does exactly
|
||
that — it lazily creates the map — so a first-request race between a loader and a
|
||
reader is possible in principle. In practice the maps are created in the
|
||
constructor and only ever written *into*, not reassigned, which is why this has
|
||
not shown up: `go test -race` on the package passes. It is nevertheless
|
||
unsynchronised access to a field that another method assigns, and the race
|
||
detector will flag it the moment someone adds a `Clear`-style method that does
|
||
`m.ColumnSecurity = nil`.
|
||
|
||
The guard is also nearly useless: a nil map read returns the zero value and
|
||
`ok == false`, which the code immediately below already handles. The check exists
|
||
only to produce a different error message.
|
||
|
||
### Remediation
|
||
|
||
Move the check inside the lock, or delete it:
|
||
|
||
```go
|
||
m.ColumnSecurityMutex.RLock()
|
||
defer m.ColumnSecurityMutex.RUnlock()
|
||
|
||
colsecList, ok := m.ColumnSecurity[securityKey(pSchema, pTablename, pUserID)]
|
||
if !ok || len(colsecList) == 0 {
|
||
return records, nil, nil // see finding 22 on not returning this as an error
|
||
}
|
||
```
|
||
|
||
Reading a nil map is legal, so dropping the pre-check loses nothing. Do the same
|
||
at `:127` and `:445`.
|
||
|
||
---
|
||
|
||
## 27. Low — the security maps and their mutexes are exported
|
||
|
||
```go
|
||
// provider.go:54-61
|
||
type SecurityList struct {
|
||
provider SecurityProvider
|
||
|
||
ColumnSecurityMutex sync.RWMutex
|
||
ColumnSecurity map[string][]ColumnSecurity
|
||
RowSecurityMutex sync.RWMutex
|
||
RowSecurity map[string]RowSecurity
|
||
}
|
||
```
|
||
|
||
`provider` is correctly unexported; everything else is public. Any package holding
|
||
a `*SecurityList` can read the maps without the lock, write to them without the
|
||
lock, take the mutex and forget to release it, or copy the struct — which copies a
|
||
`sync.RWMutex` and is a `go vet` error class of its own. The invariant "the map is
|
||
only touched while its mutex is held" is unenforceable from outside the package,
|
||
and `SecurityList` is reachable through `GetSecurityList()` from every spec
|
||
adapter.
|
||
|
||
There is no reason for the exposure: every legitimate operation already has a
|
||
method (`LoadColumnSecurity`, `ApplyColumnSecurity`, `LoadRowSecurity`,
|
||
`GetRowSecurityTemplate`, `ClearSecurity`).
|
||
|
||
### Remediation
|
||
|
||
```go
|
||
type SecurityList struct {
|
||
provider SecurityProvider
|
||
|
||
colMu sync.RWMutex
|
||
columns map[string][]ColumnSecurity
|
||
rowMu sync.RWMutex
|
||
rows map[string]RowSecurity
|
||
}
|
||
```
|
||
|
||
Add `func (m *SecurityList) Snapshot() (...)` if a consumer genuinely needs to
|
||
inspect the state, returning copies. A grep for `ColumnSecurityMutex` and
|
||
`RowSecurity` outside `pkg/security` shows no external users today, so this is a
|
||
rename with no downstream churn — cheap to do now, and it stops being cheap once
|
||
an integrator depends on it.
|
||
|
||
Also embed `_ noCopy` or keep `SecurityList` pointer-only by convention; `go vet`'s
|
||
`copylocks` check catches accidental value copies, but only if the package is
|
||
vetted in CI (see `_CROSS-CUTTING.audit.md` on the lint gate).
|
||
|
||
---
|
||
|
||
## 28. Low — three nested loops share the index name `i`, and one of them indexes the wrong slice
|
||
|
||
```go
|
||
// provider.go:144-185 (abridged, indentation preserved)
|
||
for i := range colsecList {
|
||
colsec := &colsecList[i]
|
||
...
|
||
for i, path := range colsec.Path {
|
||
...
|
||
for ri := range newRecords {
|
||
...
|
||
columnData := reflection.GetModelColumnDetail(newRecords[ri])
|
||
lastColumnData := reflection.GetModelColumnDetail(lastRecords[ri])
|
||
for i, cols := range columnData {
|
||
if cols.SQLName != "" && strings.EqualFold(cols.SQLName, path) {
|
||
...
|
||
oldField = lastColumnData[i].FieldValue
|
||
break
|
||
}
|
||
if cols.Name != "" && strings.EqualFold(cols.Name, path) {
|
||
...
|
||
oldField = lastColumnData[i].FieldValue
|
||
break
|
||
}
|
||
```
|
||
|
||
`i` is declared three times in nested scopes (`:144`, `:153`, `:170`), each
|
||
shadowing the last. The innermost `i` is what `:175` and `:182` use, which is
|
||
correct for `columnData` — but `lastColumnData` is a *separate* call to
|
||
`GetModelColumnDetail` on a *different* value (`lastRecords[ri]` versus
|
||
`newRecords[ri]`), and the code assumes the two slices are index-aligned.
|
||
|
||
They are aligned as long as both records are the same struct type, which
|
||
`ColumSecurityApplyOnRecord` verifies at `:130-133`. So this is not a live
|
||
mis-indexing today. It is fragile in two specific ways:
|
||
|
||
* `GetModelColumnDetail` walks embedded structs and relations; if it ever returns
|
||
a different number of entries for two values of the same type — for example
|
||
because a relation pointer is nil on one and populated on the other — the
|
||
alignment breaks and `oldField` silently becomes a different column's value. In
|
||
`ColumSecurityApplyOnRecord` that value is used to decide whether the field
|
||
changed, so a mis-alignment would mean a modified protected column is treated as
|
||
unmodified: an authorisation decision made on the wrong field.
|
||
* `lastColumnData[i]` is an unchecked index. If `lastColumnData` is shorter than
|
||
`columnData`, this panics — and the panic is recovered by
|
||
`logger.CatchPanic("ApplyColumnSecurity")` in the sibling function
|
||
(`provider.go:302`), which returns the *unmasked* records with a nil error
|
||
(finding 3).
|
||
|
||
The shadowing is what makes this hard to see: a reader checking whether `i` is the
|
||
right index has to count three scopes.
|
||
|
||
### Remediation
|
||
|
||
Rename the indices to say what they are, and look the old field up by name rather
|
||
than by position:
|
||
|
||
```go
|
||
for ci := range colsecList {
|
||
colsec := &colsecList[ci]
|
||
for pi, path := range colsec.Path {
|
||
for ri := range newRecords {
|
||
columnData := reflection.GetModelColumnDetail(newRecords[ri])
|
||
lastByName := indexByName(reflection.GetModelColumnDetail(lastRecords[ri]))
|
||
for _, col := range columnData {
|
||
if !matchesPath(col, path) { continue }
|
||
oldField, ok := lastByName[strings.ToLower(col.SQLName)]
|
||
if !ok {
|
||
return cols, fmt.Errorf("column %q present in new record but not in previous", col.SQLName)
|
||
}
|
||
...
|
||
```
|
||
|
||
Matching by name removes the positional assumption entirely and turns a silent
|
||
mis-alignment into an explicit error. Note the `pi`/`pathLen` interaction at
|
||
`:155-159` also depends on the middle `i`; renaming makes that logic legible too.
|
||
|
||
`go vet -shadow` (or `golangci-lint`'s `govet` with `shadow` enabled) catches this
|
||
class; it is not currently enabled.
|
||
|
||
---
|
||
|
||
## 29. Low — `ClearSecurity` deletes everything it is asked to keep, and has no callers
|
||
|
||
```go
|
||
// provider.go:397-417
|
||
func (m *SecurityList) ClearSecurity(pUserID int, pSchema, pTablename string) error {
|
||
var filtered []ColumnSecurity
|
||
m.ColumnSecurityMutex.Lock()
|
||
defer m.ColumnSecurityMutex.Unlock()
|
||
|
||
secKey := fmt.Sprintf("%s.%s@%d", pSchema, pTablename, pUserID)
|
||
list, ok := m.ColumnSecurity[secKey]
|
||
if !ok {
|
||
return nil
|
||
}
|
||
|
||
for i := range list {
|
||
cs := &list[i]
|
||
if cs.Schema != pSchema && cs.Tablename != pTablename && cs.UserID != pUserID {
|
||
filtered = append(filtered, *cs)
|
||
}
|
||
}
|
||
|
||
m.ColumnSecurity[secKey] = filtered
|
||
return nil
|
||
}
|
||
```
|
||
|
||
The entries under `secKey` are, by construction, exactly the entries whose schema,
|
||
table and user match the key. So `cs.Schema != pSchema && cs.Tablename !=
|
||
pTablename && cs.UserID != pUserID` is false for every one of them, `filtered`
|
||
stays nil, and the function assigns a nil slice — wiping the whole bucket rather
|
||
than clearing a subset of it. Even reading the condition charitably as intended
|
||
(`||` rather than `&&`), it would still be a no-op filter for the same reason: the
|
||
key already pins all three fields.
|
||
|
||
The effect of the wipe is not a security hole in the fail-open sense, because
|
||
`ApplyColumnSecurity` treats a nil list as "no rules" and returns an error rather
|
||
than data — but combined with `hooks.go:182-187`, which logs and continues when
|
||
`ApplyColumnSecurity` fails, a wiped bucket means **unmasked data** until the next
|
||
`LoadColumnSecurity` with `pOverwrite`.
|
||
|
||
It is also never called: grepping the repository for `ClearSecurity` finds the
|
||
definition and nothing else — no production caller, no test. Another X10 instance.
|
||
And it does not touch `RowSecurity` at all, so the one cache that most needs
|
||
invalidation (finding 14) has no invalidation path.
|
||
|
||
### Remediation
|
||
|
||
Decide which it is.
|
||
|
||
**If per-key invalidation is wanted,** it is a `delete`, not a filter, and it needs
|
||
to cover both maps:
|
||
|
||
```go
|
||
func (m *SecurityList) ClearSecurity(userID any, schema, table string) {
|
||
key := securityKey(schema, table, userID)
|
||
|
||
m.colMu.Lock()
|
||
delete(m.columns, key)
|
||
m.colMu.Unlock()
|
||
|
||
m.rowMu.Lock()
|
||
delete(m.rows, key)
|
||
m.rowMu.Unlock()
|
||
}
|
||
```
|
||
|
||
Then call it — from logout (`providers.go:316-320`, `providers_direct.go:182-206`)
|
||
and from whatever administrative path changes a rule.
|
||
|
||
**If it is not wanted,** delete the function. Dead security code is worse than
|
||
absent security code, because a reader assumes it works.
|
||
|
||
Either way, once finding 6 introduces real caching the invalidation story has to be
|
||
designed deliberately, and `pkg/cache`'s tag support (`DeleteByTag`,
|
||
`cache/provider_memory.go:194-228`) is the better substrate: tag entries with
|
||
`table:orders` and a rule change invalidates every user's copy in one call.
|
||
|
||
---
|
||
|
||
## 30. Low — string building by concatenation in per-field loops
|
||
|
||
`maskString` (`provider.go:102-120`) and `splitTag` (`hooks.go:424-441`) both build
|
||
their results with `+=` inside a loop:
|
||
|
||
```go
|
||
// hooks.go:427-436
|
||
for _, ch := range tag {
|
||
if ch == sep {
|
||
if current != "" {
|
||
parts = append(parts, current)
|
||
current = ""
|
||
}
|
||
} else {
|
||
current += string(ch)
|
||
}
|
||
}
|
||
```
|
||
|
||
Each `+=` allocates a new string and copies the accumulated prefix, so both are
|
||
O(n²) in bytes copied. `string(ch)` additionally allocates per rune.
|
||
|
||
At the sizes involved — struct tags, masked field values — this is a few hundred
|
||
bytes of garbage per call and will never appear in a profile. It is listed because
|
||
`maskString` runs once per masked field per record, so a 10 000-row response with
|
||
three masked columns calls it 30 000 times, and the fix is free.
|
||
|
||
### Remediation
|
||
|
||
`strings.Builder` in `maskString` (shown in finding 25), and `strings.Split` — or
|
||
`strings.FieldsFunc` if empty parts must be dropped, which is what the `current !=
|
||
""` check is doing — in place of `splitTag`:
|
||
|
||
```go
|
||
func splitTag(tag string, sep rune) []string {
|
||
return strings.FieldsFunc(tag, func(r rune) bool { return r == sep })
|
||
}
|
||
```
|
||
|
||
`FieldsFunc` already omits empty fields, so this is behaviour-preserving. Better
|
||
still, delete `splitTag` along with `contains` and `extractSQLName` (finding 24) and
|
||
use `common.ExtractTagValue` (`common/handler_utils.go:53-55`).
|
||
|
||
---
|
||
|
||
## 31. Low — registration reads the new user's ID with `LastInsertId`, after a non-transactional uniqueness check
|
||
|
||
```go
|
||
// providers_direct.go:105-130 (abridged)
|
||
err := a.runDBOpWithReconnect(func(db *sql.DB) error {
|
||
var count int
|
||
checkQuery := rewritePlaceholders(db, fmt.Sprintf(`SELECT COUNT(*) FROM %s WHERE username = ?`, a.tableNames.Users))
|
||
if err := db.QueryRowContext(ctx, checkQuery, req.Username).Scan(&count); err != nil { return err }
|
||
if count > 0 { return errUsernameExists }
|
||
|
||
checkQuery2 := ... `SELECT COUNT(*) FROM %s WHERE email = ?` ...
|
||
if count > 0 { return errEmailExists }
|
||
|
||
insertQuery := rewritePlaceholders(db, fmt.Sprintf(
|
||
`INSERT INTO %s (username, email, password, ...) VALUES (?, ?, ?, ...)`, a.tableNames.Users))
|
||
res, err := db.ExecContext(ctx, insertQuery, req.Username, req.Email, req.Password, req.UserLevel, rolesStr, true, now, now, 0, "")
|
||
if err != nil { return err }
|
||
userID, err = res.LastInsertId()
|
||
return err
|
||
})
|
||
```
|
||
|
||
Two portability defects in six lines.
|
||
|
||
**`LastInsertId` is not supported by `lib/pq` or `pgx`.** Those drivers return
|
||
`0, errors.New("LastInsertId is not supported by this driver")`, so on PostgreSQL —
|
||
the dialect the rest of this package is written against, with `pg_proc` probes and
|
||
`$n` placeholders — registration returns an error *after having created the user*.
|
||
The user exists, the caller sees a failure, and a retry hits `errUsernameExists`.
|
||
The correct form is `INSERT … RETURNING id` with `QueryRowContext`, which is also
|
||
what the stored-procedure path effectively does.
|
||
|
||
**Check-then-insert is a race, not a constraint.** Two concurrent registrations
|
||
with the same username both read `count == 0` and both insert. There is no
|
||
transaction around the sequence (each `db.ExecContext`/`QueryRowContext` is
|
||
autocommitted independently) and no `SELECT … FOR UPDATE`. Whether a duplicate
|
||
results depends entirely on whether the `users` table has unique indexes on
|
||
`username` and `email` — `database_schema.sql:9` does declare them, so the database
|
||
saves this, but then the error surfaced to the caller is the driver's constraint
|
||
violation rather than `errUsernameExists`, and the carefully written enumeration
|
||
handling at `:133-140` is bypassed.
|
||
|
||
Both are behind `registerDirect`, whose more serious problems — storing
|
||
`req.Password` in plaintext at `:125`, and accepting caller-supplied `UserLevel`
|
||
and `Roles` — are covered in findings 1 and 15.
|
||
|
||
### Remediation
|
||
|
||
```go
|
||
err := a.runDBOpWithReconnect(func(db *sql.DB) error {
|
||
q := rewritePlaceholders(db, fmt.Sprintf(
|
||
`INSERT INTO %s (username, email, password, user_level, roles, is_active, created_at, updated_at, program_user_id, program_user_table)
|
||
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) RETURNING id`, a.tableNames.Users))
|
||
return db.QueryRowContext(ctx, q, req.Username, req.Email, hashed, level, rolesStr, true, now, now, 0, "").Scan(&userID)
|
||
})
|
||
if err != nil {
|
||
switch {
|
||
case isUniqueViolation(err, "users_username_key"): return nil, errUsernameExists
|
||
case isUniqueViolation(err, "users_email_key"): return nil, errEmailExists
|
||
}
|
||
return nil, fmt.Errorf("registration failed: %w", err)
|
||
}
|
||
```
|
||
|
||
Let the unique constraint be the check — it is the only one that is actually
|
||
atomic — and map the violation back to the existing sentinel errors so the
|
||
anti-enumeration behaviour at `:133-140` still applies. For MySQL/SQLite support,
|
||
`RETURNING` needs a dialect switch; `rewritePlaceholders` (`query_mode.go:141-157`)
|
||
is the existing seam for that.
|
||
|
||
---
|
||
|
||
## 32. Low — `authenticateCallback` may return `(nil, nil)`, and the caller does not check
|
||
|
||
```go
|
||
// providers.go:382-387
|
||
if len(tokens) == 0 {
|
||
if a.authenticateCallback != nil {
|
||
return a.authenticateCallback(r)
|
||
}
|
||
return nil, fmt.Errorf("session token required")
|
||
}
|
||
```
|
||
|
||
```go
|
||
// providers.go:452-459
|
||
// All tokens failed — try callback before returning error
|
||
if a.authenticateCallback != nil {
|
||
return a.authenticateCallback(r)
|
||
}
|
||
if lastErr != nil {
|
||
return nil, lastErr
|
||
}
|
||
return nil, fmt.Errorf("authentication failed for all provided tokens")
|
||
```
|
||
|
||
`authenticateCallback` is an integrator-supplied `func(*http.Request)
|
||
(*UserContext, error)` (`providers.go:126-129`). Its result is returned
|
||
unexamined, so a callback that returns `(nil, nil)` — a plausible reading of "I
|
||
have no opinion about this request" — propagates a nil `*UserContext` with a nil
|
||
error to `authenticateRequest`:
|
||
|
||
```go
|
||
// middleware.go:82-91
|
||
userCtx, err := provider.Authenticate(r)
|
||
if err != nil {
|
||
http.Error(w, "Authentication failed: "+err.Error(), http.StatusUnauthorized)
|
||
return nil, false
|
||
}
|
||
return setUserContext(r, userCtx), true
|
||
```
|
||
|
||
which passes it to `setUserContext`:
|
||
|
||
```go
|
||
// middleware.go:59-78 (abridged)
|
||
func setUserContext(r *http.Request, userCtx *UserContext) *http.Request {
|
||
ctx := r.Context()
|
||
ctx = context.WithValue(ctx, UserContextKey, userCtx)
|
||
ctx = context.WithValue(ctx, UserIDKey, userCtx.UserID)
|
||
...
|
||
```
|
||
|
||
`userCtx.UserID` on a nil pointer panics. The panic happens inside the middleware
|
||
chain, where the outermost recovery middleware turns it into a 500 — so the
|
||
outcome is a crash-per-request, not a bypass, provided the recovery middleware is
|
||
installed. The doc comment at `:126-129` does not state the contract, so a
|
||
callback author has no way to know `(nil, nil)` is forbidden.
|
||
|
||
### Remediation
|
||
|
||
Validate at the boundary — the callback is external code, so treat its result as
|
||
untrusted:
|
||
|
||
```go
|
||
if a.authenticateCallback != nil {
|
||
userCtx, err := a.authenticateCallback(r)
|
||
if err != nil {
|
||
return nil, err
|
||
}
|
||
if userCtx == nil {
|
||
return nil, fmt.Errorf("authentication callback returned no user context")
|
||
}
|
||
return userCtx, nil
|
||
}
|
||
```
|
||
|
||
and defensively in the middleware, as shown in finding 18:
|
||
|
||
```go
|
||
if userCtx == nil {
|
||
logger.Error("authenticator %T returned nil user context with nil error", provider)
|
||
http.Error(w, "Internal Server Error", http.StatusInternalServerError)
|
||
return nil, false
|
||
}
|
||
```
|
||
|
||
Document the contract on the callback field: *"must return a non-nil UserContext or
|
||
a non-nil error; returning (nil, nil) is a programming error."* Same treatment for
|
||
`HeaderAuthenticator`'s equivalent hook (finding 8).
|
||
|
||
---
|
||
|
||
## 33. Low — `RequestPasswordReset` returns the raw token to its caller
|
||
|
||
```go
|
||
// providers_direct.go:336-359 (abridged)
|
||
rawBytes := make([]byte, 32)
|
||
if _, err := rand.Read(rawBytes); err != nil {
|
||
return nil, fmt.Errorf("failed to generate reset token: %w", err)
|
||
}
|
||
rawToken := hex.EncodeToString(rawBytes)
|
||
hash := sha256.Sum256([]byte(rawToken))
|
||
tokenHash := hex.EncodeToString(hash[:])
|
||
...
|
||
return &PasswordResetResponse{Token: rawToken, ExpiresIn: 3600}, nil
|
||
```
|
||
|
||
The cryptography here is right, and worth saying so: 256 bits from `crypto/rand`,
|
||
only the SHA-256 hash is stored (`:352`), a one-hour expiry, prior unused tokens
|
||
are deleted before insert, `used`/`used_at` are recorded on completion
|
||
(`:399-400`), and all sessions are invalidated when the password changes
|
||
(`:395-397`). The enumeration-resistant early return at `:330-332` — generic
|
||
success when the user does not exist — is also correct.
|
||
|
||
The issue is the return value. `PasswordResetResponse.Token` carries the raw token
|
||
back to the caller, and the caller is an HTTP handler. Whether that token reaches
|
||
the requester's inbox (correct) or the HTTP response body (an account-takeover
|
||
primitive: anyone who can POST `/password-reset` for `victim@example.com` receives
|
||
the reset token) is entirely up to the integrator, and nothing in the signature,
|
||
the doc comment or `SECURITY_FEATURES.md` says which is intended.
|
||
|
||
There is a second, quieter exposure: the raw token is now in the caller's memory
|
||
and in any structure the caller logs. `PasswordResetResponse` has no custom
|
||
`String()`, so `logger.Debug("reset response: %+v", resp)` prints it — and if
|
||
anyone writes that at `Warn`, it goes to Sentry (X8).
|
||
|
||
### Remediation
|
||
|
||
Make the safe path the only path: have the package deliver the token and return
|
||
nothing sensitive.
|
||
|
||
```go
|
||
type PasswordResetResponse struct {
|
||
// Requested is always true for a well-formed request, whether or not the
|
||
// address matched an account. The token is never returned; it is delivered
|
||
// through the configured Notifier.
|
||
Requested bool
|
||
ExpiresIn int
|
||
}
|
||
|
||
type ResetNotifier interface {
|
||
SendPasswordReset(ctx context.Context, email, rawToken string, expiresIn time.Duration) error
|
||
}
|
||
```
|
||
|
||
If returning the token must stay — for tests, or for integrators who deliver it
|
||
themselves — then:
|
||
|
||
1. Rename the field to `RawTokenDoNotLogOrReturn` or gate it behind an explicit
|
||
`AllowTokenInResponse bool` on the authenticator, defaulting to false.
|
||
2. Add `func (r PasswordResetResponse) String() string { return "PasswordResetResponse{redacted}" }`
|
||
so `%v`/`%+v` cannot print it.
|
||
3. State the requirement in the doc comment and in `SECURITY_FEATURES.md`: *the
|
||
token must never appear in an HTTP response.*
|
||
4. Rate-limit the endpoint per account and per IP — the reset path is otherwise an
|
||
unauthenticated way to generate a row in `user_password_resets` per request
|
||
(`middleware.audit.md` finding 1 again).
|
||
|
||
---
|
||
|
||
## Cross-cutting references
|
||
|
||
**X8 — `Warn`/`Error` are forwarded to the error tracker; `Info`/`Debug` are not.**
|
||
Confirmed at `logger/logger.go:118-122` and `:135-139` against `:100-106` and
|
||
`:142-148`. This package is the largest source of X8 exposure in the repository,
|
||
in both directions:
|
||
|
||
The complete `Warn`/`Error` inventory for this package is eight lines, and six of
|
||
them are problems:
|
||
|
||
| site | level | reached when | problem |
|
||
| --- | --- | --- | --- |
|
||
| `providers.go:391` | Warn | one comma in an `Authorization` header | logs the **raw header** — finding 7 |
|
||
| `hooks.go:36` | Warn | any request with no user ID | fires on every guest request |
|
||
| `hooks.go:48` | Warn | column rules cannot be loaded | every request in Direct mode — finding 22 |
|
||
| `hooks.go:61` | Warn | row rules cannot be loaded | every request in Direct mode — finding 22 |
|
||
| `hooks.go:93` | Warn | a user is blocked | logs the rendered `userRef` — `*UserContext` with session token and claims |
|
||
| `hooks.go:184` | Warn | a table has no column rules | every secured read — finding 22 |
|
||
| `provider.go:132` | Error | prev/new record type mismatch | appropriate |
|
||
| `provider.go:204` | Warn | an unsettable field | once per unsettable field per record — finding 4 |
|
||
|
||
Five of the six are attacker-triggerable with no rate limiter in front of them
|
||
(`middleware.audit.md` finding 1), and three of them fire during entirely normal
|
||
operation. Any one will exhaust a Sentry quota from a single client. `hooks.go:93`
|
||
and `providers.go:391` additionally put a credential into the error tracker.
|
||
|
||
The fix in each case is to choose the level by *who causes the event*: client
|
||
behaviour is `Debug`, operator misconfiguration is `Warn` (once, at startup), and
|
||
only genuine server-side faults are `Error`. `hooks.go:93` is the one case where
|
||
`Warn` is right — a blocked access attempt is worth an alert — and it needs only the
|
||
redacting `String()` from finding 7 to stop leaking.
|
||
|
||
**X10 — configured, written, and never installed.** Two new instances from this
|
||
package:
|
||
|
||
* `ValidateSQLNames` (`sql_names.go:242-258`) and `ValidateTableNames`
|
||
(`table_names.go:82-96`) have no production callers (finding 17).
|
||
* `ClearSecurity` (`provider.go:397-417`) has no callers at all (finding 29).
|
||
|
||
To which finding 2 adds a third and worst variant: row-level security is *wired*
|
||
but inert, because the type assertion at `hooks.go:134-136` can never succeed.
|
||
|
||
**`pkg/logger`.** `CatchPanic` on a function with unnamed results returns zero
|
||
values and a nil error, which is how finding 3 turns a panic into "masking
|
||
succeeded". `logger.HandlePanic(name, r)` with a named `err` result is the correct
|
||
helper. See `logger.audit.md` finding 2 for the general form; `pkg/security` has
|
||
the two most consequential instances (`provider.go:302`, `:443`).
|
||
|
||
**`pkg/modelregistry`.** `modelregistry.audit.md` finding 1 (lookup by name misses
|
||
schema-qualified registrations) is the upstream half of finding 15: the registry
|
||
fails to find the model, and `hooks.go:287`/`:311` turn that into `return nil //
|
||
model not registered, allow by default`.
|
||
|
||
**`pkg/cache`.** The natural substrate for finding 6 (no caching at all) and
|
||
finding 14 (unbounded maps): `MaxSize` with LRU eviction
|
||
(`cache/provider_memory.go:105-109`, `:307-326`) and tag-based invalidation
|
||
(`SetWithTags` `:121-170`, `DeleteByTag` `:194-228`). `restheadspec` already uses
|
||
the tag pattern for query totals (`restheadspec/cache_helpers.go:99-113`), so
|
||
there is a working precedent in-tree.
|
||
|
||
**`pkg/middleware`.** `middleware.audit.md` finding 1 — no rate limiter is
|
||
installed by default — is what makes findings 11, 18, 22 and 33 practical rather
|
||
than theoretical. `pkg/security`'s own `middleware.go` is separate from
|
||
`pkg/middleware` and composes with it; the recovery middleware from that package
|
||
is what keeps finding 32 to a 500 rather than a crash.
|
||
|
||
**`pkg/config`.** There is no `security` section in `setDefaults`
|
||
(`config/manager.go:170-293`), so nothing in this package is configurable through
|
||
the standard config path — the OAuth signing key (finding 23), the query mode
|
||
(finding 19) and the table/SQL name overrides (finding 17) are all constructor
|
||
arguments only. Adding them is the prerequisite for making the safe options the
|
||
default in deployment.
|
||
|
||
**`pkg/restheadspec`.** `security_hooks.go:14-58` is the wiring that registers
|
||
every hook discussed here, and `handler.go:396-400`/`:424` is what guarantees
|
||
`ApplyColumnSecurity` always receives a slice (which is why the read path does not
|
||
have the single-record masking gap it first appears to have). Separately, and
|
||
unrelated to security: `handler.go:1006` passes `modelPtr` to the response writer
|
||
rather than `hookCtx.Result`, so an `AfterRead` hook that *replaces* the result has
|
||
no effect — worth noting here because a hook that redacts by substitution would
|
||
silently do nothing.
|
||
|
||
---
|
||
|
||
## Recommended order of work
|
||
|
||
The list is ordered by risk reduced per unit of effort, not by finding number.
|
||
|
||
1. **Verify passwords (finding 1).** Everything else is secondary to the fact that
|
||
`loginDirect` never reads `req.Password` and the stored procedure compares it to
|
||
nothing. Add bcrypt (or argon2id) hashing on registration and comparison on
|
||
login, in both query modes, with a migration path for existing plaintext rows.
|
||
Until this is done the package has no authentication.
|
||
2. **Make row-level security actually run, and fix the injection it enables at the
|
||
same time (findings 2 and the `GetTemplate` interpolation).** Replace the
|
||
impossible assertion at `hooks.go:134-136` with the real `common.SelectQuery`
|
||
interface, and make `GetTemplate` bind `{UserID}` as a parameter instead of
|
||
`fmt.Sprintf("%v", …)` (`provider.go:48`). These must ship together: turning the
|
||
filter on while it still interpolates its user ID into SQL would introduce a
|
||
live injection where today there is only dead code. Add an integration test that
|
||
asserts a second tenant's rows are absent from the result.
|
||
3. **Replace `CatchPanic` with `HandlePanic` on the two masking functions
|
||
(finding 3).** Two lines, and it converts "panic ⇒ unmasked data, nil error"
|
||
into "panic ⇒ error".
|
||
4. **Stop holding the global write locks across provider calls (finding 6).** Call
|
||
the provider outside the lock; take the lock only to publish the result. This is
|
||
the whole of the package's throughput problem and it is a mechanical change.
|
||
5. **Fail closed, now that failures are distinguishable (findings 5, 15 and 22).**
|
||
Change `GetRowSecurityTemplate`/`ApplyColumnSecurity` to return `(value, ok,
|
||
err)`, then deny on `err` and skip silently on `!ok`; make an unregistered model
|
||
a denial rather than a permission. Step 22's return-type change is what makes
|
||
this possible, so do it here rather than earlier.
|
||
6. **Add a redacting `String()` to `UserContext` and drop the credential log lines
|
||
(finding 7).** One method plus four log-line edits removes session tokens and
|
||
`Authorization` headers from logs, from Sentry, and from the cache keys in
|
||
finding 14.
|
||
7. **Close the four token-handling gaps (findings 9, 10, 11, 34).** Reject
|
||
multi-token `Authorization` headers instead of trying each; strip the scheme
|
||
prefix before building the logout cache key; gate or bound `/oauth/register`;
|
||
and copy `ClientSecretHash`/`TokenEndpointAuthMethod` in
|
||
`lookupOrFetchClient` so a restart stops turning confidential clients into
|
||
public ones. Finding 34 is a three-line fix for a High, so it can go first.
|
||
8. **Fix the masking primitives (findings 4 and 25).** Type-switch instead of
|
||
kind-string matching so `float*`, `bool`, `time.Time` and `[]byte` are actually
|
||
masked, guard every setter with `CanSet()`, and rewrite `maskString` on runes
|
||
with corrected bounds. Ship with a before/after table, because output changes.
|
||
9. **Build the audit log, or stop calling it one (finding 16).**
|
||
10. **The remainder** — findings 12, 13, 17, 19, 20, 21, 23, 24, 26–33 — in any
|
||
order. Findings 12 (delete or implement `JWTAuthenticator`) and 17 (call the
|
||
identifier validators) are the two that remove the most latent risk for the
|
||
least work.
|
||
|
||
Two things to preserve while doing all of this. The **OAuth 2.1 authorization-code
|
||
grant** (`oauth_server.go:794-869`) is correct: mandatory PKCE with S256 only,
|
||
exact `redirect_uri` matching, single-use codes, confidential-client
|
||
authentication, `subtle.ConstantTimeCompare` for secrets. The **password-reset
|
||
flow** (`providers_direct.go:310-407`) is correct: 256 bits from `crypto/rand`,
|
||
hash-at-rest, one-hour expiry, single use, session invalidation on completion, and
|
||
enumeration-resistant responses. Both are the standard the rest of the package
|
||
should be brought up to, and neither should be refactored casually in the course of
|
||
fixing the items above.
|