Roundup Tracker - Issues

Issue 2551196

classification
Unset labelprop of a Multilink can lead to Python error when using context/history
Type: crash Severity: normal
Components: Web interface Versions: 2.1.0
process
Status: fixed fixed
:
: rouilj : ngaba, rouilj
Priority: normal : NeedsTesting

Created on 2022-02-17 22:38 by ngaba, last changed 2022-03-06 00:00 by rouilj.

Files
File name Uploaded Description Edit Remove
error.log.txt ngaba, 2022-02-17 22:38 Error log
Messages
msg7453 Author: [hidden] (ngaba) Date: 2022-02-17 22:38
When the history renderer of the web interface wants to print the change of a multilink property in the form
"propname: + added_item",
e.g.
"documents: + mydocument.docx",
it will crash, if the label property of the newly linked item is None. And in this case we get a cryptic error message in the browser (see the attached file error.log), which requires some time to debug.

I agree that no item should have empty labelprop, but users can do strange things... (In my scenario, the user made a filename empty.)

The problem is in the history() method of _HTMLItem class in cgi/templating.py (lines 1241-1248):
                                # if we have a label property, try to use it
                                # TODO: test for node existence even when
                                # there's no labelprop!
                                try:
                                    if labelprop is not None and \
                                            labelprop != 'id':
                                        label = linkcl.get(linkid, labelprop)
                                        label = html_escape(label)

Here html_escape(None) leads to an unhandled exception. I've added an
if label is None:
    label = "[empty]"
block before calling html_escape() as a workaround, and it is fixed my issue. I note that setting a default_value for the multilinked Class in schema.py does not help either, because the above code ignores default_value...
msg7454 Author: [hidden] (rouilj) Date: 2022-02-21 03:33
Hi Nagy:

Thanks for identifying the issue and your patch. Can you try replacing your change with:

                                 try:
                                     if labelprop is not None and \
                                             labelprop != 'id':
-                                        label = linkcl.get(linkid, labelprop)
+                                        label = linkcl.get(linkid, labelprop,
+                                                           default=self._("[la\
bel is missing]"))
                                         label = html_escape(label)


and tell me if it still fixes the issue. Feel free to replace the string "[label is missing]".
Keep the self._() call as it allows the string to be localized. Given how get() is
implemented, I think this is the better way to fix the issue.

Also an added benefit is to allow the string to be translated or overridden using the
translation system.

I just wish I could figure out how to test this code. I have spent more than an hour
trying to figure out how to get a test and database entries right to trigger this code.

-- rouilj
msg7455 Author: [hidden] (ngaba) Date: 2022-03-05 21:10
Dear John,

Your patch also works for me, and it is more elegant.
msg7456 Author: [hidden] (rouilj) Date: 2022-03-06 00:00
Thanks for the confirmation Nagy.

I committed the fix for the multilink case you found. I also handled the link path
the same way. I also applied the fix to the plain() calls. I am not sure that it would
result in a crash, but it shouldn't be generating null anyway.

I still don't have a good way to test it, but...
 
Closing.
History
Date User Action Args
2022-03-06 00:00:52rouiljsetstatus: new -> fixed
resolution: fixed
messages: + msg7456
2022-03-05 21:10:17ngabasetmessages: + msg7455
2022-02-21 03:33:56rouiljsetkeywords: + NeedsTesting
assignee: rouilj
messages: + msg7454
nosy: + rouilj
priority: normal
2022-02-17 22:38:13ngabacreate