Question about performance

[redphx]

Wouldn't it be faster if this code (from aesecure.py)

def is_waf(self):
    schemes = [
        self.matchHeader(('aeSecure-code', '.+?')),
        self.matchContent(r'aesecure_denied\.png')
    ]
    if any(i for i in schemes):
        return True
    return False

become

def is_waf(self):
    if self.matchHeader(('aeSecure-code', '.+?')):
        return True

    if self.matchContent(r'aesecure_denied\.png'):
        return True

    return False

This way if self.matchHeader(('aeSecure-code', '.+?')) returns True it doesn't have to check for self.matchContent(...), unlike the original code. This applies to all plugins using any() instead of all().

[sandrogauci]

@redphx valid point. Thanks for pointing that out as when I reviewed these patterns, I made the wrong assumption. Will you submit a PR with the above changes?

[redphx]

@sandrogauci sure. I'll check all other plugins too.

[sandrogauci]

@sandrogauci sure. I'll check all other plugins too.

thanks!

[redphx]

Hi @sandrogauci, which one do you prefer?

def is_waf(self):
    if self.matchContent(r'iis (\d+.)+?detailed error'):
        return True

    if self.matchContent(r'potentially dangerous request querystring'):
        return True

    if self.matchContent(r'application error from being viewed remotely (for security reasons)?'):
        return True

    if self.matchContent(r'An application error occurred on the server'):
        return True

    return False

or

def is_waf(self):
    if (self.matchContent(r'iis (\d+.)+?detailed error') or
            self.matchContent(r'potentially dangerous request querystring') or
            self.matchContent(r'application error from being viewed remotely (for security reasons)?') or
            self.matchContent(r'An application error occurred on the server')):
        return True

    return False

[sandrogauci]

Hi @sandrogauci, which one do you prefer?

def is_waf(self):
    if self.matchContent(r'iis (\d+.)+?detailed error'):
        return True

    if self.matchContent(r'potentially dangerous request querystring'):
        return True

    if self.matchContent(r'application error from being viewed remotely (for security reasons)?'):
        return True

    if self.matchContent(r'An application error occurred on the server'):
        return True

    return False

or

def is_waf(self):
    if (self.matchContent(r'iis (\d+.)+?detailed error') or
            self.matchContent(r'potentially dangerous request querystring') or
            self.matchContent(r'application error from being viewed remotely (for security reasons)?') or
            self.matchContent(r'An application error occurred on the server')):
        return True

    return False

thanks for asking :) I'd personally go with the first - clearer and slightly easier to debug

[redphx]

I also go with the first, but I think I need to ask you first.
I think this change should be applied to all() check too.

def is_waf(self):
    schemes = [
        self.matchContent(r'<(title|h\d{1})>requested url cannot be found'),
        self.matchContent(r'we are sorry.{0,10}?but the page you are looking for cannot be found'),
        self.matchContent(r'back to previous page'),
        self.matchContent(r'proceed to homepage'),
        self.matchContent(r'reference id'),
        ]
    if all(i for i in schemes):
        return True
    return False

becomes

def is_waf(self):
    if not self.matchContent(r'<(title|h\d{1})>requested url cannot be found'):
        return False

    if not self.matchContent(r'we are sorry.{0,10}?but the page you are looking for cannot be found'):
        return False

    if not self.matchContent(r'back to previous page'):
        return False

    if not self.matchContent(r'proceed to homepage'):
        return False

    if not self.matchContent(r'reference id'):
        return False

    return True

But there is a problem: it'll become difficult to understand the code for plugins with multiple schemas.

def is_waf(self):
    schema1 = [
        self.matchHeader(('Server', 'LiteSpeed')),
        self.matchStatus(403)
    ]
    schema2 = [
        self.matchContent(r'Proudly powered by litespeed web server'),
        self.matchContent(r'www\.litespeedtech\.com/error\-page')
    ]
    if all(i for i in schema1):
        return True
    if any(i for i in schema2):
        return True
    return False

My idea: I'll split these schemas to separate functions. For example:

def is_waf(self):
    if check_schema_1(self):
        return True

    if check_schema_2(self):
        return True

    return False


def check_schema_1(self):
    # all
    if not self.matchHeader(('Server', 'LiteSpeed')):
        return False

    if not self.matchStatus(403):
        return False

    return True


def check_schema_2(self):
    # any
    if self.matchContent(r'Proudly powered by litespeed web server'):
        return True

    if self.matchContent(r'www\.litespeedtech\.com/error\-page'):
        return True

    return False

[sandrogauci]

offhand I like your solution. I'll review the PR once you're done / I'll get a moment (perhaps @0xInfection might take a look too :))

[redphx]

There are a lot of files so I think it'd be better to ask you first. The MR will be ready in a few days.

[0xInfection]

Closing this as the MR is merged.