CodeSOD: An Endpoint's Plugin

Heidi is doing some support work and maintenance on a application owned by a government agency. Currently, the work environment is a bit of a bureaucratic nightmare where you can't do even the mildest code change without going through four hundred layers of paperwork, signoff, and consensus building. This isn't just normal government stuff- it's coming straight as a reaction to the previous work done on this project.

Heidi was specifically trying to track down a bug where one of the generated documents was displaying incorrect data. That lead her to this method in their C# web code:

public ActionResult GenerateDocument(FormCollection form){ int briefType = int.Parse(form["reportId"]); int senderId = int.Parse(form["senderId"]); int signatureId = int.Parse(form["signatureId"]); int profileId = int.Parse(form["profileId"]); decimal dossierId = Decimal.Parse(form["dossierId"]); int? contactId = null; bool minute = "true".Equals(form["minuteBool"]); string rheIds = form["rheids"]; if (form.AllKeys.Contains("contactId")) { contactId = int.Parse(form["contactId"]); } ProfileDTO profile = ProfileManager.GetInstance().GetProfile(profileId); IEndPoint endpoint = profile.GetEndPoint(); DossierBrief brief = CreateBrief(briefType, dossierId, signatureId, senderId, contactId, null, null, minute, profile, rheIds); brief.Engine = endpoint.IsLocal() ? "Local" : "Service"; endpoint.SetLogger(this); try { endpoint.SendTo(brief, CurrentUserName); } catch (Exception e) { Logger(e, LogTypeEnum.ERROR); return ReturnFailure(e.Message); } return ReturnSuccess(null, "loadBriefStore();");}

There's a lot of meaningless garbage in here, and mostly you can skip over it and just look at the last line. When we successfully communicate to the endpoint, we send back a ReturnSuccess that contains a JavaScript method to invoke on the client. Yes, when the process works, the client just evals the body of the success message.

That's ugly coupling between the server-side and client-side layers of this code. But the GetEndPoint method got Heidi curious. What did that do?

public IEndPoint GetEndPoint(){ IEndPoint endPoint = EndPointManager.GetInstance().GetEndPoint(this.ENGINE); endPoint.SetProfile(this); return endPoint;}

Well, that doesn't seem like too much, does it? It's suspicious that they're using the Singleton pattern- C# tends to favor using Static classes for that. It's not wrong, but hey, let's see what it's doing.

public class EndPointManager{ private static EndPointManager current = new EndPointManager(); private Dictionary<string, IEndPoint> endPoints = new Dictionary<string, IEndPoint>(); public static EndPointManager GetInstance() { return current; } private EndPointManager() { foreach (string dll in Directory.GetFiles(AppDomain.CurrentDomain.BaseDirectory, "bin\\Application.Engine.*.dll")) { Assembly assembly = Assembly.LoadFile(dll); foreach (Type type in assembly.GetExportedTypes()) { if (type.GetInterfaces().Contains(typeof(IEndPoint))) { ConstructorInfo constructorInfo = type.GetConstructor(new Type[] { }); IEndPoint endPoint = (IEndPoint)constructorInfo.Invoke(new object[] { }); this.endPoints.Add(endPoint.GetName(), endPoint); } } } } //"}

And here we have everyone's favorite thing to write: a plugin loader that scans a directory for DLLs, loads them, and then scans them for objects which implement the IEndPoint interface.

At some point, I think, every "clever" developer gets the temptation to do this. You can deploy new endpoints without ever recompiling anything else! It's a plugin architecture! It's fancy! It's extensible!

The problems, of course, are that no one actually needed this to have a plugin architecture. The previous developer just did that because they were prematurely abstracting out their application engine, which is what happens to 70% of these sorts of plugin architectures. Worse, actually designing a safe, reliable, and usable plugin architecture is deceptively tricky. While this works fine, because they're not actually using it, if they were trying to really build plugins, they'd quickly realize what a mess they've made with all the unaddressed edge cases.

In fact, given how tricky it is, Microsoft actually provided a Managed Extension Framework (MEF), so this doubles as a case of reimplementing what already exists, and that dates back to at least .NET 4.0, and is part of CoreFx, so it's even available through .NET core (though its behavior might differ slightly).

So, what we have here is a case where a developer solved a problem they didn't have by reinventing a wheel that they could have been using, and did all that to ensure that they could pass raw JavaScript from the server to the client so the client can just eval it.

Heidi adds:

I'm not changing any of this (yet). I have no idea what would break (unit test coverage is something like 5.4%), and this does seem to actually work as intended- unlike a lot of other parts of this application.

[Advertisement] Ensure your software is built only once and then deployed consistently across environments, by packaging your applications and components. Learn how today!