Add regex pattern check to PORTS setting of `nmap-ports`

Donnype commented 2 months ago

Changes

Adds regex pattern validation to the PORTS setting for nmap-ports. The only thing we cannot really check without exploding the pattern is if the left side of a 1-20 style range is smaller than the right side, which I think is an OK edge case not to handle if we treat is as an empty list.

See the unit test to see how the pattern is constructed:

def get_pattern():
    max_65535 = r"(6553[0-5]|655[0-2]\d|65[0-4]\d{2}|6[0-4]\d{3}|[1-5]\d{0,4}|\d)"
    max_65535_or_port_range = f"({max_65535}|{max_65535}-{max_65535})"
    one_or_comma_separated = f"^{max_65535_or_port_range}$|^{max_65535_or_port_range}(,{max_65535_or_port_range})+$"

    return re.compile(one_or_comma_separated)

Issue link

Closes https://github.com/minvws/nl-kat-coordination/issues/3143

Demo

See the unit tests for the behavior and how this pattern was constructed!

QA notes

Please check that in the UI bad patterns are not allowed for this settings!

Code Checklist

[x] All the commits in this PR are properly PGP-signed and verified.
[x] This PR only contains functionality relevant to the issue.
[x] I have written unit tests for the changes or fixes I made.
[x] I have checked the documentation and made changes where necessary.
[x] I have performed a self-review of my code and refactored it to the best of my abilities.

Checklist for code reviewers:

Copy-paste the checklist from the docs/source/templates folder into your comment.

Checklist for QA:

Copy-paste the checklist from the docs/source/templates folder into your comment.

underdarknl commented 2 months ago

Approved because it's implemented as suggested in the issue ticket and includes well written tests. But in my opinion, we're overcomplicating it by using regex. The only advantage I see using regex is that we get free validation using jsonschema on the frontend side. This is a valid and reasonable advantage, but we might want to reconsider the complexity trade-off in future iterations.

I agree a regex is a horrible solution for this. however, its the only solution that fits inside the Schema, is somewhat declarative (our end goal for plugin definitions) and is usable in the backend, and frontend.

stephanie0x00 commented 2 months ago

Checklist for QA:

[x] I have checked out this branch, and successfully ran a fresh make reset.
[x] I confirmed that there are no unintended functional regressions in this branch:
- [x] I have managed to pass the onboarding flow
- [x] Objects and Findings are created properly
- [x] Tasks are created and completed properly
[x] I confirmed that the PR's advertised feature or hotfix works as intended.
[x] I checked the logs for errors and/or warnings and made issues where necessary

What works:

Seems to work for the ports and ranges I tested. The ports I skipped also seem to be skipped in the raw file.

What doesn't work:

n/a

Bug or feature?:

n/a

dekkers commented 2 months ago

I think the regex can be shorter because as far as I can see we can remove some repetition. That would make it easier to understand, but the current one does work, so I am not sure if I should spend a little bit of time on this?

minvws / nl-kat-coordination