Skip to content

luci-base, luci-app-firewall: enable range of MAC addresses#8017

Draft
ckorber wants to merge 2 commits into
openwrt:masterfrom
ckorber:pr/macrange
Draft

luci-base, luci-app-firewall: enable range of MAC addresses#8017
ckorber wants to merge 2 commits into
openwrt:masterfrom
ckorber:pr/macrange

Conversation

@ckorber
Copy link
Copy Markdown
Contributor

@ckorber ckorber commented Oct 17, 2025

In order to handle a range of MAC addresses it seems to be useful being able to set it in luci.
Therefore these commits use the src_mac field to specify ranges like can be seen in the pictures.
Also negation can be used to exclude a certain range for being handled.

This PR depends on openwrt/firewall4#74

  • This PR is not from my main or master branch 💩, but a separate branch ✅
  • Each commit has a valid ✒️ Signed-off-by: <my@email.address> row (via git commit --signoff)
  • Each commit and PR title has a valid 📝 <package name>: title first line subject for packages
  • Tested on: (x86/64, openwrt-24.10, firefox) ✅
  • Screenshot or mp4 of changes:

If a single MAC address is specified, it is validated by macaddr if a range is given it is validated by macrange
Screenshot From 2025-10-17 11-14-10
invalid MAC address
image
valid MAC address
Screenshot From 2025-10-17 11-17-00
invalid range
image
valid range
image
also negation is possible
image
upper address is lower than lower macaddr -> error

@systemcrash systemcrash added the depends on PR in other repo PR depends on PR in sister repo e.g. openwrt/packags label Oct 17, 2025
@systemcrash systemcrash marked this pull request as draft October 17, 2025 11:51
Copy link
Copy Markdown
Collaborator

@openwrt-ai openwrt-ai left a comment

Choose a reason for hiding this comment

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

Reviewed 2 new commits.

Commit checks

  • d7d71590bc1 (luci-base: parse for mac range): message body has a typo — "range of MAC addresses int they syntax of" should read "in the syntax of" — and uses ''' as a code fence (Markdown) where plain indentation is the git-log convention. Content otherwise matches the diff.
  • 4a5a74ae217 (luci-app-firewall: enable to handle mac ranges): message matches the diff.

One inline question on the macrange validator (equal-endpoint handling, plus two minor cleanups). The cross-repo dependency on openwrt/firewall4#74 is already disclosed in the PR description and the "depends on PR in other repo" label, so the missing backend support for the - range syntax is expected and not flagged here.

Note: the failing "Lua Lint" check does not appear related to this PR, which touches only JavaScript files.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator

@openwrt-ai openwrt-ai left a comment

Choose a reason for hiding this comment

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

Follow-up to my previous review: the inline comment referenced there failed to attach, so it is added here. Also correcting that previous note — all CI checks on this head commit are passing; please disregard the "Lua Lint" remark.


Generated by Claude Code

return this.assert(false, multicast ? _('valid multicast MAC address') : _('valid MAC address'));
}

return this.assert(mac1 < mac2, multicast ? _('valid multicast MAC address range') : _('valid MAC address range'));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The final check uses strict <, so a degenerate range with identical endpoints (e.g. 00:11:22:33:44:55-00:11:22:33:44:55) is rejected as invalid. Is that intended? nftables accepts an equal-endpoint range, and a user pasting the same MAC into both halves would get a confusing "valid MAC address range" error. If single-address ranges should be allowed, use mac1 <= mac2.

Two smaller notes on this validator:

  • The regex requires both capture groups (no ? quantifier), so once m != null both m[1] and m[2] are always present. The if (m[1]) / if (m[2]) guards can never be false and can be dropped.
  • tonumber parses the addresses only to compare ordering, while apply('macaddr', ...) is what actually validates them. Consider computing mac1/mac2 after both macaddr checks pass, so ordering is compared on values already known to be well-formed.

Generated by Claude Code

Copy link
Copy Markdown
Collaborator

@openwrt-ai openwrt-ai left a comment

Choose a reason for hiding this comment

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

Reviewed 2 re-pushed commits.

Commit checks

  • 054d5be "luci-base: parse for mac range" — the typo from the previous version ("int they syntax of") is now fixed. The message body still wraps the example in ''' (Markdown fence) rather than plain indentation as is the git-log convention; minor, content matches the diff.
  • a38abd2 "luci-app-firewall: enable to handle mac ranges" — message matches the diff.

The macrange validator logic is unchanged from the previously reviewed version, so my earlier inline note on the strict < (rejects equal-endpoint ranges), the always-true if (m[1]) / if (m[2]) guards, and the ordering of tonumber vs the macaddr checks still stands (see the existing thread on line 799) and is not repeated here.


Generated by Claude Code

Comment thread modules/luci-base/htdocs/luci-static/resources/validation.js Outdated
Christian Korber added 2 commits June 1, 2026 16:04
This commit is a preparation to add the possibility to block or accept a
range of MAC addresses in the syntax of:

'''
00:11:22:00:00:00-00:11:22:ff:ff:ff
'''

The commit depends on the PR openwrt/firewall4#74 being merged first.

Signed-off-by: Christian Korber <ck@dev.tdt.de>
This commit adds the feature to handle mac ranges to `luci-app-firewall`

Signed-off-by: Christian Korber <ck@dev.tdt.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

depends on PR in other repo PR depends on PR in sister repo e.g. openwrt/packags

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants