Skip to content

add new parameters for AWG 2.0#1807

Merged
jasonlyc merged 1 commit intofirewalla:masterfrom
chgchi:my_dev
Mar 30, 2026
Merged

add new parameters for AWG 2.0#1807
jasonlyc merged 1 commit intofirewalla:masterfrom
chgchi:my_dev

Conversation

@chgchi
Copy link
Copy Markdown
Contributor

@chgchi chgchi commented Mar 27, 2026

No description provided.

@MelvinTo
Copy link
Copy Markdown
Contributor

PR Review Summary

✅ What looks good

  • Change is tightly scoped to AWG obfuscation option propagation, which keeps risk contained.
  • Expanding supported keys (s3/s4, i1..i5) aligns with broader AmneziaWG parameter sets and improves compatibility with newer client configs.
  • No behavioral changes outside _addObfuscationOptions, so regression surface is small.

⚠️ Issues found

  • Medium – falsy-value drop bug remains and affects new fields too.
    The current gate if (networkConfig[key]) skips valid values like 0 (or "0" depending normalization). For protocol/tuning knobs, zero can be meaningful. This can silently omit intended config.
  • Low – no validation/range checks for newly accepted keys.
    Adding passthrough keys without validation may allow malformed values to reach downstream command/config generation.
  • Issue requirement coverage: No linked issue found.

💡 Suggestions

  • Change presence check to explicit existence instead of truthiness, e.g.:
  • if (networkConfig[key] !== undefined && networkConfig[key] !== null).
  • Add lightweight validation for numeric obfuscation fields (type/range), at least for newly introduced i*/s* keys.
  • Add/extend unit tests:
  • includes 0 value cases,
  • confirms all new keys are emitted when present,
  • confirms invalid values are rejected or normalized.

Verdict

COMMENT


Repo: firewalla/firerouter
PR: #1807
Head SHA: 8a8735af9b69c06104e08ff3b4e96c1b57c8115e
Checked at: 2026-03-27 20:01:06 CST

@j-sallyjin
Copy link
Copy Markdown

PR Review Summary

✅ What looks good

  • Extends obfuscation option coverage in a focused way by adding:
  • s3, s4
  • i1i5
  • Change is minimal and localized to key whitelist logic, which keeps regression surface small.
  • Backward compatibility looks preserved for existing keys (jc/jmin/jmax/s1/s2/h1..h4).

⚠️ Issues found

  • Medium – potential correctness gap for numeric zero values remains.
    The condition if (networkConfig[key]) excludes valid falsy values (e.g., 0). If any obfuscation field allows zero, it will be silently dropped from generated entries.
  • Low – style inconsistency in key literals.
    "i5" uses double quotes while neighboring entries use single quotes; not functional, but reduces consistency/readability.
  • Issue requirement coverage: No linked issue found.

💡 Suggestions

  • Prefer explicit presence checks instead of truthiness:
  • e.g., if (networkConfig[key] !== undefined && networkConfig[key] !== null) (or Object.hasOwn(networkConfig, key) depending on desired semantics).
  • Keep key literal style consistent ('i5') to match project conventions.
  • Optional: add a small unit test ensuring obfuscation entries include zero-valued fields when provided.

Verdict

COMMENT


Repo: firewalla/firerouter
PR: #1807
Head SHA: 8a8735af9b69c06104e08ff3b4e96c1b57c8115e
Checked at: 2026-03-28 00:11:14 CST

@chgchi
Copy link
Copy Markdown
Contributor Author

chgchi commented Mar 30, 2026

AI feedback reviewed, no issues.

@jasonlyc jasonlyc merged commit 3f1482d into firewalla:master Mar 30, 2026
1 check passed
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.

4 participants