From e457934f7c17f99e7f9ac598890a0ddb609aa558 Mon Sep 17 00:00:00 2001 From: Andrea Cecchi Date: Thu, 3 Apr 2025 13:00:41 +0200 Subject: [PATCH 1/8] fix content check in add endpoint: now can handle also paths that starts with a whitelist pattern --- base.cfg | 3 + .../feedback/restapi/services/add.py | 40 ++++----- test_plone60.cfg | 85 ++++++++----------- 3 files changed, 59 insertions(+), 69 deletions(-) diff --git a/base.cfg b/base.cfg index 80a3979..38ca0be 100644 --- a/base.cfg +++ b/base.cfg @@ -22,6 +22,7 @@ parts = develop = . +eggs-directory = eggs [instance] recipe = plone.recipe.zope2instance @@ -128,3 +129,5 @@ mode = 755 [versions] # Don't use a released version of collective.feedback collective.feedback = +setuptools = +zc.buildout = diff --git a/src/collective/feedback/restapi/services/add.py b/src/collective/feedback/restapi/services/add.py index ec2aec8..38cba73 100644 --- a/src/collective/feedback/restapi/services/add.py +++ b/src/collective/feedback/restapi/services/add.py @@ -20,6 +20,11 @@ class FeedbackAdd(Service): def reply(self): alsoProvides(self.request, IDisableCSRFProtection) + self.allowed_views = api.portal.get_registry_record( + "allowed_feedback_view", + interface=ICollectiveFeedbackSettings, + default=False, + ) form_data = json_body(self.request) self.validate_form(form_data=form_data) data = self.extract_data(form_data=form_data) @@ -55,29 +60,26 @@ def validate_form(self, form_data): if not value: raise BadRequest("Campo obbligatorio mancante: {}".format(field)) - def looks_like_path(self, string): - return bool(re.match(r"^(/|/[^\s<>:\"|?*]+.*)$", string)) + def check_allowed_views(self, value): + if value in self.allowed_views: + return True + for allowed_view in self.allowed_views: + if value.startswith(allowed_view + "/"): + return True + return False def extract_data(self, form_data): path = form_data.pop("content") - if self.looks_like_path(path): + + if self.check_allowed_views(path): + form_data.update({"title": path}) + else: portal = api.portal.get() contextual_path = "/" + portal.id + path context = api.content.get(path=contextual_path) - if not context: - raise BadRequest(f"Object with path {contextual_path} not found.") - - form_data.update({"uid": context.UID()}) - form_data.update({"title": context.Title()}) - else: - allowed_view = api.portal.get_registry_record( - "allowed_feedback_view", - interface=ICollectiveFeedbackSettings, - default=False, - ) - if path not in allowed_view: - raise BadRequest(f"View non consentita: {path}") - - form_data.update({"title": path}) - + if context: + form_data.update({"uid": context.UID()}) + form_data.update({"title": context.Title()}) + else: + raise BadRequest(f"Object with path {path} not found.") return form_data diff --git a/test_plone60.cfg b/test_plone60.cfg index aece9ad..2da4387 100644 --- a/test_plone60.cfg +++ b/test_plone60.cfg @@ -8,57 +8,23 @@ extends = update-versions-file = test_plone60.cfg [versions] -createcoverage = 1.5 -watchdog = 2.1.6 -pygments = 2.14.0 -# Added by buildout at 2023-02-03 17:48:39.187222 +# Added by buildout at 2025-04-03 12:59:02.125519 bleach = 5.0.1 +build = 1.0.3 +check-manifest = 0.49 +collective.honeypot = 2.1 commonmark = 0.9.1 coverage = 7.1.0 +createcoverage = 1.5 i18ndude = 5.3.4 keyring = 23.11.0 -pkginfo = 1.8.3 -readme-renderer = 37.3 -repoze.catalog = 0.9.0 -requests-toolbelt = 0.10.1 -rfc3986 = 2.0.0 -rich = 12.6.0 -souper = 1.1.1 -souper.plone = 1.3.1 -twine = 4.0.1 -zest.releaser = 7.2.0 -zope.index = 5.2.1 - -# Required by: -# keyring==23.11.0 -jaraco.classes = 3.2.3 - -# Required by: -# jaraco.classes==3.2.3 -more-itertools = 9.0.0 - -# Required by: -# bleach==5.0.1 -webencodings = 0.5.1 - -# Required by: -# collective.feedback==1.0a1 -z3c.jbot = 1.1.1 - -# Added by buildout at 2023-02-06 09:08:21.501543 node = 1.1 +node-ext-zodb = 1.5 odict = 1.9.0 -plumber = 1.7 - -# Required by: -# souper==1.1.1 -node.ext.zodb = 1.5 - -# Added by buildout at 2024-02-08 15:13:37.168123 -build = 1.0.3 -check-manifest = 0.49 pep440 = 0.1.2 +pkginfo = 1.8.3 +plumber = 1.7 pyproject-hooks = 1.0.0 pyroma = 4.2 pytest = 7.4.3 @@ -66,10 +32,19 @@ pytest-cov = 4.1.0 pytest-docker = 2.0.1 pytest-mock = 3.12.0 pytest-plone = 0.2.0 +readme-renderer = 37.3 +repoze.catalog = 0.9.0 requests-mock = 1.11.0 +requests-toolbelt = 0.10.1 +rfc3986 = 2.0.0 +rich = 12.6.0 +souper-plone = 1.3.1 towncrier = 23.6.0 trove-classifiers = 2023.8.7 -zestreleaser.towncrier = 1.3.0 +twine = 4.0.1 +zest.releaser = 7.2.0 +zestreleaser-towncrier = 1.3.0 +zope-index = 5.2.1 # Required by: # towncrier==23.6.0 @@ -87,12 +62,22 @@ incremental = 22.10.0 # pytest==7.4.3 iniconfig = 2.0.0 -# Added by buildout at 2024-02-08 16:09:12.800009 -collective.honeypot = 2.1 +# Required by: +# keyring==23.11.0 +jaraco.classes = 3.2.3 -# Added by buildout at 2024-02-08 21:49:12.973900 -zpretty = 3.1.0 +# Required by: +# jaraco.classes==3.2.3 +more-itertools = 9.0.0 -# Added by buildout at 2024-10-08 14:48:52.237066 -pluggy = 1.5.0 -tomli = 2.0.2 +# Required by: +# souper-plone==1.3.1 +souper = 1.1.1 + +# Required by: +# bleach==5.0.1 +webencodings = 0.5.1 + +# Required by: +# collective.honeypot==2.1 +z3c.jbot = 1.1.1 From bdb2bb095e43ddcb34370261deb17527cab7cd8e Mon Sep 17 00:00:00 2001 From: Andrea Cecchi Date: Thu, 3 Apr 2025 13:01:34 +0200 Subject: [PATCH 2/8] fix ci --- .github/workflows/black.yml | 2 +- .github/workflows/flake8.yml | 2 +- .github/workflows/pyroma.yml | 2 +- .github/workflows/test.yml | 2 +- .github/workflows/zpretty.yml | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/black.yml b/.github/workflows/black.yml index 60e9669..b5ab644 100644 --- a/.github/workflows/black.yml +++ b/.github/workflows/black.yml @@ -19,7 +19,7 @@ jobs: python-version: ${{ matrix.python-version }} # python cache - - uses: actions/cache@v1 + - uses: actions/cache@v3 with: path: ~/.cache/pip key: ${{ runner.os }}-pip-${{ hashFiles('**/requirements.txt') }} diff --git a/.github/workflows/flake8.yml b/.github/workflows/flake8.yml index 29fa34b..0ccd634 100644 --- a/.github/workflows/flake8.yml +++ b/.github/workflows/flake8.yml @@ -19,7 +19,7 @@ jobs: python-version: ${{ matrix.python-version }} # python cache - - uses: actions/cache@v1 + - uses: actions/cache@v3 with: path: ~/.cache/pip key: ${{ runner.os }}-pip-${{ hashFiles('**/requirements.txt') }} diff --git a/.github/workflows/pyroma.yml b/.github/workflows/pyroma.yml index 81bd325..ad5ad14 100644 --- a/.github/workflows/pyroma.yml +++ b/.github/workflows/pyroma.yml @@ -19,7 +19,7 @@ jobs: python-version: ${{ matrix.python-version }} # python cache - - uses: actions/cache@v1 + - uses: actions/cache@v3 with: path: ~/.cache/pip key: ${{ runner.os }}-pip-${{ hashFiles('**/requirements.txt') }} diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 28cca02..3126d9d 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -13,7 +13,7 @@ jobs: steps: - uses: actions/checkout@v1 - name: Cache eggs - uses: actions/cache@v1 + uses: actions/cache@v3 with: path: eggs key: ${{ runner.OS }}-build-python${{ matrix.python }}-${{ matrix.plone }} diff --git a/.github/workflows/zpretty.yml b/.github/workflows/zpretty.yml index 5c89129..a6c3003 100644 --- a/.github/workflows/zpretty.yml +++ b/.github/workflows/zpretty.yml @@ -19,7 +19,7 @@ jobs: python-version: ${{ matrix.python-version }} # python cache - - uses: actions/cache@v1 + - uses: actions/cache@v3 with: path: ~/.cache/pip key: ${{ runner.os }}-pip-${{ hashFiles('**/requirements.txt') }} From 6d9c71120d359ef1f7ff8035bcd4a90152912035 Mon Sep 17 00:00:00 2001 From: Andrea Cecchi Date: Thu, 3 Apr 2025 13:11:22 +0200 Subject: [PATCH 3/8] add test for startswith --- .../feedback/restapi/services/add.py | 2 - .../feedback/tests/test_feedbacks_add.py | 49 +++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/collective/feedback/restapi/services/add.py b/src/collective/feedback/restapi/services/add.py index 38cba73..b729c67 100644 --- a/src/collective/feedback/restapi/services/add.py +++ b/src/collective/feedback/restapi/services/add.py @@ -8,8 +8,6 @@ from zope.component import getUtility from zope.interface import alsoProvides -import re - class FeedbackAdd(Service): """ diff --git a/src/collective/feedback/tests/test_feedbacks_add.py b/src/collective/feedback/tests/test_feedbacks_add.py index 64cfce4..818cc42 100644 --- a/src/collective/feedback/tests/test_feedbacks_add.py +++ b/src/collective/feedback/tests/test_feedbacks_add.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- from collective.feedback.interfaces import ICollectiveFeedbackStore +from collective.feedback.controlpanels.settings import ICollectiveFeedbackSettings from collective.feedback.testing import RESTAPI_TESTING from plone import api from plone.app.testing import setRoles @@ -180,3 +181,51 @@ def test_add_feedback_to_allowed_and_disallowed_views(self): self.assertEqual(res.status_code, 400) transaction.commit() self.assertEqual(len(tool.search(query={"title": not_allowed_view})), 0) + + def test_add_feedback_to_allowed_path_starts_with(self): + + allowed_views = api.portal.get_registry_record( + "allowed_feedback_view", + interface=ICollectiveFeedbackSettings, + default=False, + ) + allowed_views.append("/my-path") + api.portal.set_registry_record( + "allowed_feedback_view", + allowed_views, + interface=ICollectiveFeedbackSettings, + ) + + transaction.commit() + + # Aggiunta di un feedback in una vista consentita + res = self.anon_api_session.post( + self.url, + json={ + "vote": 5, + "comment": "Great login experience", + "honey": "", + "content": "/my-path", + }, + ) + self.assertEqual(res.status_code, 204) + transaction.commit() + + tool = getUtility(ICollectiveFeedbackStore) + self.assertEqual(len(tool.search(query={"title": "/my-path"})), 1) + + res = self.anon_api_session.post( + self.url, + json={ + "vote": 5, + "comment": "Great admin experience", + "honey": "", + "content": "/my-path/foo", + }, + ) + + self.assertEqual(res.status_code, 204) + transaction.commit() + + tool = getUtility(ICollectiveFeedbackStore) + self.assertEqual(len(tool.search(query={"title": "/my-path/foo"})), 1) From 2849819a76344f6bf41f8d16ad1997c0b26d7531 Mon Sep 17 00:00:00 2001 From: Andrea Cecchi Date: Thu, 3 Apr 2025 13:16:44 +0200 Subject: [PATCH 4/8] Fix logic for non-content paths: now can handle also paths that startw with a pattern. --- CHANGES.rst | 3 ++- .../feedback/tests/test_feedbacks_add.py | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/CHANGES.rst b/CHANGES.rst index e854e26..6e94d85 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -5,7 +5,8 @@ Changelog 1.2.1 (unreleased) ------------------ -- Nothing changed yet. +- Fix logic for non-content paths: now can handle also paths that startw with a pattern. + [cekk] 1.2.0 (2025-02-28) diff --git a/src/collective/feedback/tests/test_feedbacks_add.py b/src/collective/feedback/tests/test_feedbacks_add.py index 818cc42..63ba203 100644 --- a/src/collective/feedback/tests/test_feedbacks_add.py +++ b/src/collective/feedback/tests/test_feedbacks_add.py @@ -229,3 +229,17 @@ def test_add_feedback_to_allowed_path_starts_with(self): tool = getUtility(ICollectiveFeedbackStore) self.assertEqual(len(tool.search(query={"title": "/my-path/foo"})), 1) + + # Aggiunta di un feedback in una vista non consentita + res = self.anon_api_session.post( + self.url, + json={ + "vote": 5, + "comment": "Great admin experience", + "honey": "", + "content": "/my-pathh", + }, + ) + self.assertEqual(res.status_code, 400) + transaction.commit() + self.assertEqual(len(tool.search(query={"title": "/my-pathh"})), 0) From 73a44f00dba70dc9cff19df2c862c47013b60287 Mon Sep 17 00:00:00 2001 From: Andrea Cecchi Date: Mon, 7 Apr 2025 10:21:12 +0200 Subject: [PATCH 5/8] Fix serialization of contextless votes --- src/collective/feedback/restapi/services/__init__.py | 5 +++++ src/collective/feedback/restapi/services/add.py | 3 ++- src/collective/feedback/restapi/services/get.py | 12 ++++++++++-- src/collective/feedback/tests/test_feedbacks_add.py | 3 +-- 4 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/collective/feedback/restapi/services/__init__.py b/src/collective/feedback/restapi/services/__init__.py index e69de29..64d6cf6 100644 --- a/src/collective/feedback/restapi/services/__init__.py +++ b/src/collective/feedback/restapi/services/__init__.py @@ -0,0 +1,5 @@ +import re + + +def looks_like_path(string): + return bool(re.match(r"^(/|/[^\s<>:\"|?*]+.*)$", string)) diff --git a/src/collective/feedback/restapi/services/add.py b/src/collective/feedback/restapi/services/add.py index b729c67..b7304fb 100644 --- a/src/collective/feedback/restapi/services/add.py +++ b/src/collective/feedback/restapi/services/add.py @@ -1,5 +1,6 @@ from collective.feedback.controlpanels.settings import ICollectiveFeedbackSettings from collective.feedback.interfaces import ICollectiveFeedbackStore +from collective.feedback.restapi.services import looks_like_path from plone import api from plone.protect.interfaces import IDisableCSRFProtection from plone.restapi.deserializer import json_body @@ -62,7 +63,7 @@ def check_allowed_views(self, value): if value in self.allowed_views: return True for allowed_view in self.allowed_views: - if value.startswith(allowed_view + "/"): + if looks_like_path(allowed_view) and value.startswith(allowed_view): return True return False diff --git a/src/collective/feedback/restapi/services/get.py b/src/collective/feedback/restapi/services/get.py index 8761f86..bd366d0 100644 --- a/src/collective/feedback/restapi/services/get.py +++ b/src/collective/feedback/restapi/services/get.py @@ -1,5 +1,6 @@ from AccessControl import Unauthorized from collective.feedback.interfaces import ICollectiveFeedbackStore +from collective.feedback.restapi.services import looks_like_path from copy import deepcopy from datetime import datetime from plone import api @@ -201,18 +202,25 @@ def get_data(self): # only manager can list deleted object's reviews continue + title = feedback._attrs.get("title", "") new_data = { "vote_num": 0, "vote_sum": 0, "comments": 0, - "title": feedback._attrs.get("title", ""), "uid": uid, + "title": title, } if obj: new_data["title"] = obj.Title() new_data["url"] = obj.absolute_url() - + else: + # it's a contextless feedback + if looks_like_path(title): + fixed_title = title.rstrip("/").rsplit("/", 1)[-1] + fixed_title = fixed_title.replace("-", " ").capitalize() + new_data["title"] = fixed_title + new_data["url"] = title feedbacks[uid] = new_data # vote avg diff --git a/src/collective/feedback/tests/test_feedbacks_add.py b/src/collective/feedback/tests/test_feedbacks_add.py index 63ba203..e7b4174 100644 --- a/src/collective/feedback/tests/test_feedbacks_add.py +++ b/src/collective/feedback/tests/test_feedbacks_add.py @@ -1,6 +1,6 @@ # -*- coding: utf-8 -*- -from collective.feedback.interfaces import ICollectiveFeedbackStore from collective.feedback.controlpanels.settings import ICollectiveFeedbackSettings +from collective.feedback.interfaces import ICollectiveFeedbackStore from collective.feedback.testing import RESTAPI_TESTING from plone import api from plone.app.testing import setRoles @@ -183,7 +183,6 @@ def test_add_feedback_to_allowed_and_disallowed_views(self): self.assertEqual(len(tool.search(query={"title": not_allowed_view})), 0) def test_add_feedback_to_allowed_path_starts_with(self): - allowed_views = api.portal.get_registry_record( "allowed_feedback_view", interface=ICollectiveFeedbackSettings, From 0834388c80ee00496d78740edac9db9c649c751f Mon Sep 17 00:00:00 2001 From: Andrea Cecchi Date: Mon, 7 Apr 2025 10:51:02 +0200 Subject: [PATCH 6/8] fix test --- src/collective/feedback/restapi/services/add.py | 8 ++++++-- test_plone60.cfg | 17 +++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/collective/feedback/restapi/services/add.py b/src/collective/feedback/restapi/services/add.py index b7304fb..15c4047 100644 --- a/src/collective/feedback/restapi/services/add.py +++ b/src/collective/feedback/restapi/services/add.py @@ -63,8 +63,12 @@ def check_allowed_views(self, value): if value in self.allowed_views: return True for allowed_view in self.allowed_views: - if looks_like_path(allowed_view) and value.startswith(allowed_view): - return True + if looks_like_path(allowed_view): + if allowed_view.endswith("/"): + check_path = allowed_view + else: + check_path = allowed_view + "/" + return value.startswith(check_path) return False def extract_data(self, form_data): diff --git a/test_plone60.cfg b/test_plone60.cfg index 2da4387..cb9cd20 100644 --- a/test_plone60.cfg +++ b/test_plone60.cfg @@ -81,3 +81,20 @@ webencodings = 0.5.1 # Required by: # collective.honeypot==2.1 z3c.jbot = 1.1.1 + +# Added by buildout at 2025-04-07 10:24:51.519798 +node-ext-zodb = 1.5 +souper-plone = 1.3.1 +zestreleaser-towncrier = 1.3.0 +zope-index = 5.2.1 + +# Added by buildout at 2025-04-07 10:38:19.587549 +pluggy = 1.5.0 +souper-plone = 1.3.1 +zestreleaser-towncrier = 1.3.0 +zope.index = 7.0 +zpretty = 3.1.0 + +# Required by: +# souper==1.1.1 +node.ext.zodb = 1.6 From dfaa20298e4499fc3b9a79aee701d682488dc240 Mon Sep 17 00:00:00 2001 From: Andrea Cecchi Date: Mon, 7 Apr 2025 10:54:38 +0200 Subject: [PATCH 7/8] refactored get_data to decrease complexity --- .../feedback/restapi/services/get.py | 78 +++++++++++-------- 1 file changed, 44 insertions(+), 34 deletions(-) diff --git a/src/collective/feedback/restapi/services/get.py b/src/collective/feedback/restapi/services/get.py index bd366d0..41f973e 100644 --- a/src/collective/feedback/restapi/services/get.py +++ b/src/collective/feedback/restapi/services/get.py @@ -187,40 +187,10 @@ def get_data(self): vote = feedback._attrs.get("vote", "") if uid not in feedbacks: - try: - uuid.UUID(uid) - valid_uuid = True - except ValueError: - valid_uuid = False - - obj = None - if valid_uuid: - obj = self.get_commented_obj(uid=uid) - if not obj and not api.user.has_permission( - "collective.feedback: Show Deleted Feedbacks" - ): - # only manager can list deleted object's reviews - continue - - title = feedback._attrs.get("title", "") - new_data = { - "vote_num": 0, - "vote_sum": 0, - "comments": 0, - "uid": uid, - "title": title, - } - - if obj: - new_data["title"] = obj.Title() - new_data["url"] = obj.absolute_url() - else: - # it's a contextless feedback - if looks_like_path(title): - fixed_title = title.rstrip("/").rsplit("/", 1)[-1] - fixed_title = fixed_title.replace("-", " ").capitalize() - new_data["title"] = fixed_title - new_data["url"] = title + new_data = self.get_new_data(uid=uid, feedback=feedback) + if not new_data: + # no data, skip + continue feedbacks[uid] = new_data # vote avg @@ -271,6 +241,46 @@ def get_data(self): return self.sort_result(result) + def get_new_data(self, uid, feedback): + """ + Generate data for feedback entry + """ + try: + uuid.UUID(uid) + valid_uuid = True + except ValueError: + valid_uuid = False + + obj = None + if valid_uuid: + obj = self.get_commented_obj(uid=uid) + if not obj and not api.user.has_permission( + "collective.feedback: Show Deleted Feedbacks" + ): + # only manager can list deleted object's reviews + return None + + title = feedback._attrs.get("title", "") + new_data = { + "vote_num": 0, + "vote_sum": 0, + "comments": 0, + "uid": uid, + "title": title, + } + + if obj: + new_data["title"] = obj.Title() + new_data["url"] = obj.absolute_url() + else: + # it's a contextless feedback + if looks_like_path(title): + fixed_title = title.rstrip("/").rsplit("/", 1)[-1] + fixed_title = fixed_title.replace("-", " ").capitalize() + new_data["title"] = fixed_title + new_data["url"] = title + return new_data + class FeedbackGetCSV(FeedbackGet): """Service for getting feedbacks as csv file""" From 462e9b2d8a6ad0b93f785c10a27707834c7bde62 Mon Sep 17 00:00:00 2001 From: Andrea Cecchi Date: Mon, 7 Apr 2025 11:22:08 +0200 Subject: [PATCH 8/8] avoid double check Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/collective/feedback/restapi/services/add.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/collective/feedback/restapi/services/add.py b/src/collective/feedback/restapi/services/add.py index 15c4047..6b2e90c 100644 --- a/src/collective/feedback/restapi/services/add.py +++ b/src/collective/feedback/restapi/services/add.py @@ -60,9 +60,9 @@ def validate_form(self, form_data): raise BadRequest("Campo obbligatorio mancante: {}".format(field)) def check_allowed_views(self, value): - if value in self.allowed_views: - return True for allowed_view in self.allowed_views: + if value == allowed_view: + return True if looks_like_path(allowed_view): if allowed_view.endswith("/"): check_path = allowed_view