Skip to content

Fix discord json encoded strings#221

Open
stoggi wants to merge 6 commits intojellyfin:masterfrom
stoggi:fix-discord-json-encoded-strings
Open

Fix discord json encoded strings#221
stoggi wants to merge 6 commits intojellyfin:masterfrom
stoggi:fix-discord-json-encoded-strings

Conversation

@stoggi
Copy link

@stoggi stoggi commented Mar 30, 2024

The Discord webhook does not JSON encode the strings used in the handlebars template. So if a description has newlines, it results in invalid JSON, and does not correctly post to discord. As described in #217.

This pull request serializes all of the objects in the data dictionary with System.Text.Json.JsonSerializer.Serialize before rendering the template so that quotes and newlines are correctly escaped.

This may be necessary for other webhook clients depending on their APIs.

@crobibero
Copy link
Member

Create a function in DataObjectHelpers and call that instead

/// <summary>
/// JsonEncode the string values of the data object.
/// </summary>
/// <param name="dataObject">The data object to encode.</param>
/// <returns>The encoded data object.</returns>
public static Dictionary<string, object> JsonEncodeStrings(this Dictionary<string, object> dataObject)
{
    foreach (var key in dataObject.Keys)
    {
        var value = dataObject[key];
        if (value is string stringValue)
        {
            dataObject[key] = JsonEncodedText.Encode(stringValue);
        }
    }

    return dataObject;
}

@stoggi
Copy link
Author

stoggi commented Mar 30, 2024

Create a function in DataObjectHelpers and call that instead

After reviewing DataObjectHelpers I see that it already attempts to escape quotes for JSON encoded strings in many places, and we can extend the Escape helper:

    /// <summary>
    /// Escape quotes for proper json.
    /// </summary>
    /// <param name="input">Input string.</param>
    /// <returns>Escaped string.</returns>
    public static string Escape(this string? input)
    {
        if (input == null)
        {
            return string.Empty;
        }

        return JsonEncodedText.Encode(input).ToString();
    }

Copy link
Member

@crobibero crobibero left a comment

Choose a reason for hiding this comment

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

I think Escaping is different from JsonEncoding. I almost think that we would need to move this to a Handlebars function so the user can specify to encode in the template

return string.Empty;
}

return JsonEncodedText.Encode(input).ToString();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return JsonEncodedText.Encode(input).ToString();
return JsonEncodedText.Encode(input).Value;

@CapivaraTupiniquim
Copy link

Notifications that are not sent to discord. If I understand, this needs to be implemented to fix?

@Bpendragon
Copy link

I think Escaping is different from JsonEncoding.

In this case they're the exact same thing, assuming webhooks are always JSON and not XML, the existing Escape function calls out JSON specifically in the declarative comment.

JsonEncodedText.Encode(input) will call this function which will then after some validation (specifically that the UTF8 string is as long as expected) eventually end up on line 123 of that same file (note that the source itself has moved from "Encode" to "Escape" at this point). This function will go through some low level magic to find any character that might need escaped (including newlines) and then escape it appropriately. In this case Line Feeds are escaped right here

Now, if the webhook plugin can also send XML payloads (which does not seem to be the case but I fully admit I might be blind and missing it) I would say they are different things, and the second part of your suggestion might need followed up on. As it stands at the moment though in this case "Encode" and "Escape" mean the exact same thing.

@crobibero
Copy link
Member

Yes, this plugin supports xml and any other format that someone can imagine.

@Bpendragon
Copy link

@crobibero I just opened a PR that should resolve some of the questions you had on this one. I would have liked to extend/modify this PR but I got my local git into such a mess that I found it easier to simply fork the whole repo myself and start fresh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants