Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion src/imio/esign/browser/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
from plone import api
from plone.app.registry.browser.controlpanel import ControlPanelFormWrapper
from plone.app.registry.browser.controlpanel import RegistryEditForm
from plone.registry.interfaces import IRecordModifiedEvent
from plone.z3cform import layout
from string import Formatter
from zope import schema
from zope.interface import Interface
from zope.interface import Invalid
Expand Down Expand Up @@ -39,6 +39,17 @@ def validate_vat_number(va_nb):
return True


def validate_signing_users_email_content(value):
"""Allow only known placeholders in the email template."""
allowed = {"fullname", "firstname", "lastname", "email", "userid"}
for _ignored1, field, _ignored2, _ignored3 in Formatter().parse(value or ""):
if field and field not in allowed:
raise Invalid(
_("Unknown placeholder: ${field}", mapping={"field": field})
)
return True
Copy link

@coderabbitai coderabbitai bot Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify that Formatter().parse() raises ValueError on malformed format strings
python3 - <<'PY'
from string import Formatter

samples = ["Hello {name", "Hello }", "Hello {name}}"]
for s in samples:
    try:
        list(Formatter().parse(s))
        print("OK:", s)
    except Exception as e:
        print(type(e).__name__, "for", repr(s))
PY

Repository: IMIO/imio.esign

Length of output: 143


🏁 Script executed:

# Check the actual file to confirm code location and implementation
if [ -f "src/imio/esign/browser/settings.py" ]; then
  wc -l src/imio/esign/browser/settings.py
  echo "---"
  sed -n '40,55p' src/imio/esign/browser/settings.py
else
  echo "File not found at expected location. Searching..."
  find . -name "settings.py" -path "*/esign/browser/*" 2>/dev/null
fi

Repository: IMIO/imio.esign

Length of output: 612


Handle malformed templates to avoid uncaught ValueError.

Formatter().parse() raises ValueError on malformed format strings (e.g., unmatched braces). When used as a schema constraint, this will surface as a server error instead of a validation error. Catch it and re‑raise Invalid with a friendly message.

🔧 Suggested fix
 def validate_signing_users_email_content(value):
     """Allow only known placeholders in the email template."""
     allowed = {"fullname", "firstname", "lastname", "email", "userid"}
-    for _ignored1, field, _ignored2, _ignored3 in Formatter().parse(value or ""):
-        if field and field not in allowed:
-            raise Invalid(
-                _("Unknown placeholder: ${field}", mapping={"field": field})
-            )
+    try:
+        for _ignored1, field, _ignored2, _ignored3 in Formatter().parse(value or ""):
+            if field and field not in allowed:
+                raise Invalid(
+                    _("Unknown placeholder: ${field}", mapping={"field": field})
+                )
+    except ValueError as exc:
+        raise Invalid(_("Invalid email template format.")) from exc
     return True
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def validate_signing_users_email_content(value):
"""Allow only known placeholders in the email template."""
allowed = {"fullname", "firstname", "lastname", "email", "userid"}
for _ignored1, field, _ignored2, _ignored3 in Formatter().parse(value or ""):
if field and field not in allowed:
raise Invalid(
_("Unknown placeholder: ${field}", mapping={"field": field})
)
return True
def validate_signing_users_email_content(value):
"""Allow only known placeholders in the email template."""
allowed = {"fullname", "firstname", "lastname", "email", "userid"}
try:
for _ignored1, field, _ignored2, _ignored3 in Formatter().parse(value or ""):
if field and field not in allowed:
raise Invalid(
_("Unknown placeholder: ${field}", mapping={"field": field})
)
except ValueError as exc:
raise Invalid(_("Invalid email template format.")) from exc
return True
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/imio/esign/browser/settings.py` around lines 42 - 50, The
validate_signing_users_email_content validator currently calls
Formatter().parse(value) which can raise ValueError on malformed format strings;
wrap the parse loop in a try/except that catches ValueError and re-raises an
Invalid with a clear message (e.g. "Malformed template" or similar) so malformed
templates surface as validation errors; keep the existing behavior of checking
placeholders against the allowed set
{"fullname","firstname","lastname","email","userid"} and raising Invalid for
unknown placeholders inside validate_signing_users_email_content.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine because this is a setting that will only be edited by the support team or developers, not end users

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chris-adam, understood! I've noted that for admin-only configuration settings in this project, verbose error handling for malformed input is not required.

(\\_/)
(•_•)
/>📝 Noted for future reference!

✏️ Learnings added
Learnt from: chris-adam
Repo: IMIO/imio.esign PR: 6
File: src/imio/esign/browser/settings.py:42-50
Timestamp: 2026-02-17T12:37:05.819Z
Learning: In the imio.esign project, for configuration settings that are only edited by the support team or developers (not end users), it's acceptable to allow technical errors like ValueError to surface as server errors rather than wrapping them in user-friendly validation messages.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.



class IImioEsignSettings(Interface):

enabled = schema.Bool(
Expand Down Expand Up @@ -78,6 +89,16 @@ class IImioEsignSettings(Interface):
required=False,
)

signing_users_email_content = schema.Text(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tu peux en faire un champ riche (comme omail_email_signature de nos settings) et utiliser du tal pour rendre les variables dynamiques. Cela permettra de mettre en forme certaines choses si nécessaire.

title=_("Email content for signing users"),
description=_(
"Email content sent to users when inviting them to Parapheo. "
"Use {fullname}, {firstname}, {lastname}, {email}, {userid} as placeholders."
),
required=False,
constraint=validate_signing_users_email_content,
)


class ImioEsignSettings(RegistryEditForm):
schema = IImioEsignSettings
Expand Down
Loading