Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d997b17ab
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const ruleModel = normalizeRuleModelFromConfig(config); | ||
| const customProxyGroups = ruleModel.customProxyGroups.length > 0 | ||
| ? ruleModel.customProxyGroups | ||
| : normalizeCustomProxyGroups(config.customProxyGroups); | ||
| const customRuleSets = ruleModel.customRuleSets; |
There was a problem hiding this comment.
When an existing v2.4 subscription config is generated after upgrade, user-added rule sets still live under legacy fields such as moduleRuleOverrides and customProxyGroups[].rules; this path now only reads the new customRuleSets model, so those saved RULE-SET providers/rules are silently omitted from the downloaded YAML. Please migrate/extract the legacy rule-set fields here before falling back to the new model so existing subscriptions keep their routing behavior.
Useful? React with 👍 / 👎.
| ? sanitizeDialerProxyGroups( | ||
| enabledDialerProxyGroups, | ||
| nodeNameSet, | ||
| new Set([...moduleGroupNameSet, ...customGroupNameSet]) | ||
| ) |
There was a problem hiding this comment.
When a dialer group lists a built-in group that is disabled by the current template, this merged moduleGroupNameSet still admits every module name, but generateProxyGroups only emits enabled modules. The generated dialer proxy group can therefore reference a non-existent proxy-group and produce invalid Mihomo YAML; filter module names to the enabled group set before passing them to the sanitizer.
Useful? React with 👍 / 👎.
| customProxyGroups, | ||
| moduleRuleOverrides, | ||
| moduleRuleExclusions, | ||
| customRuleSets, | ||
| builtinRuleEdits, |
There was a problem hiding this comment.
When a user saves a config template after changing built-in group advanced filters, member order, or group type, those settings live in proxyGroupAdvanced, but this upload payload only serializes custom groups/rules. The upload succeeds and later applying the template silently loses all built-in advanced proxy-group configuration, so include proxyGroupAdvanced and the mode flag in the saved config.
Useful? React with 👍 / 👎.
| out.push({ | ||
| id: id.value, | ||
| name: name.value, | ||
| emoji: emoji.value, | ||
| ...(description ? { description } : {}), |
There was a problem hiding this comment.
When a config template contains a custom proxy group with enabled: false, this normalization constructs a new group object without copying that flag. Because generation now respects enabled === false, uploading/validating such a template re-enables disabled custom groups when it is later applied, changing the generated proxy groups unexpectedly.
Useful? React with 👍 / 👎.
| ? selectDnsAddressesAfterFakeIpRecheck( | ||
| addresses, | ||
| await resolveHostnameByDoh(hostname, { timeoutMs: DOH_TIMEOUT_MS }) | ||
| ) |
There was a problem hiding this comment.
When the system resolver returns only a 198.18/15 fake-IP answer, these lines replace the checked address set with DoH results, but the subsequent fetch(currentUrl) still uses the system resolver rather than those DoH addresses. In that environment a hostname can pass the public-address check while the actual connection goes to the reserved/private fake-IP address, reopening the SSRF guard for that range; either force fetch to the rechecked address or keep blocking the system-resolved address.
Useful? React with 👍 / 👎.
| const member = availableByKey.get(getProxyGroupMemberKey(ref)) || buildMemberFromRef(ref, { | ||
| nodeNameSet, | ||
| moduleNames: options.moduleNames || {}, | ||
| customGroupsById, | ||
| }); |
There was a problem hiding this comment.
When an advanced extra member references a built-in module that is later disabled, it is absent from availableMemberProxyNames, but this fallback rebuilds it from the full module-name map and keeps it in the generated proxies list. Since disabled modules are not emitted as proxy-groups, the resulting YAML can reference a non-existent policy; only resolve extra module refs that are still available/enabled.
Useful? React with 👍 / 👎.
| const nextCustomRuleSets = removedTarget | ||
| ? state.customRuleSets.filter((ruleSet) => ruleSet.target !== removedTarget) | ||
| : state.customRuleSets; |
There was a problem hiding this comment.
When a preset rule has been moved to a custom group via builtinRuleEdits, deleting that custom group only removes matching customRuleSets; the edit target remains and generation later resolves the missing target to the fallback policy, silently routing that preset rule to the wrong group. Clear or reset builtin edits whose target is the removed custom group alongside these rule-set removals.
Useful? React with 👍 / 👎.
2 participants
Summary
Validation