Feb 26, 2024

Updates

• 4th March: Added a snippet to detect bad content models.

In short

On a recent Umbraco 10 project we introduced a bug that meant logged in members were sometimes seeing other users' data. After a bit of scrambling we found the underlying issue and fixed it (before it went live - phew!). It turned out we were extending the Umbraco page model using partials, to send data to the view, which was inadvertently sharing data between users. Needless to say, we don’t extend page models in this way anymore and would advise against it.

Note that I've reproduced this issue in Umbraco 10 and 13 (specifically 10.8.4 and 13.1.1).

The problem

The problem is content model instances (e.g. BlogListing or SearchResults instances) in Umbraco have an “undefined life-cycle” and are often shared across requests (and users). So far, so unexpected.

As a result, if you happen to want to send other information to the view from the controller and decide to extend the page model by adding settable properties (using it as a view model), you may be inadvertently sharing data between user requests. If this data is sensitive or crucial, the results can be disastrous.

Figure 1: An interaction diagram showing two users requesting a page and receiving the same model instance.

Figure 2: A simplified interaction diagram showing two users making overlapping requests for a page and receiving the same model instance. Note that Anna, receives the model modified by Bob's request.

A quick example

Here’s an extension to the order page model (MyOrder) which includes a settable property for the order that the member is trying to view.

In the render controller, we load the order based on an order ID query param.

There are a few problems in this example but in most circumstances they would be innocuous - maybe the template isn’t expecting a null order after all and blows up. Unfortunately, thanks to the content model life-cycle we’ve created a pretty scary bug. Now, if a visitor removes the order ID param, they will see whatever order was last loaded onto that page model, which in all likelihood will belong to another user.

What do the docs say?

Some of you may already know, this is against best practice according to the Umbraco docs. Once you dig into the topic, the docs are clear that content models should be “stateless” and should not be used as “view models”. To me though, it feels like the docs bury the lead a little, so it’s not surprising that I’ve seen this issue crop up in a few projects. The issue can be summarised by these snippets from the docs:

“Extending models should be used to add stateless, local features to models.”

“Generally speaking, anything that is tied to the current request, or that depends on more than the modelled content, is a bad idea.”

“The scope and life-cycle of a model is not specified. In other words, you don't know whether the model exists only for you and for the context of the current request, or if it is cached by Umbraco and shared by all requests."

Best practice for view models

Surprisingly, the best starting point in the Umbraco docs for extending models is actually the route hijacking page (not the models builder page). This page is well worth a read and introduces one option for creating usable view models (PublishedContentWrapped) but maybe not the one you want! The key is to create a view model that either wraps the content model or is otherwise independent.

There are three options that I’m aware of:

1. Skip the controller completely and use View Components: this is a reasonable fallback but moving things like model binding or search executions to the component level is not an ideal solution.
2. Create a view model (e.g. MyOrderViewModel) that inherits PublishedContentWrapped: this means you’re still passing an IPublishedContent to the view but has the disadvantage of not allowing access to the properties of the wrapped model.
3. Create a view model that inherits from the model you want to extend: as far as I’m aware, this is the ideal solution since it lets you freely extend a model that is essentially the same content model your views would expect. Credit to Markus Johansson who suggested it in a forum answer in 2019.

A quick example

To continue the earlier example, here is a simple view model that inherits from the Umbraco content model MyOrder.

Conclusion

In summary, Umbraco content models should be stateless, they aren’t view models. If you want to add extra data to the view, create a view model that inherits from the page model.

Like many pitfalls, once you know it’s there, stateful Umbraco content models are an easy problem to avoid. I have seen this issue in the wild a few times though and I think it’s an easy one to miss, so it’s worth highlighting.

I think what’s scary about this issue is it can be difficult to spot in testing and while some examples are easily observable (like the badly wired MyOrderController above), most are probably totally invisible and only manifest when overlapping requests change the model at the same time (see figure 2 above). Truly, the stuff of nightmares!

Update (4th March): An easy way to check for bad content models

For anyone wondering if their project contains bad (stateful) content models, here is a quick snippet that I've used in my example project: