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

fix: [M3-6442] - Inefficient firewall custom ports regex #9010

Conversation

abailly-akamai
Copy link
Contributor

Description 📝

Fix a codeQL inefficient regex warning for firewall custom ports (exponential backtracking on strings containing many repetitions of '-') in the firewall rule drawer.

I increased the regex matching specificity to only match:
42
1-10
1,2,3,4,5

and disallow things like:
1--10
1-2-3
1,2,3-4-5
1-2,3-4

along with extra spaces etc

Preview 📷

Screenshot 2023-04-17 at 1 54 39 PM

How to test 🧪

  • pull code locally, and add some rules at /firewalls/[id]/rules
  • yarn test FirewallRuleDrawer
    or
  • test regex at https://regex101.com/ or other online tool

@abailly-akamai abailly-akamai self-assigned this Apr 17, 2023
@bnussman-akamai
Copy link
Member

With the current regex, I can't create a firewall with the port to a comma separated list (1,2,3,4,5)

@abailly-akamai
Copy link
Contributor Author

@bnussman-akamai ah yeah good catch, we are formatting the input so the regex had to allow for whitespace. Updated it and added tests to cover for those cases for good measure:

  • 42
  • 1-10
  • 1,2,3,4,5
  • 1, 2, 3, 4, 5

Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add the additional restriction that the port must be <= 65535?

@abailly-akamai
Copy link
Contributor Author

@hkhalil-akamai IMO It's better to keep that validation only on the back-end (shall it ever change) and it's not a formatting validation which this PR is meant to target

@abailly-akamai abailly-akamai merged commit 2628423 into linode:develop Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants