From 96f8d89442b32e2a2f6c220e474772bdebedd1d0 Mon Sep 17 00:00:00 2001 From: Tom Flux Date: Tue, 11 Aug 2026 21:52:38 +0100 Subject: [PATCH] Fix silent login loop caused by strict cookie parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The admin UI accepted the password, issued a valid session cookie, then bounced straight back to the login page with no error shown. http.cookies.SimpleCookie.load() silently discards the remainder of a Cookie header the moment it meets a value it considers illegal, rather than raising or skipping just that entry. A neighbouring cookie with a JSON-ish value such as prefs={"a":1} is enough. Everything after it — including our session — becomes invisible, so a perfectly good login looked like a failed one. curl never showed it because curl only sends the one cookie. A browser sends every cookie on the domain, and tube.jihakuz.xyz previously served TubeArchivist alongside several sibling services on jihakuz.xyz. Replaced with a tolerant hand-rolled parser and covered it with regression tests for nine hostile neighbour values. Also log successful logins at INFO. Only failures were logged, which made "password rejected" and "session did not stick" indistinguishable from the journal and sent the diagnosis down the wrong path. Co-Authored-By: Claude Opus 5 (1M context) --- tests/test_auth.py | 57 ++++++++++++++++++++++++++++++++++ youtube_automate/web/auth.py | 18 +++++++++++ youtube_automate/web/server.py | 19 +++++------- 3 files changed, 83 insertions(+), 11 deletions(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index 1e4ac5a..e47f4c6 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -2,6 +2,8 @@ import time +import pytest + from youtube_automate.web import auth @@ -91,6 +93,61 @@ class TestCookie: assert "Max-Age=0" in auth.clear_cookie_header() +class TestCookieParsing: + """Regression tests for a real failure: http.cookies.SimpleCookie silently + drops everything after a value it dislikes, which made valid sessions + invisible and bounced users back to the login page with no error.""" + + def test_finds_our_cookie_alone(self): + assert auth.cookie_value("yta_session=abc") == "abc" + + def test_finds_it_after_a_neighbour(self): + assert auth.cookie_value("sessionid=xyz; yta_session=abc") == "abc" + + def test_finds_it_before_a_neighbour(self): + assert auth.cookie_value("yta_session=abc; sessionid=xyz") == "abc" + + @pytest.mark.parametrize( + "neighbour", + [ + 'prefs={"a":1}', # JSON value — what actually broke it + "junk=[1,2,3]", + "weird=a b c", + "empty=", + "novalue", + "quoted=\"has spaces\"", + "path=/a/b/c", + "colons=a:b:c", + "comma=a,b", + ], + ) + def test_survives_hostile_neighbours(self, neighbour): + assert auth.cookie_value(f"{neighbour}; yta_session=abc") == "abc" + assert auth.cookie_value(f"yta_session=abc; {neighbour}") == "abc" + + def test_strips_surrounding_quotes(self): + assert auth.cookie_value('yta_session="abc"') == "abc" + + def test_tolerates_whitespace(self): + assert auth.cookie_value(" yta_session = abc ") == "abc" + + def test_absent_cookie_returns_empty(self): + assert auth.cookie_value("sessionid=xyz") == "" + + def test_empty_header_returns_empty(self): + assert auth.cookie_value("") == "" + assert auth.cookie_value(None) == "" + + def test_does_not_match_a_name_that_merely_contains_ours(self): + assert auth.cookie_value("not_yta_session=nope") == "" + + def test_real_token_round_trips_through_the_header(self): + secret = auth.new_secret() + token = auth.issue_session(secret) + header = f'prefs={{"theme":"dark"}}; yta_session={token}; other=1' + assert auth.verify_session(secret, auth.cookie_value(header)) + + class TestCsrf: def test_token_verifies(self): secret, session = auth.new_secret(), auth.issue_session(auth.new_secret()) diff --git a/youtube_automate/web/auth.py b/youtube_automate/web/auth.py index 67df83c..7c5fe13 100644 --- a/youtube_automate/web/auth.py +++ b/youtube_automate/web/auth.py @@ -129,6 +129,24 @@ def clear_cookie_header() -> str: return f"{COOKIE_NAME}=; Path=/; HttpOnly; SameSite=Lax; Max-Age=0" +def cookie_value(header: str, name: str = COOKIE_NAME) -> str: + """Pull one cookie out of a request's Cookie header. + + Deliberately hand-rolled rather than using http.cookies.SimpleCookie. That + parser silently discards the remainder of the header the moment it meets a + value it considers illegal — a JSON-ish value such as `prefs={"a":1}` is + enough — so any cookie appearing after it becomes invisible. A browser sends + us every cookie on the domain, including ones set by unrelated services, so + one stray value would otherwise make a perfectly valid session vanish and + bounce the user back to the login page with no error shown. + """ + for part in (header or "").split(";"): + candidate, separator, value = part.strip().partition("=") + if separator and candidate.strip() == name: + return value.strip().strip('"') + return "" + + # -------------------------------------------------------------------------- # CSRF diff --git a/youtube_automate/web/server.py b/youtube_automate/web/server.py index 4f3e852..ea80505 100644 --- a/youtube_automate/web/server.py +++ b/youtube_automate/web/server.py @@ -8,7 +8,6 @@ throttling. from __future__ import annotations -import http.cookies import json import logging import subprocess @@ -89,16 +88,7 @@ class Handler(BaseHTTPRequestHandler): } def _cookie_token(self) -> str: - header = self.headers.get("Cookie") - if not header: - return "" - jar = http.cookies.SimpleCookie() - try: - jar.load(header) - except http.cookies.CookieError: - return "" - morsel = jar.get(auth.COOKIE_NAME) - return morsel.value if morsel else "" + return auth.cookie_value(self.headers.get("Cookie") or "") # ------------------------------------------------------------- session @@ -237,8 +227,15 @@ class Handler(BaseHTTPRequestHandler): ) stored = settings.raw("admin_password_hash") + if not stored: + log.warning("login attempted from %s but no password is set", key) + if stored and auth.verify_password(stored, form.get("password", "")): self.server.throttle.record_success(key) + # Logged at INFO so "did my login work?" is answerable from the + # journal. A rejected login only ever re-renders the form, which + # looks identical to a session that failed to stick. + log.info("successful login from %s", key) token = auth.issue_session(self._secret(settings)) return self._redirect( "/",