7

I have a piece of code that composes method name to call from a string parameter. I don't feel it's a good thing to do but I'm not sure what can go wrong with this.

Here is a simplified snippet of that code:

switchToFilter = function(filter){
    var filterMethod = 'switchTo' + filter;
    myObject[filterMethod]();
}
| improve this question | |
  • potential to cause name conflicts? what woudl myObject[MyObject] do? – Pieter B Jul 9 '15 at 14:28
  • 1
    I forgot to check my calendar. Is it "guess the programing language day" again already? It seems to come around more quickly every year. – Mawg says reinstate Monica Jul 9 '15 at 15:10
  • @Mawg I think the presumption was that the question could be asked independent of programming language, for any languages that store their functions as string keys (of which there are more than one) – Katana314 Jul 9 '15 at 16:33
  • Perhaps - but wouldn't the point of the question - "what can go wrong with this" - depend upon the language? – Mawg says reinstate Monica Jul 10 '15 at 6:13
12

At run time, the worst thing that could probably happen is that the method doesn't exist, so you get some sort of run-time error saying ERROR: method: 'switchToFoo' could not be found in context blah blah blah...

I'm not really sure if this type of code could be vulnerable to injection attacks. Where does the value for filter come from?

A bigger problem might be that whatever development tools you're using won't be able to work with this. For example, if you want your IDE to search for all places where switchToFoo is called, it won't be possible to find the ones called by the snippet you show above. Or if you want to refactor switchToFoo to switchToFou, you have to change the method name, and then change the code that passes the value for filter. You will know all about this, but this sort of thing usually trips up developers who are new to the project.

| improve this answer | |
  • 1
    You are right. I was missing the chain in call stack right here when I was debugging – Mohsen Dec 18 '13 at 23:35
19

What can go wrong with composing a method name out of a string?

If myObject[filterMethod]() doesn't exist, you could crash your application.

If your application is part of the safety controls for nuclear missile fire control, you could be potentially disabling critical safety controls.

If other software for the controls doesn't account for this crash, you could allow anyone to fire ze missiles.

If someone is having a bad day, they could decide to try to launch the missiles and be surprised they launch.

If they launch the missiles, you could start a nuclear war.

If a nuclear war starts, you could end civilization as we know it.

FOR THE LOVE OF THE CHILDREN CHECK THINGS EXISTS BEFORE ACCESSING THEM!

| improve this answer | |
  • 12
    Before they are engulfed by balls of fire, the type theorists will yell "I told you row-types had real life applications!!" – Daniel Gratzer Dec 18 '13 at 19:33
  • 1
    @jozefg: They always have to get the last word in, don't they? hmph – FrustratedWithFormsDesigner Dec 18 '13 at 19:48
  • Assuming they did myObject.containsKey(filterMethod), or something similar, though, what would be the drawbacks? – mowwwalker Dec 18 '13 at 20:55
  • 1
    @mowwwalker I guess, well, some people really, really dislike children. That might be a disadvantage in this case? – enderland Dec 18 '13 at 20:56
  • Am I missing something? The question asks about the problem with composing method name but this answer talks about non-existing method. Would a missing method be a problem in javascript regardless how it is called as long as no existence check? – Codism Dec 18 '13 at 22:01
4

The biggest risk is trying to access something that doesn't exist within the array structure. In the worst case, that will crash your application. In the best case, there wouldn't be any perceived operation going on.

If you wrapped that logic with some guard checks to make sure you weren't accessing something that wasn't there, it would be a little bit more robust. You could also consider adding logging for the errant requests that still manage to come in. With log information, you could hunt the problems down after the fact.

| improve this answer | |
1

It's a form of reflection, which has its uses in dynamically-built applications (like a GUI builder for example) or plugins, but should be a matter of last resort outside those contexts.

In addition to the excellent reasons stated in other answers, it's a sign that your class hierarchy probably needs refactoring. Doing so will most likely make it much easier to reason about all your code. For one refactored example:

switchToFilter = function(filterName){
    var filter = filters[filterName];
    filter.switch();
}

This allows you to use inheritance to avoid duplication in the switch functions, and most likely has a side effect of providing a convenient place to put other filter-specific behavior.

| improve this answer | |
0
  • If filter is from an external source like a user you have a potential source of unintended errors, possibly a security issue depending on the contents of myobject.
  • The lookup occurs at runtime so you don't get the typical benefits of type checking (existence, call signature matches function signature, etc)

That being said I do use dispatch tables much like what you have in your example. They are useful in particular instances. The key is understanding what the costs (and risks) are and making sure they don't out weigh the benefits.

| improve this answer | |
-1

If the string comes from an insecure source (user input etc. ) it is absolutely, totally unacceptable. You voluntarily added a code injection vulnerability to your code.

You can write some code that will map a list of known function names to known functions, where you have verified yourself that each of those functions may be called without negative side effects.

| improve this answer | |
  • The accepted answer already mentions code injection. – Brian Jul 9 '15 at 14:23
  • Yes, as a remote possibility. He's not really sure about it. I'm damned sure about it which makes the practice unacceptable. Unless you prove that code injection is not possible. Which you can't. – gnasher729 Jul 10 '15 at 0:42
  • Meh. Code injection doesn't really constitute a serious vulnerability if either A) the user providing the input is also the user who is at risk of being compromised or B) the user provided the input already has access to the machine. So, abusing reflection in this manner is not necessarily a significant security risk. Hence why the accepted answer asked where the value came from; if it came from the owner of the machine, it's not as much of a risk. – Brian Jul 10 '15 at 3:57

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.