4
\$\begingroup\$

This code exists in my view; it makes use of a model passed through from the controller.

$("#email").emailautocomplete({
domains: [@foreach (var organisation in Model.Organisations)
             {
                @Html.Raw("\"") @organisation.EmailDomain 
                @Html.Raw("\", \r\n") 
             }
                @Html.Raw("]")
});

Which renders as:

$("#email").emailautocomplete({
domains: ["domain1.gov.uk", 
"domain2.com", 
"domain3.gov.uk", 
"domain4.gov.uk", 
] 
});

This achieves what I want but I feel the razor syntax is messy and wonder if there is a cleaner way of passing the email domain strings into the JavaScript, to achieve the same rendered result.

I understand an ajax call is an option however I don't want to call the database again to get the list of organisations as the data has already been retrieved by the controller and passed into the view, and is used elsewhere in the view to populate a combo box.

\$\endgroup\$
3
\$\begingroup\$

You can simply use

$("#email").emailautocomplete({
    domains: @Html.Raw(Json.Encode(Model.Organisations.Select(x => x.EmailDomain)))
});

The combination of Html.Raw and Json.Encode will convert the collection to a javascript array.

It would however be cleaner if your view model contains a IEnumerable<string> Domains property so that in the view, it can be just

domains: @Html.Raw(Json.Encode(Model.Domains))
\$\endgroup\$
  • 1
    \$\begingroup\$ From a pragmatic point of view I'd agree but formally JSON and JavaScript encodings are different. That means that a valid JSON array of strings may not be a valid JavaScript array of strings. Few examples: \/ isn't a formally valid JS escape sequence and vice-versa \v and \0 aren't valid JSON escape sequences. This issue is mitigated because some browsers apply exactly same escaping rules (at least last time I checked) but it's still not portable and possible cause of errors. \$\endgroup\$ – Adriano Repetti Sep 5 '17 at 8:51
  • \$\begingroup\$ Very nice, please note for my .net core app I could not use Json.Encode - I had to use Newtonsoft.Json.JsonConvert.SerializeObject. \$\endgroup\$ – egmfrs Sep 5 '17 at 11:27
  • 1
    \$\begingroup\$ Should have tagged your question with it :) Refer also this answer \$\endgroup\$ – Stephen Muecke Sep 5 '17 at 11:30
  • \$\begingroup\$ @StephenMuecke Thanks, that's actually the answer I got my code from! I was about to ask you to update your answer before I accepted however I then noticed I hadn't tagged core, hence my comment to supplement it :) \$\endgroup\$ – egmfrs Sep 5 '17 at 11:39
3
\$\begingroup\$

I just see three issues:

  • You're making the view harder to read and you're missing a chance to reuse your code.
  • You're not properly escaping e-mail address (see RFC5322 and Wikipedia). If you don't then you're open to Code Injection attack (see later).
  • Some (older) browsers won't accept the trailing comma after the last array item (see MDN).

To solve the first issue you may write an extension method to build a JavaScript array string from an IEnumerable<T>:

static string AsJsArrayString<T>(this IEnumerable<T> items)
{
    var array = new StringBuilder();
    array.Append('[');
    array.Append(String.Join(",", items.Select(x => $"'{x}'")));
    array.Append(']');

    return array.ToString();
}

This code could be optimized and make more terse but I guess you understand the point:

$("#email").emailautocomplete({
    domains: @Html.Raw(Model.Organisations.Select(x => x.EmailDomain).AsJsArrayString())
});

This function may also be reused elsewhere.

Now we can look at second issue. E-mail addresses may contain invalid characters for a JavaScript string (for example " or '), JavaScript escaping is not the same as JSON escaping (done with Json.Encode()) but fortunately we have an ad-hoc function HttpUtility.JavaScriptStringEncode() (JavaScriptEncoder for .NET Core, see later):

static string AsJsArrayString(this IEnumerable<string> items)
    => "[" + String.Join(",", items.Select(x => "'" +  HttpUtility.JavaScriptStringEncode(x) + "'")) + "]";

Using String.Join() we also quickly fixed last issue in the list. If you can't/don't want to use an extension method you may also inline this function.

Last thing to change is that @Html.Raw() declaration into something less error-prone (I don't know you but I often forget @Html.Raw() if it does not always produce an invalid output):

@functions {    
    public static string ToJsArrayString(IEnumerable<string> items)
    {
        return "["
          + String.Join(",", items.Select(x => "'" + HttpUtility.JavaScriptStringEncode(x) + "'"))
          + "]";
    }
}

Now we may simply write:

$("#email").emailautocomplete({
    domains: @ToJsArrayString(Model.Organisations.Select(x => x.EmailDomain))
});

HttpUtility is a .NET thing, in .NET Core you have a brand you class JavaScriptEncoder. Let's write an extension method for that:

public static string Encode<T>(this JavaScriptEncoder encoder, IEnumerable<T> items)
{
    return '[' + String.Join(",", items.Select(x => EncodeAndQuote(x))) + ']';

    string EncodeAndQuote(string text)
        => '"' + encoder.Encode(text) + '"';
}

Used like this:

$("#email").emailautocomplete({
    domains: @JavaScriptEncoder.Encode(Model.Organisations.Select(x => x.EmailDomain))
});

Code injection. Unfortunately I've not been clear, domain names can contain characters that need to be escaped. In case you think it's not an issue: would you allow your users to have a perfectly valid and usable email address that, however, will break your Javascript code (I guess in an administrative page) with a chance to execute arbitrary code? I would not.

Let's pick this perfectly valid e-mail address: john.smith@(here\I\can\write\anything)example.com. If not escaped then it will break your JavaScript code. An easy way to block your service until you will find the issue and fix the problem.

Worse that that, they can execute code with the privileges of the user of that page (and it seems an administrative page). Imagine they write this perfectly valid e-mail address:

john.smith@(",$.post('/api/customers/delete'),")example.com

Your JavaScript code isn't broken but you're executing code they injected, in this case an innocent AJAX POST.

In short: do not EVER trust user input, even when you think you do not need escaping.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your insight. A few things learnt! Although: HttpUtility "does not exist" - am I missing a using directive? Though the local part of an email address can contain quotation characters, domain parts can't and since my list only contains strings of domains, I didn't consider escaping was necessary. Lastly, I did not understand what you said about Html.Raw (error prone / invalid output?) \$\endgroup\$ – egmfrs Sep 4 '17 at 15:56
  • 1
    \$\begingroup\$ This is a nice review for just a few lines of code ;-) \$\endgroup\$ – t3chb0t Sep 4 '17 at 15:58
  • \$\begingroup\$ In the "domains: @functions.." line of code I get Expected "{" - any ideas? Also I had to change your uppercase F in @Functions to @functions otherwise I get The name 'Functions' does not exist in the current context \$\endgroup\$ – egmfrs Sep 4 '17 at 16:06
  • \$\begingroup\$ @eg which MVC version are you using? For older versions you may go with the extension method (returning HtmlString). 2) but also domain may include UTF8 encoded text /and comments). 3) right, just drop functions. prefix, sorry \$\endgroup\$ – Adriano Repetti Sep 4 '17 at 18:40
  • \$\begingroup\$ @t3chb0t short code snippet are more funny! \$\endgroup\$ – Adriano Repetti Sep 5 '17 at 9:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.