Ayende @ Rahien

Hi!
My name is Ayende Rahien
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:

ayende@ayende.com

+972 52-548-6969

@

Posts: 5,947 | Comments: 44,540

filter by tags archive

Reviewing OSS ProjectWhiteboard Chat–setup belongs in the initializer


Originally posted at 3/8/2011

As a reminder, I am reviewing the problems that I found while reviewing the Whiteboard Chat project during one of my NHibernate’s Courses. Here is the method:

[Transaction]
[Authorize]
[HttpPost]
public ActionResult GetLatestPost(int boardId, string lastPost)
{
    DateTime lastPostDateTime = DateTime.Parse(lastPost);
    
    IList<Post> posts = 
        _postRepository
        .FindAll(new GetPostLastestForBoardById(lastPostDateTime, boardId))
        .OrderBy(x=>x.Id).ToList();

    //update the latest known post
    string lastKnownPost = posts.Count > 0 ? 
        posts.Max(x => x.Time).ToString()
        : lastPost; //no updates
    
    Mapper.CreateMap<Post, PostViewModel>()
        .ForMember(dest => dest.Time, opt => opt.MapFrom(src => src.Time.ToString()))
        .ForMember(dest => dest.Owner, opt => opt.MapFrom(src => src.Owner.Name));

    UpdatePostViewModel update = new UpdatePostViewModel();
    update.Time = lastKnownPost; 
    Mapper.Map(posts, update.Posts);

    return Json(update);
}

In this post, I want to focus on the usage of AutoMapper, in the form of Mapper.CreateMap() call.

It was fairly confusing to figure out why that was going on there. In fact, since I am not a regular user of AutoMapper, I assumed that this was the correct way of doing things, which bothered me. And then I looked deeper, and figured out just how troubling this thing is.

Usage of Mapper.CreateMap is part of the system initialization, in general. Putting it inside the controller method result in quite a few problems…

To start with, we are drastically violating the Single Responsibility Principle, since configuring the Auto Mapper really have very little to do with serving the latest posts.

But it actually gets worse, Auto Mapper assumes that you’ll make those calls at system initialization, and there is no attempt to make sure that those static method calls are thread safe.

In other words, you can only run the code in this action in a single thread. Once you start running it on multiple thread, you are open to race conditions, undefined behavior and plain out weirdness.

On the next post, I’ll focus on the security vulnerability that exists in this method, can you find it?

More posts in "Reviewing OSS Project" series:

  1. (15 Mar 2011) Whiteboard Chat–The Select N+1 issue
  2. (14 Mar 2011) Whiteboard Chat–Unbounded Result Sets and Denial of Service Attacks
  3. (11 Mar 2011) Whiteboard Chat–setup belongs in the initializer
  4. (10 Mar 2011) Whitboard Chat–overall design
  5. (09 Mar 2011) Whiteboard Chat

Comments

Ryan Heath

DateTime lastPostDateTime = DateTime.Parse(lastPost);

They could have used DateTime.TryParse/.TryParseExact or

change the type from string to DateTime.

DateTime.Parse will throw exceptions when lastPost cannot be parsed.

Ironically, in my spoken language 'lastPost' means 'menace' :)

// Ryan

Walter Poch

I'm not sure how the system behaves but if there are some limitations in wich board a user can see, the security check is missing and the boardId is passed directly to the _postRepository, which isn't responsible of security concerns.

Pete

At first, I didn't like that "lastPost" input string was being sent to the Model if there was no updates - always a bad idea to echo user input to your output - but that looks safe as the "Parse" method is effectively validating user input (it will throw if the string is not a date string)

Next thing is that you are not escaping output: Owner.Name is not being escaped on output.

Rule of thumb is to ALWAYS "validate input, escape output" and that will catch alot of security issues.

Ryan Heath

I think escaping is a responsibility of the view.

In this case the conversion to Json.

It bothers me more that Time is converted to string.

-- .ForMember(dest => dest.Time, opt => opt.MapFrom(src => src.Time.ToString()))

// Ryan

Corey

Once authenticated you can pass in whatever id's you want and there is no restriction to the logged in user.

Nick

There are actually 2 security vulnerabilities.

If no anti-forgery token is required (you can set this globally in MVC 3 with special filters) then they are open to csrf attacks.

I think the main point would be the passing around of ID's though as the poster above says. Pretty obvious.

Nick

Escaping output is usually handled by the view engine, pete. I don't think you could call that one a bug. But if they don't output encode in the view then you're right.

kshitij

What did automapper really them?

Syntax for mapping seems quite verbose.

Seems like all it avoided was a foreach on the posts.

Couldn't they have just created a mapper class of type IMapper <from,to> instantiated it once?

Corey

Using AutoMapper like this ends up littering the code-base with a ton of dependency on AutoMapper. You could easily reduce that dependency to one place with an extension method. posts.Map(update.Posts);

Christopher Wright

GET whiteboard/getlatestpost

This is a problem since a single call can create arbitrary amounts of work. If you require multiple calls, then you can have a firewall rule limiting the number of simultaneous connections from a given source or the number of connections from a source per minute. That guarantees that you can use the majority of server resources to process legitimate requests. If it's a single call, though, you're going to tie up an unbounded amount of memory and DB resources with no way to throttle individual users.

Also, your title for the next post kind of gave it away. Though since I've been reading your blog, the phrase "unbounded result set" is baked into my cerebellum, and it's also the major player in most of my nightmares.

Christopher Wright

I really want a preview function for these comments. The essence is providing a lastPost value of DateTime.MinValue (or something else very far in the past).

If you're guaranteeing that boards have a limited lifecycle in terms of number of updates, though, that wouldn't be so bad.

Jeff

What benefit do you get from using AutoMapper at all in the scenario? Looks like somebody is using it just for the sake of using it...

Jimmy Bogard

@Jeff

There's some pointless configuration there - the Time member configuration isn't needed, AutoMapper can handle object->string w/o configuration. The second one I'd argue isn't needed either, you could just name the destination member OwnerName and be done with it.

Comment preview

Comments have been closed on this topic.

FUTURE POSTS

No future posts left, oh my!

RECENT SERIES

  1. RavenDB Sharding (2):
    21 May 2015 - Adding a new shard to an existing cluster, the easy way
  2. The RavenDB Comic Strip (2):
    20 May 2015 - Part II – a team in trouble!
  3. Challenge (45):
    28 Apr 2015 - What is the meaning of this change?
  4. Interview question (2):
    30 Mar 2015 - fix the index
  5. Excerpts from the RavenDB Performance team report (20):
    20 Feb 2015 - Optimizing Compare – The circle of life (a post-mortem)
View all series

RECENT COMMENTS

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats