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

luci-app-firewall: remove zone name length limitiation #6158

Closed
wants to merge 1 commit into from
Closed

luci-app-firewall: remove zone name length limitiation #6158

wants to merge 1 commit into from

Conversation

evlli
Copy link

@evlli evlli commented Dec 18, 2022

It was already possible to exceed the 11 character limit by creating the new zone in the interface edit view. #6008
I could not find a test case where a longer name causes functional problems, therefor this patch removes the limit in the firewall view as well, in order to allow for more descriptive zone names.

Signed-off-by: Evelyn Alicke [email protected]

@jow-
Copy link
Contributor

jow- commented Dec 18, 2022

This change is only legal for firewall4, the iptables based firewall3 still has a length limitation.

@systemcrash
Copy link
Contributor

@evlli So one way one could get around this is by putting a firewall version check in the JS. And having the length check still apply if fw version == 3. Not sure how to handle the htm tho. Do you have any suggestions?

@systemcrash systemcrash added WIP pull request the author is still working on fix pull request fixing a bug Work needed Needs work by the pullrequest creator labels Dec 5, 2023
@evlli
Copy link
Author

evlli commented Mar 8, 2024

@systemcrash I've thought about this a few times and I'm honestly not very familiar with the codebase. I guess replacing the htm check with more js is the only way? that feels like a much bigger change than I originally planned for but I'm fine with it.

@systemcrash
Copy link
Contributor

The JS part seems trivial enough. But the HTML part, I guess we can drop? All new stuff is JS only, though this does not say what we must do with the Lua only parts. @jow- ?

If you think you can crack the HTML part also, give it a go!

@evlli evlli closed this by deleting the head repository Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix pull request fixing a bug WIP pull request the author is still working on Work needed Needs work by the pullrequest creator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants