FelisMC/Felis
2
0 Fork 0
Code Issues Pull Requests 6 Actions Packages Projects Releases Wiki Activity

fix(api): require reauthentication for changes to own email and passkeys; update OpenAPI descriptions

Browse Source
This commit is contained in:
flyemoji committed 2026-09-29 13:39:35 +09:00
1 parent f7a826d635
commit 3922aae9a7
6 files changed
+149 -15
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

deploy/bootstrap.sh
+10 -8
View File
@@ -3438,16 +3438,18 @@ atomic_install_file() {
mv -fT "$staged" "$target"
}
# install_if_changed is atomic_install_file that leaves a target with the same bytes in
# place, fixing only its mode and owner, so the target's mtime keeps meaning "the content
# changed". felis domain check reads a proxy started before felis-link.properties' mtime
# as one still on the old names, and a re-run that rewrote the same bytes made every
# install look behind (and `felis domain set` restart the proxy for nothing).
# install_if_changed is atomic_install_file that leaves the target alone when it already
# has the same bytes, owner and mode, so its mtime keeps meaning "the content changed".
# felis domain check reads a proxy started before felis-link.properties' mtime as one
# still on the old names, and a re-run that rewrote the same bytes made every install look
# behind (and `felis domain set` restart the proxy for nothing).
# It never fixes a target in place: the proxy's account owns these directories and can
# swap the file for a symlink after the checks, and a chown or chmod by path would follow
# it to, say, k3s.yaml. Owner and group compare by name, as the callers pass them.
install_if_changed() {
local source="$1" target="$2" mode="$3" owner="$4" group="$5"
if [ -f "$target" ] && [ ! -L "$target" ] && cmp -s "$source" "$target"; then
chown "${owner}:${group}" "$target"
chmod "$mode" "$target"
if [ -f "$target" ] && [ ! -L "$target" ] && cmp -s "$source" "$target" \
&& [ "$(stat -c '%U:%G %a' "$target")" = "${owner}:${group} ${mode#0}" ]; then
return 0
fi
atomic_install_file "$@"
deploy/bootstrap_test.sh
+40 -3
View File
@@ -2064,7 +2064,8 @@ else
fi
# A re-run that writes the same felis-link.properties leaves the file alone: felis domain
# check reads a proxy started before the file's mtime as still on the old names.
# check reads a proxy started before the file's mtime as still on the old names. stat answers
# as for the file the first install left, root:v 0640, which the test's user cannot make.
run_link() { # velocity-dir [root-domain]
VD="$1" RD="${2:-r.example.com}" TMPDIR="$1" FNFILE="$fnfile" bash -c '
set -Eeuo pipefail
@@ -2073,6 +2074,7 @@ run_link() { # velocity-dir [root-domain]
prepare_velocity_layout() { :; }
atomic_install_file() { echo "REPLACED $(basename "$2")"; cp "$1" "$2"; }
chown() { echo "CHOWN $*"; }; chmod() { echo "CHMOD $*"; }
stat() { echo "root:v 640"; }
. "$FNFILE"
STATE_DIR="$VD" FELIS_ROOT_DOMAIN="$RD" FORWARDING_SECRET=f SERVICE_TOKEN=t LOGIN_SERVER=login \
LOBBY_SERVER=lobby FELIS_GAME_PORT=25565 VELOCITY_DIR="$VD" VELOCITY_USER=v NODE_IP=10.0.0.5
@@ -2089,8 +2091,10 @@ case "$out" in
*"REPLACED felis-link.properties"*) echo "FAIL: a re-run with the same names replaced felis-link.properties"; fails=$((fails + 1)) ;;
*) echo "PASS a re-run with the same names leaves felis-link.properties in place" ;;
esac
expect "the re-run still fixes the owner" "CHOWN root:v $lprops" "$out"
expect "the re-run still fixes the mode" "CHMOD 0640 $lprops" "$out"
case "$out" in
*CHOWN*|*CHMOD*) echo "FAIL: the re-run changed felis-link.properties by path:"; printf '%s\n' "$out"; fails=$((fails + 1)) ;;
*) echo "PASS the re-run changes nothing by path" ;;
esac
expect "the kept file keeps its mtime" "$before" "$(ls -l --time-style=+%s "$lprops" 2>/dev/null || stat -f '%m' "$lprops")"
expect "a re-run on other names replaces felis-link.properties" "REPLACED felis-link.properties" "$(run_link "$ldir2" other.example.net)"
expect "the replaced file has the new root domain" "root-domain=other.example.net" "$(grep '^root-domain=' "$lprops")"
@@ -4493,6 +4497,39 @@ expect "from FELIS_ARTIFACT_DIR it stops the install" "DIE: FELIS_ARTIFACT_DIR:
expect "a source build builds the plugin" "ENSURE
BUILD" "$(run_plugin "" 0)"
# --- install_if_changed never fixes a target in place -----------------------------------------
# The proxy's account owns the directories these files land in and can swap one for a symlink
# after the checks, so a chown or chmod by path would land on whatever the link names. The cmp
# stub makes that swap right after the content check; chown and chmod report every call.
iicblock="$(bsfn install_if_changed)"
[ -n "$iicblock" ] || { echo "FAIL: no install_if_changed in $BS"; exit 1; }
mkdir "$adir/iic"
printf 'plugin\n' > "$adir/iic/src"
printf 'not the plugin\n' > "$adir/iic/decoy"
chmod 600 "$adir/iic/decoy"
mine="$(stat -c '%U:%G' "$adir/iic/src")"
run_iic() { # target's mode, the owner:group asked for, then "edited" or "swapped"
rm -f "$adir/iic/dst"
if [ "${3-}" = edited ]; then printf 'an older plugin\n' > "$adir/iic/dst"; else cp "$adir/iic/src" "$adir/iic/dst"; fi
chmod "$1" "$adir/iic/dst"
D="$adir/iic" OG="$2" HOW="${3-}" bash -c '
atomic_install_file() { echo "ATOMIC $2"; }
chown() { echo "CHOWN $*"; command chown "$@"; }
chmod() { echo "CHMOD $*"; command chmod "$@"; }
cmp() { command cmp "$@" || return; [ "$HOW" != swapped ] || ln -sfn "$D/decoy" "$3"; }
'"$iicblock"'
install_if_changed "$D/src" "$D/dst" 0644 "${OG%%:*}" "${OG#*:}"' 2>&1
}
iic_is() { # label want got
[ "$3" = "$2" ] && echo "PASS $1" || { printf 'FAIL %s: got\n%s\nwant\n%s\n' "$1" "$3" "$2"; fails=$((fails + 1)); }
}
iic_is "the same bytes, owner and mode are left alone" "" "$(run_iic 644 "$mine")"
iic_is "the same bytes with the wrong mode are reinstalled" "ATOMIC $adir/iic/dst" "$(run_iic 600 "$mine")"
iic_is "the same bytes with the wrong owner are reinstalled" "ATOMIC $adir/iic/dst" "$(run_iic 644 "felis-nobody:${mine#*:}")"
iic_is "new bytes are installed" "ATOMIC $adir/iic/dst" "$(run_iic 644 "$mine" edited)"
iic_is "a target swapped for a symlink after the checks is reinstalled" "ATOMIC $adir/iic/dst" "$(run_iic 644 "$mine" swapped)"
iic_is "and the file the link named keeps its mode" 600 "$(stat -c %a "$adir/iic/decoy")"
# --- k3s's own images from its GitHub release ----------------------------------------------------
kablock="$(bsfn stage_k3s_airgap_images)"
mkdir -p "$adir/k3simg" "$adir/k3srel"
docs/openapi.yaml
+6 -2
View File
@@ -5864,7 +5864,7 @@ paths:
$ref: '#/components/responses/Unauthorized'
'403':
description: >-
Not an owner (forbidden); a change to the caller's own role (self_protected); or a role change on the owner account (owner_protected), which only the host's break-glass console (sudo felis breakGlass) may make.
Not an owner (forbidden); a change to the caller's own role (self_protected); a role change on the owner account (owner_protected), which only the host's break-glass console (sudo felis breakGlass) may make; or a change to the caller's own email without a reauth in the last 5 minutes (reauth_required).
content:
application/json:
schema: { $ref: '#/components/schemas/Error' }
@@ -6136,7 +6136,11 @@ paths:
'401':
$ref: '#/components/responses/Unauthorized'
'403':
$ref: '#/components/responses/Forbidden'
description: >-
Not an owner (forbidden); or unbinding the caller's own passkeys without a reauth in the last 5 minutes (reauth_required).
content:
application/json:
schema: { $ref: '#/components/schemas/Error' }
/api/v1/users/{id}/links:
post:
internal/api/handlers_users.go
+10
View File
@@ -175,6 +175,11 @@ func (a *API) handlePatchUser(w http.ResponseWriter, r *http.Request) {
"role must be 'admin' or 'user', got %q", *body.Role))
return
}
// A new address unverifies the old one, and with it the email factor that
// guards adding a passkey: your own takes the same reauth as /account/email.
if body.Email != nil && id == p.UserID && !a.requireReauth(w, r, p) {
return
}
u, err := a.Repo.UpdateUser(r.Context(), id, UpdateUserInput(body), p.Email)
if err != nil {
@@ -431,6 +436,11 @@ func (a *API) handleUnbindUserPasskeys(w http.ResponseWriter, r *http.Request) {
writeError(w, r, errBadRequest)
return
}
// Your own passkeys are a factor that guards adding one: severing them takes
// the same reauth as removing one under /account/passkey.
if p := principalFromContext(r.Context()); id == p.UserID && !a.requireReauth(w, r, p) {
return
}
if err := a.Repo.DeleteAllPasskeyCredentialsForUser(r.Context(), id); err != nil {
writeError(w, r, err)
internal/api/handlers_users_test.go
+73
View File
@@ -1,7 +1,9 @@
package api
import (
"context"
"net/http"
"net/http/httptest"
"strings"
"testing"
)
@@ -83,6 +85,77 @@ func TestOwnerAccountProtectedFromPanelMutations(t *testing.T) {
})
}
// The caller's own email and passkeys are ways into the caller's account, so
// changing them through /users/{id} takes the same recent reauth as through
// /account. Without it a stolen owner session could strip both here, and with no
// factor left to guard, /account/passkey/register/begin would let it plant its own.
func TestOwnSignInFactorsNeedReauthUnderUsers(t *testing.T) {
owner := &Principal{UserID: "usr-root", Role: "owner", Email: "[email protected]",
ViaAdminAccess: true, ViaSession: true, EmailVerified: true}
repo := newFakeRepo()
repo.seedUser(UserView{ID: "usr-root", Username: "root", Email: "[email protected]", Role: "owner", EmailVerified: true})
repo.seedUser(UserView{ID: "u2", Username: "alice", Email: "[email protected]", Role: "user"})
repo.passkeyCreds["pk-root"] = PasskeyCredential{ID: "pk-root", UserID: "usr-root", CredentialID: "c-root", UserVerified: true, CreatedAt: frozenNow}
api := newTestAPI(repo, newFakeCluster())
api.Mailer = &captureMailer{}
api.External = staticExternal{p: owner}
eh := api.ExternalHandler()
send := func(method, path, body string) *httptest.ResponseRecorder {
if body == "" {
return do(eh, method, path, "", nil)
}
return do(eh, method, path, body, jsonHeader)
}
own := []struct {
name, method, path, body string
untouched func() bool
}{
{"own email", "PATCH", "/api/v1/users/usr-root", `{"email":"[email protected]"}`, func() bool {
d, err := repo.UserDetail(context.Background(), "usr-root")
return err == nil && d.Email == "[email protected]"
}},
{"own passkeys", "DELETE", "/api/v1/users/usr-root/passkeys", "", func() bool {
_, ok := repo.passkeyCreds["pk-root"]
return ok
}},
}
for _, tc := range own {
t.Run(tc.name+" refused without a recent reauth", func(t *testing.T) {
w := send(tc.method, tc.path, tc.body)
if w.Code != http.StatusForbidden || decodeErr(t, w) != "reauth_required" {
t.Fatalf("code = %d body %s, want 403 reauth_required", w.Code, w.Body.String())
}
if !tc.untouched() {
t.Fatal("the change went through despite the refusal")
}
})
}
// Another account's factors are the owner's to manage, and a username is no way in.
for _, tc := range []struct{ name, method, path, body string }{
{"another user's email", "PATCH", "/api/v1/users/u2", `{"email":"[email protected]"}`},
{"another user's passkeys", "DELETE", "/api/v1/users/u2/passkeys", ""},
{"own username", "PATCH", "/api/v1/users/usr-root", `{"username":"root2"}`},
} {
t.Run("control: "+tc.name+" needs no reauth", func(t *testing.T) {
if w := send(tc.method, tc.path, tc.body); w.Code != http.StatusOK {
t.Fatalf("code = %d body %s, want 200", w.Code, w.Body.String())
}
})
}
owner.ReauthAt = api.now()
for _, tc := range own {
t.Run(tc.name+" allowed after a reauth", func(t *testing.T) {
if w := send(tc.method, tc.path, tc.body); w.Code != http.StatusOK {
t.Fatalf("code = %d body %s, want 200", w.Code, w.Body.String())
}
if tc.untouched() {
t.Fatal("the change did not go through")
}
})
}
}
// The user-scoped admin sub-resources (quotas, account links) must answer 404
// for an unknown user id. Before the requireLiveUser guard the quota upsert and
// the link insert reached the users(id) foreign key and surfaced as an opaque
panel/src/lib/openapi.gen.ts
+10 -2
View File
@@ -8280,7 +8280,7 @@ export interface operations {
};
400: components["responses"]["BadRequest"];
401: components["responses"]["Unauthorized"];
/** @description Not an owner (forbidden); a change to the caller's own role (self_protected); or a role change on the owner account (owner_protected), which only the host's break-glass console (sudo felis breakGlass) may make. */
/** @description Not an owner (forbidden); a change to the caller's own role (self_protected); a role change on the owner account (owner_protected), which only the host's break-glass console (sudo felis breakGlass) may make; or a change to the caller's own email without a reauth in the last 5 minutes (reauth_required). */
403: {
headers: {
[name: string]: unknown;
@@ -8541,7 +8541,15 @@ export interface operations {
};
};
401: components["responses"]["Unauthorized"];
403: components["responses"]["Forbidden"];
/** @description Not an owner (forbidden); or unbinding the caller's own passkeys without a reauth in the last 5 minutes (reauth_required). */
403: {
headers: {
[name: string]: unknown;
};
content: {
"application/json": components["schemas"]["Error"];
};
};
};
};
linkAccount: {