codejail icon indicating copy to clipboard operation
codejail copied to clipboard

Stop using global state to store configuration options

Open kdmccormick opened this issue 1 year ago • 0 comments

Codejail has a global variable at codejail.jail_code.COMMANDS. It's a dictionary that maps command names (like "python") to safe shell commands. An example value is:

{'python': {'cmdline_start': ['/edx/app/edxapp/venvs/edxapp-sandbox/bin/python', '-E', '-B'], 'user': 'sandbox'}}

The dictionary is populated by calling codejail.jail_code.configure, which is generally done via the codejail.django_integration middleware.

This all works fine, but the fact that COMMANDS is a global variable is not idiomatic Python. More concretely, it's a pain in the neck for unit tests: the order and timing with which unit tests are run will affect whether .configure has been called or not. This could also cause problems across Python processes in production, leading to situations where codejail is configured in one process but effectively disabled in another process without anyone knowing. Not good.

This stateful system also forces us to use a middleware in order to configure codejail in Django--that's another layer where something could go wrong.

I recommend replacing COMMANDS with either:

  • a stateless function named load_commands(), which is safe to call over and over again, which does something like this:
     try:
        from django.conf import settings
     except ImportError, AttributeError
         settings = {}
      if settings.get('CODE_JAIL'):
          return _load_command_from_django_settings(settings)
      else:
          # .... load alternative/fallback configuration options, if we're not using Django
    
  • putting the entire Codejail interface behind a class, so clients would need to pass the settings into it, like this:
    from django.conf import settings
    codejail = CodeJail(some_setting=settings.CODEJAIL[...], etc..)
    codejail.safe_exec(...)
    

kdmccormick avatar Oct 18 '23 18:10 kdmccormick