diff --git a/securedrop/journalist_app/__init__.py b/securedrop/journalist_app/__init__.py index 1ebee836f4d..a2fa3478b3d 100644 --- a/securedrop/journalist_app/__init__.py +++ b/securedrop/journalist_app/__init__.py @@ -5,7 +5,7 @@ from pathlib import Path from datetime import datetime from flask import (Flask, redirect, url_for, flash, g, request, - render_template, json, abort, session) + render_template, json, abort) from flask_assets import Environment from flask_babel import gettext from flask_wtf.csrf import CSRFProtect, CSRFError @@ -29,8 +29,7 @@ from journalist_app import account, admin, api, main, col from journalist_app.sessions import Session, session from journalist_app.utils import get_source -from models import InstanceConfig, Journalist -from werkzeug.exceptions import default_exceptions +from models import InstanceConfig # https://www.python.org/dev/peps/pep-0484/#runtime-or-type-checking if typing.TYPE_CHECKING: diff --git a/securedrop/journalist_app/admin.py b/securedrop/journalist_app/admin.py index 25ddb44759d..5db35ad4fe0 100644 --- a/securedrop/journalist_app/admin.py +++ b/securedrop/journalist_app/admin.py @@ -26,8 +26,9 @@ ) from flask_babel import gettext from journalist_app.decorators import admin_required +from journalist_app.sessions import logout_user from journalist_app.utils import (commit_account_changes, set_diceware_password, - validate_hotp_secret, logout_user) + validate_hotp_secret) from journalist_app.forms import LogoForm, NewUserForm, SubmissionPreferencesForm, OrgNameForm from sdconfig import SDConfig from passphrases import PassphraseGenerator @@ -345,7 +346,7 @@ def delete_user(user_id: int) -> werkzeug.Response: abort(403) else: user.delete() - logout_user(user.id) + logout_user(user.id, is_current=False) db.session.commit() flash(gettext("Deleted user '{user}'.").format(user=user.username), "notification") @@ -361,7 +362,7 @@ def new_password(user_id: int) -> werkzeug.Response: password = request.form.get('password') if set_diceware_password(user, password) is not False: - logout_user(user.id) + logout_user(user.id, is_current=False) db.session.commit() return redirect(url_for("admin.edit_user", user_id=user_id)) diff --git a/securedrop/journalist_app/api.py b/securedrop/journalist_app/api.py index 4fd50c005db..24f22090cd4 100644 --- a/securedrop/journalist_app/api.py +++ b/securedrop/journalist_app/api.py @@ -5,7 +5,7 @@ import flask import werkzeug -from flask import abort, Blueprint, jsonify, request, g +from flask import abort, Blueprint, jsonify, request from sqlalchemy import Column from sqlalchemy.exc import IntegrityError diff --git a/securedrop/journalist_app/decorators.py b/securedrop/journalist_app/decorators.py index 4f8268f162a..6b9da2c58f7 100644 --- a/securedrop/journalist_app/decorators.py +++ b/securedrop/journalist_app/decorators.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- from typing import Any -from flask import redirect, url_for, flash, g, session +from flask import redirect, url_for, flash, session from flask_babel import gettext from functools import wraps from typing import Any, Callable diff --git a/securedrop/journalist_app/sessions.py b/securedrop/journalist_app/sessions.py index 067fe984830..f26eb826f63 100644 --- a/securedrop/journalist_app/sessions.py +++ b/securedrop/journalist_app/sessions.py @@ -23,6 +23,9 @@ def on_update(self: ServerSideSession) -> None: if initial and 'uid' in initial: self.set_uid(initial['uid']) self.set_user() + else: + self.uid = None + self.user = None CallbackDict.__init__(self, initial, on_update) self.sid = sid self.token: str = token @@ -32,7 +35,6 @@ def on_update(self: ServerSideSession) -> None: self.to_regenerate = False self.modified = False - def get_token(self) -> Optional[str]: return self.token @@ -40,28 +42,32 @@ def get_lifetime(self) -> datetime: return datetime.now(timezone.utc) + timedelta(seconds=self.lifetime) def set_user(self) -> None: - if hasattr(self, 'uid') and self.uid is not None: + if self.uid is not None: self.user = Journalist.query.get(self.uid) + if self.user is None: + # The uid has no match in the database, and this should really not happen + self.uid = None + self.to_destroy = True def get_user(self) -> Optional[Journalist]: - if hasattr(self, 'user'): - return self.user + return self.user def get_uid(self) -> Optional[int]: - if hasattr(self, 'uid'): - return self.uid + return self.uid - def set_uid(self, uid) -> None: + def set_uid(self, uid: int) -> None: + self.user = None self.uid = uid def logged_in(self) -> bool: - if hasattr(self, 'uid') and self.uid is not None: + if self.uid is not None: return True else: return False def destroy(self) -> None: self.uid = None + self.user = None self.to_destroy = True def regenerate(self) -> None: @@ -150,6 +156,8 @@ def open_session(self, app: Flask, request: Request) -> Optional[ServerSideSessi initial=data) except (JSONDecodeError, NotImplementedError): return self._new_session(is_api) + # signed session_id provided in cookie is valid, but the session is not on the server + # anymore so maybe here is the code path for a meaningful error message return self._new_session(is_api) def save_session( # type: ignore[override] # noqa @@ -188,9 +196,14 @@ def save_session( # type: ignore[override] # noqa self.redis.delete(self.key_prefix + session.sid) session.sid = self._generate_sid() session.token = self._get_signer(app).dumps(session.sid) # type: ignore - if session.modified or session.new: + if session.new or session.to_regenerate: self.redis.setex(name=self.key_prefix + session.sid, value=val, time=expires) + elif session.modified: + # To prevent race conditions where session is delete by an admin in the middle of a req + # accept to save the session object if and only if alrady exists using the xx flag + self.redis.set(name=self.key_prefix + session.sid, value=val, + ex=expires, xx=True) if not session.is_api and (session.new or session.to_regenerate): response.headers.add('Vary', 'Cookie') response.set_cookie(app.session_cookie_name, session.token, @@ -228,6 +241,25 @@ def _get_interface(self, app: Flask) -> SessionInterface: return session_interface +def logout_user(uid: int, is_current: bool = True) -> None: + redis = Redis() + for key in (redis.keys(app.config['SESSION_KEY_PREFIX'] + "*") + + redis.keys("api_" + app.config['SESSION_KEY_PREFIX'] + "*")): + found = redis.get(key) + if found: + sess = session_json_serializer.loads(found.decode()) + if 'uid' in sess and sess['uid'] == uid: + redis.delete(key) + if is_current: + session.pop('uid') + + +def logout_all() -> None: + redis = Redis() + for key in (redis.keys(app.config['SESSION_KEY_PREFIX'] + "*") + + redis.keys("api_" + app.config['SESSION_KEY_PREFIX'] + "*")): + redis.delete(key) + # Re-export flask.session, but with the correct type information for mypy. from flask import session # noqa session = typing.cast(ServerSideSession, session) diff --git a/securedrop/journalist_app/utils.py b/securedrop/journalist_app/utils.py index e978d9b861d..850f17b327f 100644 --- a/securedrop/journalist_app/utils.py +++ b/securedrop/journalist_app/utils.py @@ -5,12 +5,10 @@ import flask import werkzeug -from flask import (g, flash, current_app, abort, send_file, redirect, url_for, +from flask import (flash, current_app, abort, send_file, redirect, url_for, Markup, escape, session) -from flask.sessions import session_json_serializer from flask_babel import gettext, ngettext from sqlalchemy.exc import IntegrityError -from redis import Redis from db import db from encryption import EncryptionManager @@ -529,22 +527,3 @@ def serve_file_with_etag(db_obj: Union[Reply, Submission]) -> flask.Response: response.direct_passthrough = False response.headers["Etag"] = db_obj.checksum return response - - -def logout_user(uid: int) -> None: - redis = Redis() - for key in (redis.keys(current_app.config['SESSION_KEY_PREFIX'] + "*") + - redis.keys("api_" + current_app.config['SESSION_KEY_PREFIX'] + "*")): - found = redis.get(key) - if found: - sess = session_json_serializer.loads(found.decode()) - if 'uid' in sess and sess['uid'] == uid: - redis.delete(key) - session.pop('uid') - - -def logout_all() -> None: - redis = Redis() - for key in (redis.keys(current_app.config['SESSION_KEY_PREFIX'] + "*") + - redis.keys("api_" + current_app.config['SESSION_KEY_PREFIX'] + "*")): - redis.delete(key) diff --git a/securedrop/tests/test_integration.py b/securedrop/tests/test_integration.py index 8220a946d90..93ee2df9ab5 100644 --- a/securedrop/tests/test_integration.py +++ b/securedrop/tests/test_integration.py @@ -8,6 +8,11 @@ from base64 import b32encode from binascii import unhexlify from io import BytesIO +import mock + +from bs4 import BeautifulSoup +from flask import escape, session +from pyotp import HOTP, TOTP import journalist_app as journalist_app_module import mock diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index 8b48cf236f4..6e11bed6bce 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -68,7 +68,7 @@ def _login_user(app, username, password, otp_secret, success=True): follow_redirects=True, ) assert resp.status_code == 200 - assert username in resp.data.decode('utf-8') + assert ((username in resp.data.decode('utf-8')) == success) @pytest.mark.parametrize("otp_secret", ["", "GA", "GARBAGE", "JHCOGO7VCER3EJ4"]) @@ -705,7 +705,6 @@ def test_user_edits_password_success_response(config, journalist_app, test_journ follow_redirects=True, ) - assert page_language(resp.data) == language_tag(locale) msgids = [ "Password updated. Don't forget to save it in your KeePassX database. New password:" ] @@ -744,7 +743,7 @@ def test_user_edits_password_expires_session(journalist_app, test_journo): ins.assert_redirects(resp, url_for("main.login")) # verify the session was expired after the password was changed - assert "uid" not in session + assert (session.uid is None and session.user is None) finally: models.LOGIN_HARDENING = original_hardening diff --git a/securedrop/tests/test_journalist_session.py b/securedrop/tests/test_journalist_session.py index 541095bdbf5..65cd2597064 100644 --- a/securedrop/tests/test_journalist_session.py +++ b/securedrop/tests/test_journalist_session.py @@ -4,7 +4,7 @@ from datetime import datetime, timedelta, timezone import re -from flask import url_for +from flask import url_for, Response from flask.sessions import session_json_serializer from redis import Redis from pyotp import TOTP @@ -282,3 +282,24 @@ def test_session_bad_signature(journalist_app, test_journo): headers=get_api_headers(token)) assert resp.status_code == 200 assert resp.json['uuid'] == test_journo['uuid'] + + +def test_session_race_condition(mocker, journalist_app, test_journo): + with journalist_app.test_request_context() as app: + session = journalist_app.session_interface.open_session(journalist_app, app.request) + assert session.sid is not None + session['uid'] = test_journo['id'] + app.response = Response() + journalist_app.session_interface.save_session(journalist_app, session, app.response) + assert redis.get(journalist_app.config['SESSION_KEY_PREFIX'] + session.sid) is not None + app.request.cookies = {journalist_app.config['SESSION_COOKIE_NAME']: session.token} + session2 = journalist_app.session_interface.open_session(journalist_app, app.request) + assert session2.sid == session.sid + assert session2['uid'] == test_journo['id'] + # Force entering the redis set xx case + session.modified = True + session.new = False + session.to_regenerate = False + redis.delete(journalist_app.config['SESSION_KEY_PREFIX'] + session.sid) + journalist_app.session_interface.save_session(journalist_app, session, app.response) + assert redis.get(journalist_app.config['SESSION_KEY_PREFIX'] + session.sid) is None diff --git a/securedrop/tests/utils/__init__.py b/securedrop/tests/utils/__init__.py index ed6cd5491d6..464b5681241 100644 --- a/securedrop/tests/utils/__init__.py +++ b/securedrop/tests/utils/__init__.py @@ -1,6 +1,5 @@ # -*- coding: utf-8 -*- -from flask import g from pyotp import TOTP from . import asynchronous # noqa