Merge pull request #541 from MerginMaps/dont_reveal_resource_to_anonymous

Don't reveal private project exists to anonymous user

Author: MarcelGeo

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

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,13 +233,18 @@ 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)
     if not is_active_workspace(workspace):
         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
 
 

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

server/mergin/sync/public_api_v2_controller.py

@@ -44,7 +44,6 @@
 from .public_api_controller import catch_sync_failure
 from .schemas import (
     ProjectMemberSchema,
-    ProjectVersionSchema,
     UploadChunkSchema,
     ProjectSchema,
 )
@@ -167,7 +166,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(

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()

server/mergin/tests/test_public_api_v2.py

@@ -11,6 +11,8 @@
     create_workspace,
     create_project,
     upload_file_to_project,
+    login,
+    file_info,
 )
 
 from ..auth.models import User
@@ -49,7 +51,6 @@
     _get_changes_with_diff_0_size,
     _get_changes_without_added,
 )
-from .utils import add_user, file_info
 
 
 def test_schedule_delete_project(client):
@@ -168,6 +169,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):
@@ -176,7 +178,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
@@ -235,6 +242,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 = [