Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it easier to re-use CSRF protection for custom web handlers #1966

Open
djmattyg007 opened this issue Jan 14, 2021 · 0 comments
Open

Make it easier to re-use CSRF protection for custom web handlers #1966

djmattyg007 opened this issue Jan 14, 2021 · 0 comments
Labels
A-http Area: HTTP frontend C-enhancement Category: A PR with an enhancement or an issue with an enhancement proposal

Comments

@djmattyg007
Copy link
Contributor

Right now to make use of the http.csrf_protection and http.allowed_origins settings in extensions, a decent chunk of code must be duplicated. Most of this duplication comes from mopidy.http.handlers:JsonRpcHandler. There's also some configuration handling in mopidy.http.handlers:make_mopidy_app_factory.

To avoid duplication in web handlers, I think it would be pretty simple to factor out the code that isn't specific to the actual request-handling logic into a new base class to be re-used. If we were feeling generous, we could even provide a JSON-specific subclass of this handler.

There are a couple of options I can see for de-duplicating the configuration handling effort.

Option 1: Localise the effort in mopidy.http.actor:HttpServer

  1. Inside HttpServer.run (or one of the methods it calls), check (and potentially warn about) the value of the csrf_protection setting, and normalise the value(s) of allowed_origins.
  2. Pass these settings in to http:app factories as part of a "general HTTP settings" object, so that they can be passed to the handlers themselves.

This has the benefit of being a rather limited set of changes. However, it would require some special handling for existing http:app factories to avoid breaking backwards-compatibility.

Option 2: Make configuration loading more powerful

Instead of relying on individual extensions to normalise configuration themselves inside of actor code, it would be nice if arbitrary transformations and other normalisation could be performed by the configuration loading mechanism.

This is kind of what I'm envisaging to automatically sanitise the list of allowed origins:

class String(ConfigValue):
    def __init__(self, optional=False, choices=None, transformer=None):
        self._required = not optional
        self._choices = choices
        self._transformer = transformer

    def deserialize(self, value):
        value = decode(value).strip()
        validators.validate_required(value, self._required)
        if not value:
            return None
        if self._transformer:
            value = self._transformer(value)
        validators.validate_choice(value, self._choices)
        return value

class List(ConfigValue):
    def __init__(self, optional=False, subtype=None):
        self._required = not optional
        if subtype:
            self._subtype = subtype
        else:
            self._subtype = String()

    def deserialize(self, value):
        value = decode(value)
        if "\n" in value:
            values = re.split(r"\s*\n\s*", value)
        else:
            values = re.split(r"\s*,\s*", value)
        values = tuple(self._subtype.deserialize(v.strip()) for v in values if v.strip())
        validators.validate_required(values, self._required)
        return tuple(values)

You would then make use of it like so:

from mopidy import config

schema["allowed_origins"] = config.List(subtype=config.String(transformer: lambda x: x.lower()))

This approach would be fully backwards-compatible, and has the benefit of making the List config type more generic. However, this is probably a slightly bigger change than option 1 that would probably require its own issue to ensure the side-effects of this kind of change are better understood.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-http Area: HTTP frontend C-enhancement Category: A PR with an enhancement or an issue with an enhancement proposal
Projects
None yet
Development

No branches or pull requests

2 participants