Hook re-authentication silently erases admin-configured per-user access Rules

Status

Repository / Component

Plain-English Description

When an external login hook is used, logging back in can quietly wipe the custom access restrictions an administrator set on that user account. A rule meant to block the user from certain files can silently disappear, giving the user more access than intended.

Description of the Underlying Issue

On the existing-user 're-auth' path of HookAuth (auth/hook.go:187-204), the code rebuilds the user via GetUser(u) (auth/hook.go:210-244), which assigns every field EXCEPT Rules (and AceEditorTheme), then calls Users.Update(u) with no field list. The bolt backend (storage/bolt/users.go:58-61) treats an empty field list as a full-record Save, overwriting the stored user. Because Clean (users/users.go:86-89) normalizes the missing Rules to an empty slice, any previously admin-configured per-user Rules are erased. validHookFields (auth/hook.go:252-271) has no 'user.rules' entry, confirming Rules is meant to be preserved rather than hook-managed — the overwrite is unintended.

Potential Attack

This is primarily an unsafe fail-open behavior rather than a directed exploit. Under hook auth, a user whose account carries an admin-set DENY rule simply logs in again in a way that trips the re-auth branch — the branch runs whenever the hook returns more than one field OR the submitted password differs from the stored hash (auth/hook.go:187-204). The re-auth silently drops the deny rule. Since Check (http/data.go:29-48) applies user rules last, the previously blocked path is now allowed on the next request.

Outcomes of Potential Attack

Loss of an access-control restriction: a per-user DENY rule that confined the user away from specific paths is removed, expanding the user's effective access (fail-open). No credential or privilege escalation beyond the erased rule; impact is scoped to whatever the deleted rule protected.

Affected Scope

HookAuth.GetUser/SaveUser existing-user 'auth' path (auth/hook.go:187-244); per-user access Rules of hook-managed users

Suggested Fix (plain english)

Preserve each user's custom access rules across hook re-authentication instead of overwriting the whole record, and add a test that confirms the rules survive a re-login.

Suggested Fix (detailed)

In auth/hook.go GetUser, copy the existing user's Rules (d.Rules) into the rebuilt user so they are not dropped; alternatively, in the re-auth branch call Users.Update with an explicit field list that excludes 'rules' so the fieldless full-overwrite path (storage/bolt/users.go:58-61) is never taken. Add a regression test that sets a DENY rule on a hook-managed user, performs a hook re-authentication (hook returns an extra field or a changed password), and asserts the rule is still present on the stored record and still enforced by http/data.go Check. Preserve User.Rules across hook re-authentication: copy d.Rules into the rebuilt user in GetUser, or call Users.Update with an explicit field list that excludes Rules so the fieldless full-overwrite path is not taken. Add a regression test asserting Rules survive a hook re-auth.

Validation

Canonical of the hook re-auth pair. The silent erasure of admin-configured per-user Rules is fully proven in source and corroborated by peer review; the security escalation (deny-rule removal) is gated on hook auth being the configured method and on deny rules actually being set on a hook user — deployment/config preconditions not settleable from source. possibly_valid with those named.

first_opinion

Full Evidence

auth/hook.go:210-244 - GetUser constructs a fresh users.User assigning every field EXCEPT Rules (and AceEditorTheme)

auth/hook.go:187-204 - existing-user 'auth' branch runs when 'len(a.Fields.Values) > 1 || p' (p = password mismatch): u = a.GetUser(u); a.Users.Update(u) called with no field names

auth/hook.go:252-271 - validHookFields has no 'user.rules' entry, so the hook cannot set Rules (the field is meant to be preserved, not hook-managed)

users/storage.go:76-91 - Update calls Clean("", fields...) then back.Update(user, fields...); storage/bolt/users.go:58-61 - Update with len(fields)==0 falls through to st.Save (full-record overwrite)

users/users.go:86-89 - Clean sets Rules nil->[] (empty slice)

http/data.go:29-48 - Check applies settings.Rules then user.Rules, with user rules applied last so a user deny rule (Allow=false) overrides and blocks access

Proven fact: On the existing-user 'auth' re-authentication path, u is replaced by GetUser(u) which omits Rules, and Users.Update(u) is called with no field list, which the bolt backend turns into a full Save that overwrites the stored record.

Proven fact: Clean normalizes the missing Rules to an empty slice, so any previously admin-configured per-user Rules are silently erased.

Proven fact: The trigger is broad: the branch runs whenever the hook returns more than one field OR the submitted password differs from the stored hash.

Proven fact: Wiping a user DENY rule removes an access restriction (Check applies user rules last), expanding the user's effective access (fail-open).

Unvalidated fact: Whether the deployment's AuthMethod is 'hook' (this path only executes under hook auth) — a runtime configuration value not settleable from source.

Unvalidated fact: Whether an admin has actually configured per-user DENY Rules on a hook-managed user; if only allow rules were set, erasure merely restricts access (fail-safe) rather than escalating it.

auth/hook.go:210-244 — GetUser constructs a fresh users.User assigning every field EXCEPT Rules (and AceEditorTheme).

auth/hook.go:187-204 — existing-user 'auth' branch runs when 'len(a.Fields.Values) > 1 || p' (p = password mismatch): u = a.GetUser(u); a.Users.Update(u) called with no field names.

auth/hook.go:252-271 — validHookFields has no 'user.rules' entry, so the hook cannot set Rules; the field is meant to be preserved, not hook-managed.

users/storage.go:76-91 — Update calls Clean("", fields...) then back.Update(user, fields...); storage/bolt/users.go:58-61 — Update with len(fields)==0 falls through to st.Save (full-record overwrite).

users/users.go:86-89 — Clean sets Rules nil->[] (empty slice).

http/data.go:29-48 — Check applies settings.Rules then user.Rules, with user rules applied last, so a user deny rule (Allow=false) overrides and blocks access; removing it re-opens the path.

Unvalidated fact: Whether the deployment's AuthMethod is 'hook' — this path only executes under hook auth; a runtime configuration value not settleable from source.

auth/hook.go:210-244 GetUser rebuilds users.User field-by-field and omits Rules (and AceEditorTheme); every other field assigned

auth/hook.go:187-204 existing-user 'auth' branch: guard len(a.Fields.Values)>1 || p; u=a.GetUser(u) at :188; a.Users.Update(u) with no fields at :200

auth/hook.go:252-271 validHookFields has no 'user.rules' key → hook cannot set Rules, so the field is meant to be preserved

auth/hook.go:70-75 'pass' action returns loaded user untouched (no SaveUser) — wipe is specific to the 'auth' existing-user path

users/storage.go:76-91 Update calls Clean("") then back.Update with fields... ; storage/bolt/users.go:58-61 Update with len(fields)==0 does full st.Save overwrite

http/data.go:41-45 Check enforces d.user.Rules (allow/deny) on every path access