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( "/",