Skip to content

Fix /settings/update clobbering subscriptions and filters cookies - #583

Open
ppjsw wants to merge 1 commit into
redlib-org:mainfrom
ppjsw:fix/settings-update-cookie-clobber
Open

ppjsw wants to merge 1 commit into
redlib-org:mainfrom
ppjsw:fix/settings-update-cookie-clobber

Conversation

@ppjsw

@ppjsw ppjsw commented Oct 9, 2026

Copy link
Copy Markdown

What

/settings/update (the "quick update one preference" endpoint) silently deletes the user's subscriptions and filters cookies on every call, unless those two fields happen to be in the query string. Clicking Enable HLS (?use_hls=on), disable this notification (?hide_hls_notification=on) or the NSFW bypass this gate link (?show_nsfw=on) wipes the whole subscription list with no warning.

Root cause

src/settings.rs, set_cookies_method(req, remove_cookies). update() passes false ("don't clear settings that were not passed"), restore() passes true. The PREFS loop honours the flag:

None => {
    if remove_cookies {
        response.remove_cookie(name.to_string());
    }
}

but the subscriptions and filters blocks below it did not — their else branches called remove_cookie(...) unconditionally (and the while loops inside those branches removed the numbered subscriptionsN / filtersN cookies unconditionally too). This PR gates both whole else blocks on the existing flag, which covers the unnumbered and numbered deletion calls with two one-line changes.

POST /settings is a separate handler (settings::set) and is untouched. /settings/restore still passes true, so it keeps clearing absent cookies.

Verification

Method: execution. A full cargo build of redlib was not feasible here (it pulls wreq → boring-sys2, building BoringSSL from source, on aarch64, inside a tight maintenance window). Instead I compiled a small harness that include!s the verbatim set_cookies_method body extracted from this file (sed -n '103,264p'), plus the real ResponseExt::insert_cookie/remove_cookie and join_until_size_limit, against the real cookie/url/time crates. Only the HTTP transport types are substituted with http::Request/http::Response; the cookie logic under test is byte-for-byte the repo source (base a4d36e954cf1bd64f209cd8868c5a29edc81b374).

Before (main, base SHA) — Cookie: subscriptions=selfhosted+Fedora+linux; filters=spam:

--- /settings/update?use_hls=on&redirect=/r/selfhosted ---
set-cookie: use_hls=on; HttpOnly; Path=/; Expires=Fri, 08 Oct 2027 ...
set-cookie: subscriptions=; HttpOnly; Path=/; Expires=Fri, 09 Oct 2026 ...   <-- deletion
set-cookie: filters=; HttpOnly; Path=/; Expires=Fri, 09 Oct 2026 ...         <-- deletion

Same for ?show_nsfw=on and ?hide_hls_notification=on.

After (this branch) — identical input:

--- /settings/update?use_hls=on&redirect=/r/selfhosted ---
set-cookie: use_hls=on; HttpOnly; Path=/; Expires=Fri, 08 Oct 2027 ...

No subscriptions= / filters= deletion, including when numbered subscriptions1/filters1 cookies are present in the request.

Regressions checked (after):

  • /settings/restore (remove_cookies = true), subs/filters absent → still emits subscriptions=, subscriptions1=, filters=, filters1= deletions. ✅
  • /settings/update with subscriptions=linux&filters=spam in the query → still sets them and cleans up stale numbered cookies. ✅

rustfmt --edition 2021 --check src/settings.rs passes (matches the repo's cargo fmt --all -- --check CI job).

Not verified: the change was not exercised against a running redlib server binary (build infeasible here); the harness executes the exact function source, not the assembled HTTP server.

Fixes #562

set_cookies_method(req, false) means "only touch preferences named in the
query string", and the PREFS loop honours that via the remove_cookies flag.
The subscriptions and filters else-branches did not, so any /settings/update
request (e.g. the "Enable HLS" link, ?use_hls=on) emitted deletion cookies
for subscriptions and filters and silently wiped the user's lists.

Gate both else branches on remove_cookies. /settings/restore still passes
true and keeps clearing absent cookies; POST /settings is a separate handler
and is unchanged.

Fixes redlib-org#562
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐛 Bug Report: /settings/update silently deletes all subscriptions and filters

1 participant