PR #2046 introduced changes to templates.js that cause content spoofing.
The specific change is that the notification text, instead of including a username, now includes @${contractID}, with the substitution happening during rendering.
I don't think that this change is a good idea because it makes the rendering logic need to parse text strings and it can lead to at least some limited content spoofing.
In particular, if the notification text contains a valid @${contractID} string, that username might be rendered instead of the intended string. Granted, this is unlikely and therefore this issue isn't exactly high priority. However, that doesn't mean it shouldn't be fixed.
For example, if a notification includes the text Something @123 happened, it might be rendered as Something alice happened instead of the original text.
Solution
There are two ways to address this:
Option 1
Revert most of the changes to templates.js that introduce this behaviour to the old behaviour of looking up usernames at the point in time the notification is created. Then, revert the string substitution changes. However, make sure that the key used for notification storage includes the ID and not the username. The downside of this is that the rendered text will reflect an outdated username, if it should change.
Option 2
This would maybe be a 'better' solution, and it should also be applied to chatroom mentions. This involves changes to separate data from presentation, so that the rendering code doesn't need to parse text to extract usernames.
For example, the notification text (or chatroom mentions) could look something like ["this", " is some text and ", { type: "mention", contractID: "123" }, " said so." ], which would be rendered as this is some text and alice said so, if alice has the contract ID 123.
Problem
PR #2046 introduced changes to
templates.jsthat cause content spoofing.The specific change is that the notification text, instead of including a username, now includes
@${contractID}, with the substitution happening during rendering.I don't think that this change is a good idea because it makes the rendering logic need to parse text strings and it can lead to at least some limited content spoofing.
In particular, if the notification text contains a valid
@${contractID}string, that username might be rendered instead of the intended string. Granted, this is unlikely and therefore this issue isn't exactly high priority. However, that doesn't mean it shouldn't be fixed.For example, if a notification includes the text
Something @123 happened, it might be rendered asSomething alice happenedinstead of the original text.Solution
There are two ways to address this:
Option 1
Revert most of the changes to
templates.jsthat introduce this behaviour to the old behaviour of looking up usernames at the point in time the notification is created. Then, revert the string substitution changes. However, make sure that the key used for notification storage includes the ID and not the username. The downside of this is that the rendered text will reflect an outdated username, if it should change.Option 2
This would maybe be a 'better' solution, and it should also be applied to chatroom mentions. This involves changes to separate data from presentation, so that the rendering code doesn't need to parse text to extract usernames.
For example, the notification text (or chatroom mentions) could look something like
["this", " is some text and ", { type: "mention", contractID: "123" }, " said so." ], which would be rendered asthis is some text and alice said so, ifalicehas the contract ID123.