port_forwarding: fix open-range TCP+UDP merge updating the wrong table#35
Merged
Merged
Conversation
addPortoRangeRule()'s merge-display code referenced portfRangeTable (the forward-range table's module-global, left behind without var by addPortfRangeRule()) instead of portoRangeTable, the open-range table it's actually meant to update. Once a user had opened the forward-range modal at any point in the session, merging two open-range rules of opposite protocol silently rewrote the forward-range table's row content instead of the open-range table's own row.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Found while adding the WAN-zone selector (#34):
addPortoRangeRule()'s TCP+UDP merge-display code referencedportfRangeTable-- the forward-range table's variable, left behind as an implicit global (novar) byaddPortfRangeRule()-- instead ofportoRangeTable, the open-range table it's actually supposed to update.In a real session this only manifests once a user has opened the Port Range Forwarding "Add" modal at some point earlier (making
portfRangeTablea defined, live table reference) -- from then on, merging two open-range rules of opposite protocol (e.g. adding a UDP rule that matches an existing TCP one on the same port/IP) silently rewrites the forward-range table's row content ("Both", or the row's description) instead of the open-range table's own row.Fix
One-line fix:
portfRangeTable.rows[...]→portoRangeTable.rows[...]on the line that sets the merged row's protocol label to "Both".Test plan
node --checkpassestests/known-devices/port-forwarding-open-range-merge.test.js(companiongargoyle-toolscommit) -- confirmed the test fails against the pre-fix code (asserts the open-range row shows "TCP" instead of merging to "Both", and that the forward-range table's unrelated row was left alone) and passes with the fixtests/known-devices/*.test.jssuite -- 51/62 pass, same 8 pre-existing unrelated failures with/without this changegrepthat the fix landed (portoRangeTable.rowsnow appears in place of the oldportfRangeTable.rowsreference)https://claude.ai/code/session_012xm1pavEAaQbR8Z5YoG8KX