diff --git a/server/.test.env b/server/.test.env index bdaa7bfa..63294a3f 100644 --- a/server/.test.env +++ b/server/.test.env @@ -24,3 +24,4 @@ SECURITY_BEARER_SALT='bearer' SECURITY_EMAIL_SALT='email' SECURITY_PASSWORD_SALT='password' DIAGNOSTIC_LOGS_DIR=/tmp/diagnostic_logs +GEVENT_WORKER=0 \ No newline at end of file diff --git a/server/mergin/sync/permissions.py b/server/mergin/sync/permissions.py index 4305a15f..7dd042d5 100644 --- a/server/mergin/sync/permissions.py +++ b/server/mergin/sync/permissions.py @@ -209,7 +209,21 @@ def require_project(ws, project_name, permission) -> Project: return project -def require_project_by_uuid(uuid: str, permission: ProjectPermissions, scheduled=False): +def require_project_by_uuid( + uuid: str, permission: ProjectPermissions, scheduled=False, expose=True +) -> Project: + """ + Retrieves a project by UUID after validating existence, workspace status, and permissions. + + Args: + uuid (str): The unique identifier of the project. + permission (ProjectPermissions): The permission level required to access the project. + scheduled (bool, optional): If ``True``, bypasses the check for projects marked for deletion. + expose (bool, optional): Controls security disclosure behavior on permission failure. + - If `True`: Returns 403 Forbidden (reveals project exists but access is denied). + - If `False`: Returns 404 Not Found (hides project existence for security). + Standard is that reading results in 404, while writing results in 403 + """ if not is_valid_uuid(uuid): abort(404) @@ -219,6 +233,10 @@ def require_project_by_uuid(uuid: str, permission: ProjectPermissions, scheduled if not scheduled: project = project.filter(Project.removed_at.is_(None)) project = project.first_or_404() + if not expose and current_user.is_anonymous and not project.public: + # we don't want to tell anonymous user if a private project exists + abort(404) + workspace = project.workspace if not workspace: abort(404) @@ -226,6 +244,7 @@ def require_project_by_uuid(uuid: str, permission: ProjectPermissions, scheduled abort(404, "Workspace doesn't exist") if not permission.check(project, current_user): abort(403, "You do not have permissions for this project") + return project diff --git a/server/mergin/sync/public_api_v2.yaml b/server/mergin/sync/public_api_v2.yaml index bf3db007..c1c74f68 100644 --- a/server/mergin/sync/public_api_v2.yaml +++ b/server/mergin/sync/public_api_v2.yaml @@ -87,8 +87,7 @@ paths: description: Include list of files at specific version required: false schema: - type: string - example: v3 + $ref: "#/components/schemas/VersionName" responses: "200": description: Success @@ -305,9 +304,7 @@ paths: default: false example: true version: - type: string - pattern: '^$|^v\d+$' - example: v2 + $ref: "#/components/schemas/VersionName" changes: type: object required: @@ -849,3 +846,7 @@ components: - editor - writer - owner + VersionName: + type: string + pattern: '^$|^v\d+$' + example: v2 diff --git a/server/mergin/sync/public_api_v2_controller.py b/server/mergin/sync/public_api_v2_controller.py index 27e0355a..67b75820 100644 --- a/server/mergin/sync/public_api_v2_controller.py +++ b/server/mergin/sync/public_api_v2_controller.py @@ -42,7 +42,6 @@ from .public_api_controller import catch_sync_failure from .schemas import ( ProjectMemberSchema, - ProjectVersionSchema, UploadChunkSchema, ProjectSchema, ) @@ -165,7 +164,7 @@ def remove_project_collaborator(id, user_id): def get_project(id, files_at_version=None): """Get project info. Include list of files at specific version if requested.""" - project = require_project_by_uuid(id, ProjectPermissions.Read) + project = require_project_by_uuid(id, ProjectPermissions.Read, expose=False) data = ProjectSchemaV2().dump(project) if files_at_version: pv = ProjectVersion.query.filter_by( diff --git a/server/mergin/tests/test_middleware.py b/server/mergin/tests/test_middleware.py index 82b9cf26..2f5cbe4f 100644 --- a/server/mergin/tests/test_middleware.py +++ b/server/mergin/tests/test_middleware.py @@ -6,6 +6,7 @@ import psycogreen.gevent import pytest import sqlalchemy +from unittest.mock import patch from ..app import create_simple_app, GeventTimeoutMiddleware, db from ..config import Configuration @@ -14,58 +15,74 @@ @pytest.mark.parametrize("use_middleware", [True, False]) def test_use_middleware(use_middleware): """Test using middleware""" - Configuration.GEVENT_WORKER = use_middleware - Configuration.GEVENT_REQUEST_TIMEOUT = 1 - application = create_simple_app() + with patch.object( + Configuration, + "GEVENT_WORKER", + use_middleware, + ), patch.object( + Configuration, + "GEVENT_REQUEST_TIMEOUT", + 1, + ): + application = create_simple_app() - def ping(): - gevent.sleep(Configuration.GEVENT_REQUEST_TIMEOUT + 1) - return "pong" + def ping(): + gevent.sleep(Configuration.GEVENT_REQUEST_TIMEOUT + 1) + return "pong" - application.add_url_rule("/test", "ping", ping) - app_context = application.app_context() - app_context.push() + application.add_url_rule("/test", "ping", ping) + app_context = application.app_context() + app_context.push() - assert isinstance(application.wsgi_app, GeventTimeoutMiddleware) == use_middleware - # in case of gevent, dummy endpoint it set to time out - assert application.test_client().get("/test").status_code == ( - 504 if use_middleware else 200 - ) + assert ( + isinstance(application.wsgi_app, GeventTimeoutMiddleware) == use_middleware + ) + # in case of gevent, dummy endpoint it set to time out + assert application.test_client().get("/test").status_code == ( + 504 if use_middleware else 200 + ) def test_catch_timeout(): """Test proper handling of gevent timeout with db.session.rollback""" psycogreen.gevent.patch_psycopg() - Configuration.GEVENT_WORKER = True - Configuration.GEVENT_REQUEST_TIMEOUT = 1 - application = create_simple_app() + with patch.object( + Configuration, + "GEVENT_WORKER", + True, + ), patch.object( + Configuration, + "GEVENT_REQUEST_TIMEOUT", + 1, + ): + application = create_simple_app() - def unhandled(): - try: - db.session.execute("SELECT pg_sleep(1.1);") - finally: - db.session.execute("SELECT 1;") - return "" + def unhandled(): + try: + db.session.execute("SELECT pg_sleep(1.1);") + finally: + db.session.execute("SELECT 1;") + return "" - def timeout(): - try: - db.session.execute("SELECT pg_sleep(1.1);") - except gevent.timeout.Timeout: - db.session.rollback() - raise - finally: - db.session.execute("SELECT 1;") - return "" + def timeout(): + try: + db.session.execute("SELECT pg_sleep(1.1);") + except gevent.timeout.Timeout: + db.session.rollback() + raise + finally: + db.session.execute("SELECT 1;") + return "" - application.add_url_rule("/unhandled", "unhandled", unhandled) - application.add_url_rule("/timeout", "timeout", timeout) - app_context = application.app_context() - app_context.push() + application.add_url_rule("/unhandled", "unhandled", unhandled) + application.add_url_rule("/timeout", "timeout", timeout) + app_context = application.app_context() + app_context.push() - assert application.test_client().get("/timeout").status_code == 504 + assert application.test_client().get("/timeout").status_code == 504 - # in case of missing rollback sqlalchemy would raise error - with pytest.raises(sqlalchemy.exc.PendingRollbackError): - application.test_client().get("/unhandled") + # in case of missing rollback sqlalchemy would raise error + with pytest.raises(sqlalchemy.exc.PendingRollbackError): + application.test_client().get("/unhandled") - db.session.rollback() + db.session.rollback() diff --git a/server/mergin/tests/test_public_api_v2.py b/server/mergin/tests/test_public_api_v2.py index dda0bc53..b890f0b9 100644 --- a/server/mergin/tests/test_public_api_v2.py +++ b/server/mergin/tests/test_public_api_v2.py @@ -10,6 +10,8 @@ create_workspace, create_project, upload_file_to_project, + login, + file_info, ) from ..auth.models import User @@ -47,7 +49,6 @@ _get_changes_with_diff_0_size, _get_changes_without_added, ) -from .utils import add_user, file_info def test_schedule_delete_project(client): @@ -166,6 +167,7 @@ def test_project_members(client): # access provided by workspace role cannot be removed directly response = client.delete(url + f"/{user.id}") assert response.status_code == 404 + Configuration.GLOBAL_READ = 0 def test_get_project(client): @@ -174,7 +176,12 @@ def test_get_project(client): test_workspace = create_workspace() project = create_project("new_project", test_workspace, admin) logout(client) + # anonymous user cannot access the private resource + response = client.get(f"v2/projects/{project.id}") + assert response.status_code == 404 # lack of permissions + user = add_user("tests", "tests") + login(client, user.username, "tests") response = client.get(f"v2/projects/{project.id}") assert response.status_code == 403 # access public project @@ -233,6 +240,9 @@ def test_get_project(client): ) assert len(response.json["files"]) == 3 assert {f["path"] for f in response.json["files"]} == set(files) + # invalid version format parameter + response = client.get(f"v2/projects/{project.id}?files_at_version=3") + assert response.status_code == 400 push_data = [