OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.45k stars 2.41k forks source link

ScriptManager.Evaluate method does not cast variables #16856

Open emrahtokalak opened 1 month ago

emrahtokalak commented 1 month ago

Describe the bug

In versions 1.8.X, I was able to record content with my javascript code without any problems using the scriptManager.Evaluate method. In my tests on 2.0.2, I needed to cast the variables I read using the deserializeRequestData() method, it worked without the need for a previous cast operation. When I did not cast, I got a Json.Serialization error.

> at OrchardCore.Modules.InvokeExtensions.InvokeAsync[TEvents,T1](IEnumerable`1 events, Func`3 dispatch, T1 arg1, ILogger logger)
> 12:59:44|Default|0HN785HNJ9J4T:00000003|OrchardCore.ContentManagement.DefaultContentManager|ERR|IContentHandler thrown from OrchardCore.ContentManagement.Handlers.ContentPartHandlerCoordinator by JsonException
> System.Text.Json.JsonException: The JSON value could not be converted to System.String. Path: $.Text | LineNumber: 0 | BytePositionInLine: 9.
>  ---> System.InvalidOperationException: Cannot get the value of a token type 'StartArray' as a string.
>    at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_ExpectedString(JsonTokenType tokenType)
>    at System.Text.Json.Serialization.Metadata.JsonPropertyInfo`1.ReadJsonAndSetMember(Object obj, ReadStack& state, Utf8JsonReader& reader)
>    at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
>    at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value, Boolean& isPopulatedValue)
>    at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
>    --- End of inner exception stack trace ---
>    at System.Text.Json.ThrowHelper.ReThrowWithPath(ReadStack& state, Utf8JsonReader& reader, Exception ex)
>    at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
>    at System.Text.Json.JsonSerializer.ReadFromNodeAsObject(JsonNode node, JsonTypeInfo jsonTypeInfo)
>    at OrchardCore.ContentManagement.Handlers.ContentPartHandlerCoordinator.InvokeHandlers[TContext,TFieldContext](TContext context, TFieldContext fieldContext, Func`4 partHandlerAction, Func`4 fieldHandlerAction, Boolean createPartIfNotExists, Boolean createFieldIfNotExists)

Orchard Core version 2.0.2

This used to work with 1.8.x, so it's a regression

To Reproduce

To simulate this error, I created a workflow and I am sending form-data to this workflow via postman, I am trying to read the values ​​with the deserializeRequestData method in the script task and create new content.

Workflow-Jint-Test.zip

`curl --location 'https://localhost:5001/workflows/Invoke?token=CfDJ8GpqN7JdvYREmCoyoRWr4NG4VFxf7K_ociCaKEGfCHAav8QbiCKSdMOTs93vPanX3uGA9jHCTSXb861mScAdsdk1LlhB7CQwtjzUMn1nCixxydStI_mAS89xaDWbfsJ_IX26FeLzfe9Pp83CzZiWmFzAxWZaFKHQ-v4Z15iA-dvAILSAtxOeLB7hBz4ZWxjxOtucrB7WgIcWzm2LugumzPv6kBYVV6TDkfhFp1I-n9RD' \
--header 'Content-Type: multipart/form-data' \
--header 'Authorization: Bearer eyJhbGciOiJSUzI1NiIsImtpZCI6IjNCMEJDREZFMzE5NEM1MzA2NURGMzBBRkI5OUUzQUIyRjM2QURBQzgiLCJ4NXQiOiJPd3ZOX2pHVXhUQmwzekN2dVo0NnN2TnEyc2ciLCJ0eXAiOiJhdCtqd3QifQ.eyJpc3MiOiJodHRwczovL2xvY2FsaG9zdDo1MDAxLyIsImV4cCI6MTcyODQ3MDkwOSwiaWF0IjoxNzI4NDY3MzA5LCJhdWQiOiJvY3Q6RGVmYXVsdCIsImp0aSI6ImZhODhhODI0LWJiNDAtNGZkMy1iZGMyLTFjZGIwZTY2ZjVjZiIsIm9jOmVudHlwIjoiYXBwbGljYXRpb24iLCJzdWIiOiIzZjk3MWY2ZGQxMGQ0ZWIzODFmM2Q3NTFhMTU0M2ZlMiIsIm5hbWUiOiJFbXJhaCBQb3N0bWFuIENsaWVudCIsInJvbGUiOiJSZXN0Q29udGVudEFwaVJvbGUiLCJQZXJtaXNzaW9uIjpbIkFjY2Vzc0NvbnRlbnRBcGkiLCJBcGlWaWV3Q29udGVudCIsIkNsb25lQ29udGVudCIsIkNsb25lT3duQ29udGVudCIsIkRlbGV0ZUNvbnRlbnQiLCJEZWxldGVPd25Db250ZW50IiwiRWRpdENvbnRlbnQiLCJFZGl0T3duQ29udGVudCIsIkVkaXRDb250ZW50T3duZXIiLCJMaXN0Q29udGVudCIsIlByZXZpZXdDb250ZW50IiwiUHJldmlld093bkNvbnRlbnQiLCJQdWJsaXNoQ29udGVudCIsIlB1Ymxpc2hPd25Db250ZW50IiwiVmlld0NvbnRlbnQiLCJWaWV3T3duQ29udGVudCIsIkV4ZWN1dGVHcmFwaFFMTXV0YXRpb25zIiwiRXhlY3V0ZUdyYXBoUUwiXSwib2lfcHJzdCI6IjNmOTcxZjZkZDEwZDRlYjM4MWYzZDc1MWExNTQzZmUyIiwiY2xpZW50X2lkIjoiM2Y5NzFmNmRkMTBkNGViMzgxZjNkNzUxYTE1NDNmZTIiLCJvaV90a25faWQiOiJlMzhlODg1MjMyZDg0MzQ2YTE3YmU2ZjhhNTliZDEwNyJ9.OvAxmJl47FoQMngoeehpmI-ezAcf1uWwPZDsfllbES-tpr2suwV_1mn5a_fPlt_kdLU8MJ_ka2dYgK0iSI_UzFgWFkyJu5Q9SBCB9K9nOwZsXxQ3-BIurlbOZRbK_c-rXWqSLnGR9qdzohBS-Rz-e83ZHTEPXXLyLtDzWhFi7pXZJM0bEolqRIgDhysPg-PsicahxcZoHG_bA-VEVfXZWUUF1PA9HLz3dqFgCgDk0pDcitg4EReIlRxY0g5dpAZe8bDMmUulaZq48JKMhPzi4j51a2pkniyhkf7GTTj3P-fTFPZP3N0tc9ahycStjOlhw31w-pjlsN-iVx9zeuSfCQ' \
--form 'Title="Jint Test Postman"' \
--form 'Name="Emrah"' \
--form 'Age="37"' \
--form 'CreateDate="2024-10-09T00:00:00Z"'`

Expected behavior

I expect it to work with this js;

var data = deserializeRequestData();

createContentItem("JintTestModel", true,
    {
        "JintTestModel": {
            "Name": {
                "Text": data.Name
            },
            "Age": {
                "Value": data.Age
            },
            "CreateDate": {
                "Value": data.CreateDate
            }
        },
        "TitlePart": {
            "Title": data.Title
        }
    });

After the 2.x upgrade, it only works properly as I cast it below.

var data = deserializeRequestData();

createContentItem("JintTestModel", true,
    {
        "JintTestModel": {
            "Name": {
                "Text": String(data.Name)
            },
            "Age": {
                "Value": Number(data.Age)
            },
            "CreateDate": {
                "Value": String(data.CreateDate)
            }
        },
        "TitlePart": {
            "Title": String(data.Title)
        }
    });

Is this an expected situation? Should I add cast functions to all my codes like this after the upgrade?

sebastienros commented 1 month ago

I tried to blame @gvkries but he said it was because of @lahma

lahma commented 1 month ago

Sounds like yet another STJ interop problem, I guess behavior could have changed after switch from Newtonsoft. Jint could also have some regression if no test coverage exists for this kind of thing.

gvkries commented 1 month ago

This could be due to the way deserializeRequestData handles the form data. I've tried to reproduce the error, but it works for me. Only difference, I used application/x-www-form-urlencoded instead of multipart data.

emrahtokalak commented 1 month ago

This could be due to the way deserializeRequestData handles the form data. I've tried to reproduce the error, but it works for me. Only difference, I used application/x-www-form-urlencoded instead of multipart data.

Are you saying that in version 2.0.2, we can use the data returned from the deserializeRequestData method without any casting? I'm getting all kinds of errors in my tests. I think the problem is not with the deserializeRequestData method, but with the Json Serializer.

gvkries commented 1 month ago

Not sure what has changed here in 2.0, but I think the issue is that deserializeRequestData returns a dictionary with StringValues in the values. That's why you get a System.InvalidOperationException: Cannot get the value of a token type 'StartArray' as a string error. So I think you must cast to the correct type or handle this in another way.

emrahtokalak commented 1 month ago

Not sure what has changed here in 2.0, but I think the issue is that deserializeRequestData returns a dictionary with StringValues in the values. That's why you get a System.InvalidOperationException: Cannot get the value of a token type 'StartArray' as a string error. So I think you must cast to the correct type or handle this in another way.

Given that casting was not previously required in the script, I'm hesitant to state definitively that casting is now mandatory post-2.0. This could potentially result in a negative user experience.

gvkries commented 1 month ago

I agree that this seems to be a regression.

emrahtokalak commented 1 month ago

To maintain compatibility with my existing scripts and minimize the need for casting, I've developed a custom method. This method implicitly converts non-string data types to strings to prevent runtime errors. However, explicit casting of non-string data types remains essential for correct data handling.

 _formAsJsonObject = new GlobalMethod
            {
                Name = "requestFormAsJsonObject",
                Method = serviceProvider => (Func<Dictionary<string, object>>)(() =>
                {
                    var httpContextAccessor = serviceProvider.GetRequiredService<IHttpContextAccessor>();
                    var sanitizer = serviceProvider.GetRequiredService<IHtmlSanitizerService>();

                    var formData = httpContextAccessor.HttpContext.Request.Form;

                    var result = new Dictionary<string, object>();

                    foreach (var field in formData)
                    {
                        var sanitizedValues = field.Value.Select(value => sanitizer.Sanitize(value)).ToArray();

                        if (sanitizedValues.Length == 1)
                        {
                            result[field.Key] = sanitizedValues[0];
                        }
                        else
                        {
                            result[field.Key] = sanitizedValues;
                        }
                    }

                    return result;
                })
            };

The ideal usage should be as follows: I have come to the conclusion that we should cast it in every way.

var data = deserializeRequestData();

createContentItem("JintTestModel", true,
    {
        "JintTestModel": {
            "Name": {
                "Text": String(data.Name)
            },
            "Age": {
                "Value": Number(data.Age)
            },
            "CreateDate": {
                "Value": String(data.CreateDate)
            }
        },
        "TitlePart": {
            "Title": String(data.Title)
        }
    });
gvkries commented 1 month ago

I think we should keep this opened.

sebastienros commented 1 month ago

I think we need a Jint type handler for StringValues which would return a string or a string[] based on the number of values.

github-actions[bot] commented 1 month ago

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.