#9347 log method parens

[glyph]

Scope and purpose

Fixes #9347

[glyph]

please review

[adiroiban]

Thanks for the fix.
I think that this can be merged, as it's better than trunk.

The comments about naming stuff are minor.

I did a quick check of the current automated tests and I don't see any tests for accessing private members.

Not sure if we should allow or deny access to private members.

I would say that formatting should only allow acces to public members.

[adiroiban]

It looks like there is no protection for private access

with the following diff

diff --git a/src/twisted/logger/test/test_format.py b/src/twisted/logger/test/test_format.py
index 209ccca7fd..dce73291b6 100644
--- a/src/twisted/logger/test/test_format.py
+++ b/src/twisted/logger/test/test_format.py
@@ -83,11 +83,15 @@ class FormattingTests(unittest.TestCase):
         """
 
         class World:
+            _priv = 'priv'
             def where(self) -> str:
                 return "world"
+            def _private(self) -> str:
+                return "private"
+
 
         self.assertEqual(
-            "hello world", self.format("hello {what.where()}", what=World())
+            "hello world", self.format("hello {what.where()} {what._priv.upper()} {what._private()}", what=World())
         )

I get this test fialure... and it looks like Twisted is happy to format the private members.

===============================================================================
[FAIL]
Traceback (most recent call last):
  File "/home/adi/chevah/twisted/src/twisted/logger/test/test_format.py", line 93, in test_formatMethod
    self.assertEqual(
  File "/home/adi/chevah/twisted/src/twisted/trial/_synctest.py", line 444, in assertEqual
    super().assertEqual(first, second, msg)
  File "/usr/lib/python3.11/unittest/case.py", line 873, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/lib/python3.11/unittest/case.py", line 1253, in assertMultiLineEqual
    self.fail(self._formatMessage(msg, standardMsg))
twisted.trial.unittest.FailTest: 'hello world' != 'hello world PRIV private'
- hello world
+ hello world PRIV private


twisted.logger.test.test_format.FormattingTests.test_formatMethod

Twisted log format is also happy to format {what._priv.upper().upper()}

I think the formating should be less permisive.

As a start, don't allow access to private members.

[glyph]

It looks like there is no protection for private access

I think the formating should be less permisive.

As a start, don't allow access to private members.

… why? There are at least four big problems with doing that:

1. We don't enforce private-member restrictions anywhere else in the code; it's a convention. So this would be inconsistent.
2. Private member restrictions would only make sense if we knew that the objects that you're logging from are not owned by the same person authoring the log format string. Quite possibly the object comes from the application that is logging about it, which is fine.
3. These format strings are designed to emulate the behavior of f"" strings, or the built in format() or str.format operators, neither of which enforce private member restrictions. So it would be inconsistent and surprising.
4. Private members were not previously restricted so this would be a huge compatibility break. This only fixes our () formatting extension, private members were happily loggable before.

[adiroiban]

Thanks Glyph for the response.

I am ok to have this merged.
I am not using the Twisted log format function for any user configurable log templates.

Also, maybe there aren't that many people using this code for user facing configuration.

[adiroiban]

My concern about stdlib .format and also Twisted log format, is that it's too permisive.
and you end up with security issues like these https://github.com/lovasoa/pyformat-challenge

As long as you control the template, it's fine... but I guess that there might me people trying to use all kind of smart tricks with .format()

[glyph]

I am not using the Twisted log format function for any user configurable log templates.

Also, maybe there aren't that many people using this code for user facing configuration.

Yeah, it is explicitly a security failure for allowing "user-facing configuration" in log strings. The idea is that they're supposed to be static and constant, always.

[om26er]

This PR broke one of the existing ways of formatting

from twisted.logger import Logger
l = Logger()

class Shape(object):
    sides = 4
    name = "bamboozle"
    config = dict(foo='bar')

l.critical("{what.config[foo]} {what.sides} {what.name}", what=Shape())

ERROR is

Unable to format event {'what': <__main__.Shape object at 0x7f2d685d2410>, 'log_logger': <Logger '__main__'>, 'log_level': <LogLevel=critical>, 'log_namespace': '__main__', 'log_source': None, 'log_format': '{what.config[foo]} {what.sides} {what.name}', 'log_time': 1705018367.486725}: 'PotentialCallWrapper' object is not subscriptable

[glyph]

@om26er can you open a new bug, ideally a release blocker, if this hasn't been in a release yet?

(Did it myself, no need)

[oberstet]

chiming in, thank you @om26er for early warning and @glyph for your clear statement and reaction! cool, IMO this is OSS as it "should be", working together, being good OSS citizens - because, yes, fallout (will) hit txaio/autobahn/crossbar, but I just recently argued for keeping Twisted trunk (as well as multiple releases) in our CI since we do want to know early on about anything coming down the road with Twisted. We are (as you likely know) intensive users of Twisted and (still) love it;) It did break pretty new ground decades ago in Python and in general, paved the way, and is a broad spectrum, reliable weapon. I am still convinced (within the context of Python) even with aio and so present nowerdays .. anyways, cool=)

[glyph]

Thanks for running trunk and catching issues like this before they make it to release!