CodeSOD: Yet Another Master of Evil

As a general rule, if you find yourself writing an extension system for your application, stop and do something else. It's almost always in the case of YAGNI: you ain't gonna need it.

George is a "highly paid consultant", and considers himself one of the "good ones": he delivers well tested, well documented, and clean code to his clients. His peer, Gracie on the other hand" is a more typical representative of the HPC class.

George and Gracie found themselves with a problem: based on the contents of a configuration file, they needed to decide what code to execute. Now, you might be thinking that some kind of conditional statement or maybe some sort of object-oriented inheritance thing would do the job.

There were five different code paths, and no one really expected to see those code paths change significantly. Gracie, who was identified as "the architect" on the responsibility matrix for the project, didn't want to write five different ways to do a similar task, so instead, she wrote one way to do all those tasks.

Here's the YAML configuration file that her efforts produced:

use.cases: 0: description: Default product user_file_name_cond: (\'product_start\' in file_name) and (\'pilot\' not in file_name) user_file_name_pf_truncate: '.sig' data_files: False downstream_file_name_templates: signal: _product_start_<platform>.sig downstream_script: another_script.sh 1: description: Full forced product user_file_name_cond: (\'force_all_products\' in file_name) and (\'pilot\' not in file_name) user_file_name_pf_truncate: '.sig' data_files: False downstream_file_name_templates: signal: _product_start_<platform>_pilot.sig conf_override: ['FORCE_PRODUCT=TRUE'] downstream_script: another_script.sh 2: description: Pilot forced product user_file_name_cond: (\'force_all_offers\' in file_name) and (\'pilot\' in file_name) user_file_name_pf_truncate: '_pilot_user.sig' data_files: True downstream_file_name_templates: signal: _product_start_<platform>_pilot_user.sig pilot_users: <platform>_pilot_users_<YYYYMMDD>.txt conf_override: ['FORCE_PRODUCT=TRUE'] downstream_script: another_script.sh 3: description: Pilot product user_file_name_cond: (\'product\' in file_name) and (\'pilot_user\' in file_name) user_file_name_pf_truncate: '_pilot.sig' data_files: True downstream_file_name_templates: signal: _product_start_<platform>_pilot_user.sig pilot_users: <platform>_pilots_<YYYYMMDD>.txt conf_override: ['FORCE_OFFERMATCH=FALSE'] downstream_script: another_script.sh 4: description: Forced assignment user_file_name_cond: (\'user_offer_mapping\' in file_name) or (\'budget\' in file_name) and (\'csv\' in file_name) user_file_name_pf_truncate: '_user_offer_mapping.sig' data_files: True downstream_file_name_templates: signal: _force_offer_assignment_start_<platform>_user_product_mapping.sig user_offer_mapping: <platform>_user_products_<YYYYMMDD>.txt budget: budget_<platform>.csv downstream_script: product/python/forced_assignment/src/main/forced_assignment.py 5: description: Test user_file_name_cond: (\'dummy\' in file_name) user_file_name_pf_truncate: '.sig' data_files: False downstream_file_name_templates: signal: _dummy_<platform>.sig downstream_script: python/forced_offers/semaphore/bin/dummy_downstream_script.sh wait_strs: ['dummy_downstream_script.sh']

Take a look at the user_file_name_cond field in each of those entries. Are you thinking to yourself, "Boy, that looks like some Python syntax in there?"

def get_file_use_case(file_name): for use_case, use_case_conf in Configuration.get_conf('use.cases').items(): user_file_name_cond = use_case_conf['user_file_name_cond'].replace('\\', '') if eval(user_file_name_cond): return use_case

You know that's an eval. It's at least not an exec, which is allowed to use the assignment operator, but there's nothing in eval that prevents it from having side effects. While a malicious config file would fail to do user_case_conf['downstrem_script'] = 'my_hostile_script', it could easily do user_case_conf.__setitem__('downstream_script', 'my_hostile_script').

Of course, hostile attacks aren't really the concern here, at least intentionally hostile. This code was going to run in production, and clients were going to be free to edit and modify those files however they liked. Malicious attacks aside, idiotic mistakes, or worse: clever hacks were entirely possible.

George did his best to illustrate why this was a terrible idea. Gracie, who was the architect, gleefully ignored him. The code got shipped. The client signed off on it. It runs in production.

There is a silver-lining here. George aggressively documented their solution, and his reasons for opposing it, and followed the company policy for involving management in conflicts. Management saw his point, and there was a reshuffling in the teams: George was made the architect, Gracie was "pivoted" to doing more development. While it was too late to fix the code, George used his position to make some policy changes: he worked with the team to democratically build a set of official code standards, made sure that they were applied to all code equally, and revised the team's approach to retros to improve their code in the future.

Of course, there's one person on the team who isn't interested in these changes. Their PRs end up being lengthy flamewars over whether or not they should be following policies. You can guess who that is"

[Advertisement] ProGet can centralize your organization's software applications and components to provide uniform access to developers and servers. Check it out!