Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Unexpected character encountered while parsing value" on save and publish #16288

Closed
kows opened this issue May 14, 2024 · 6 comments
Closed
Labels

Comments

@kows
Copy link

kows commented May 14, 2024

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

13.3.0

Bug summary

When trying to save or save and publish I get an error modal.

Specifics

Received an error from the server
An error occurred
Unexpected character encountered while parsing value: U. Path '', line 0, position 0.

Exception Details
Newtonsoft.Json.JsonReaderException, Newtonsoft.Json, Version=13.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed: Unexpected character encountered while parsing value: U. Path '', line 0, position 0.

at Newtonsoft.Json.JsonTextReader.ParseValue()
   at Newtonsoft.Json.JsonReader.ReadAndMoveToContent()
   at Newtonsoft.Json.JsonReader.ReadForType(JsonContract contract, Boolean hasConverter)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
   at Newtonsoft.Json.JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType)
   at Newtonsoft.Json.JsonSerializer.Deserialize(JsonReader reader, Type objectType)
   at Newtonsoft.Json.JsonConvert.DeserializeObject(String value, Type type, JsonSerializerSettings settings)
   at Newtonsoft.Json.JsonConvert.DeserializeObject[T](String value, JsonSerializerSettings settings)
   at Newtonsoft.Json.JsonConvert.DeserializeObject[T](String value)
   at Umbraco.Cms.Core.Models.Blocks.BlockEditorDataConverter.Deserialize(String json)
   at Umbraco.Cms.Core.PropertyEditors.BlockEditorValues.DeserializeAndClean(Object propertyValue)
   at Umbraco.Cms.Core.PropertyEditors.BlockEditorPropertyValueEditor.FromEditor(ContentPropertyData editorValue, Object currentValue)
   at Umbraco.Cms.Web.BackOffice.Controllers.ContentControllerBase.MapPropertyValuesForPersistence[TPersisted,TSaved](TSaved contentItem, ContentPropertyCollectionDto dto, Func`3 getPropertyValue, Action`3 savePropertyValue, String culture)
   at Umbraco.Cms.Web.BackOffice.Controllers.ContentController.MapValuesForPersistence(ContentItemSave contentSave)
   at Umbraco.Cms.Web.BackOffice.Controllers.ContentController.PostSaveInternal[TVariant](ContentItemSave contentItem, Func`3 saveMethod, Func`2 mapToDisplay)
   at Umbraco.Cms.Web.BackOffice.Controllers.ContentController.PostSave(ContentItemSave contentItem)
   at lambda_method2196(Closure, Object)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.AwaitableObjectResultExecutor.Execute(ActionContext actionContext, IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Logged|12_1(ControllerActionInvoker invoker)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeNextActionFilterAsync>g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextExceptionFilterAsync>g__Awaited|26_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)

Steps to reproduce

There are actually two flows in which this occurs.
Setup:

  • document type with block list editor which contains data (not empty).
  • using a handler for SendingContentNotification
    a) mark the block list property as readonly
    or b) remove the block list property from the tab.Properties to "hide" it

With this setup, if you try to save or save and publish, it is actually trying to process Umbraco.Blocklist as value iso the actual data.

Expected result / actual result

A readonly/hidden block list editor does not block save / save and publish.

@kows kows added the type/bug label May 14, 2024
Copy link

Hi there @kows!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

  • We'll assess whether this issue relates to something that has already been fixed in a later version of the release that it has been raised for.
  • If it's a bug, is it related to a release that we are actively supporting or is it related to a release that's in the end-of-life or security-only phase?
  • We'll replicate the issue to ensure that the problem is as described.
  • We'll decide whether the behavior is an issue or if the behavior is intended.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

@Migaroez
Copy link
Contributor

Migaroez commented May 15, 2024

Hey @kows do I understand correctly that

If you manipulate the content model in SendingContentNotification by either

  • marking a blocklist based property as readonly
  • or make the property not belong to any propertycollections

And then save OR Save & publish, you get the error you described?

If so, could you give me a (trimmed down) code example of your notificationHandler?

@kows
Copy link
Author

kows commented May 15, 2024

Yes and the block list editor must contain data for it to happen.

internal sealed class SendingContentNotificationHandler()
	: INotificationHandler<SendingContentNotification>
{
	public void Handle(SendingContentNotification notification)
	{
		if (notification.Content.ContentTypeAlias.Equals(Website.ModelTypeAlias))
		{
			foreach (ContentVariantDisplay variant in notification.Content.Variants)
			{
                                //exclude 'groupToHideAlias' tab/group which contains a blocklist editor
				variant.Tabs = variant.Tabs.Where(x => !x.Alias.InvariantEquals("groupToHideAlias"));
			}
		}

		if (notification.Content.ContentTypeAlias.Equals(ContentPage.ModelTypeAlias))
		{
			foreach (ContentVariantDisplay variant in notification.Content.Variants)
			{
				//making all editors on tabs readonly except 'Generic' tab
				foreach (Tab<ContentPropertyDisplay> tab in variant.Tabs.Where(t => t.Type == "Tab"))
				{
					foreach (ContentPropertyDisplay prop in tab.Properties ?? [])
					{
						prop.Readonly = true;
					}
				}
			}
		}
	}
}

@Migaroez
Copy link
Contributor

Hey @kows I had a look and was able to reproduce this with the following simplified NotificationHandler

using Umbraco.Cms.Core.Composing;
using Umbraco.Cms.Core.Events;
using Umbraco.Cms.Core.Models.ContentEditing;
using Umbraco.Cms.Core.Notifications;

namespace Umbraco.Cms.Web.UI.App_Data;

public class issue16288 : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        builder.AddNotificationHandler<SendingContentNotification, ReadOnlyBlockSendingNotificationHandler>();
    }
}

public class ReadOnlyBlockSendingNotificationHandler : INotificationHandler<SendingContentNotification>
{
    public void Handle(SendingContentNotification notification)
    {
        ContentPropertyDisplay? property =
            notification.Content.Variants
                .SelectMany(v => v.Tabs)
                .SelectMany(t => t.Properties ?? [])
                .FirstOrDefault(p => p.Alias == "blocklist");
        if (property is not null && property.SupportsReadOnly)
        {
            property.Readonly = true;
        }
    }
}

Looking at the code that processes property updates, we have some code in place that checks whether a property editor is marked as read-only. When you however mark the property as read-only/remove it from the outbound model, the client does not send any property data back for those properties while the property editor expects there to be a value.

In short: We do not expect you to change that value. The reason why this works for other editors and not this one is complex.

Because the system was not purposefully designed to handle this kind of manipulation, I am going to close this bug report. Feel free to open a discussion to express the use case where you would need this kind of functionality.

@kows
Copy link
Author

kows commented May 22, 2024

@Migaroez
whenever you want to tie security towards specific properties?
There's even a package out there: https://marketplace.umbraco.com/package/our.umbraco.community.adminonlyproperty
Kind of weird to say Because the system was not purposefully designed to handle this kind of manipulation, I am going to close this bug report. if you have a Readonly flag.

EDIT: this was no issue in older versions with Nested Content.

@Migaroez
Copy link
Contributor

Migaroez commented May 22, 2024

I do not deem setting an outbound (display) model flag as a way of enforcing security

To make something truly secure (in an editing context), you need to control the inbound flow, and the outbound just to be friendly. If you only control the outbound, you end up with security by obscurity as shown in the video below. To save time I used the package you linked with 2 users. But it should (and I am pretty sure it will) work the same with marking it read-only as I tested the client's behaviour regarding payload construction previously.

16288_obscurity.mp4

With purposefully designed I meant: I do not think all possible outcomes were considered when a feature was build that gave you the outbound model and just said "hey, update this however you want".

I can only guess as I was not a core developer at the time, but I believe the intent was that if you were to change anything or all things on an outbound model, you would make sure it wouldn't brake things when the client then send it back. And I am pretty sure that there was no clear intent to allow this feature to support implementers make things "more secure" (as per previous statement and example)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants