diff --git a/docker/services/uwsgi/uwsgi.d/40_log.ini b/docker/services/uwsgi/uwsgi.d/40_log.ini index f2a1b9cc1b..34ff84a63e 100644 --- a/docker/services/uwsgi/uwsgi.d/40_log.ini +++ b/docker/services/uwsgi/uwsgi.d/40_log.ini @@ -1,4 +1,4 @@ [uwsgi] -log-format = [uwsgi] %(var.HTTP_X_FORWARDED_FOR) (%(addr)) - - [%(ltime)] "%(method) %(uri) %(proto)" %(status) %(size) "%(referer)" "%(uagent)" host_hdr=%(host) req_time_elapsed=%(msecs) process=%(pid) worker=%(wid) +log-format = [uwsgi] %(var.HTTP_X_FORWARDED_FOR) (%(addr)) - %(REMOTE_USER) [%(ltime)] "%(method) %(uri) %(proto)" %(status) %(size) "%(referer)" "%(uagent)" host_hdr=%(host) req_time_elapsed=%(msecs) process=%(pid) worker=%(wid) logger = stdio: logger = file:/var/log/uwsgi/uwsgi.log diff --git a/src/palace/manager/api/controller/base.py b/src/palace/manager/api/controller/base.py index c02c7fb976..6bb6db171c 100644 --- a/src/palace/manager/api/controller/base.py +++ b/src/palace/manager/api/controller/base.py @@ -1,3 +1,5 @@ +import re + import flask from flask import Response from flask_babel import lazy_gettext as _ @@ -10,6 +12,11 @@ from palace.manager.util.log import LoggerMixin from palace.manager.util.problem_detail import BaseProblemDetailException, ProblemDetail +try: + import uwsgi +except ImportError: + uwsgi = None + class BaseCirculationManagerController(LoggerMixin): """Define minimal standards for a circulation manager controller, @@ -65,10 +72,23 @@ def authenticated_patron_from_request(self) -> ProblemDetail | Patron | Response # No credentials were provided. return self.authenticate() + # Set REMOTE_USER for uwsgi access log visibility. We set this early, before + # authenticating the patron, so that we have the username on authentication failures. + if auth.type.lower() == "basic" and auth.username: + self._set_logvar("REMOTE_USER", auth.username) + patron = self.authenticated_patron(auth) if isinstance(patron, Patron): setattr(flask.request, "patron", patron) + # Set REMOTE_USER for non-basic auth flows, using the patron's + # authorization identifier to provide detail in access logs. + if ( + not flask.request.environ.get("REMOTE_USER") + and patron.authorization_identifier + ): + self._set_logvar("REMOTE_USER", patron.authorization_identifier) + return patron def authenticated_patron( @@ -102,6 +122,24 @@ def authenticate(self) -> Response: data = self.manager.authentication_for_opds_document return Response(data, 401, headers) + @staticmethod + def _set_logvar(key: str, value: str) -> None: + """Set a uwsgi log variable and the WSGI environ entry. + + The value is sanitized to prevent log injection — uwsgi does not + escape logvar values, so control characters and whitespace that + could split or corrupt log lines are replaced. + """ + # Replace control characters with underscores to prevent log + # injection (newlines split lines, tabs shift fields). + sanitized = re.sub(r"[\x00-\x1f\x7f]", "_", value) + # Quote values containing spaces to preserve log field boundaries. + if " " in sanitized: + sanitized = f'"{sanitized}"' + flask.request.environ[key] = sanitized + if uwsgi: + uwsgi.set_logvar(key, sanitized) + def library_for_request( self, library_short_name: str | None ) -> Library | ProblemDetail: diff --git a/tests/manager/api/controller/test_base.py b/tests/manager/api/controller/test_base.py index b21d51a987..58f7d0ace3 100644 --- a/tests/manager/api/controller/test_base.py +++ b/tests/manager/api/controller/test_base.py @@ -138,6 +138,48 @@ def test_authenticated_patron_from_request( assert result.status_code == 401 assert getattr(flask.request, "patron") is None + def test_authenticated_patron_from_request_sets_remote_user_basic_auth( + self, circulation_fixture: CirculationControllerFixture + ): + """REMOTE_USER is set from basic auth credentials, even on auth failure.""" + # Successful basic auth sets REMOTE_USER to the username. + with circulation_fixture.request_context_with_library( + "/", headers=dict(Authorization=circulation_fixture.valid_auth) + ): + result = circulation_fixture.controller.authenticated_patron_from_request() + assert isinstance(result, Patron) + assert flask.request.environ["REMOTE_USER"] == "unittestuser" + + # Failed basic auth still sets REMOTE_USER to the submitted username. + with circulation_fixture.request_context_with_library( + "/", + headers=dict(Authorization=circulation_fixture.invalid_auth), + ): + result = circulation_fixture.controller.authenticated_patron_from_request() + assert not isinstance(result, Patron) + assert flask.request.environ["REMOTE_USER"] == "user1" + + def test_authenticated_patron_from_request_sets_remote_user_non_basic_auth( + self, circulation_fixture: CirculationControllerFixture + ): + """REMOTE_USER is set from patron.authorization_identifier for non-basic auth.""" + patron = circulation_fixture.default_patron + patron.authorization_identifier = "patron-auth-id" + + with ( + patch.object( + circulation_fixture.manager.auth, + "authenticated_patron", + return_value=patron, + ), + circulation_fixture.request_context_with_library( + "/", headers=dict(Authorization="Bearer some-token") + ), + ): + result = circulation_fixture.controller.authenticated_patron_from_request() + assert result == patron + assert flask.request.environ["REMOTE_USER"] == "patron-auth-id" + def test_authenticated_patron_invalid_credentials( self, circulation_fixture: CirculationControllerFixture ):