Skip to content

Conversation

@yswtrue
Copy link

@yswtrue yswtrue commented Dec 27, 2025

Copilot AI review requested due to automatic review settings December 27, 2025 18:07
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request adds Clash API support to sing-box configuration, enabling external applications to control Sing-Box through the Clash API interface. This is a version 1.12.0+ feature.

Key changes:

  • Added Clash API configuration options (enable, address, port, token) to the sing-box settings UI
  • Implemented Clash API configuration in the sing-box config generator
  • Applied code formatting improvements throughout util_sing-box.lua for better readability

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.

File Description
luci-app-passwall2/luasrc/passwall2/util_sing-box.lua Added Clash API configuration logic (lines 1908-1925) and extensive code formatting improvements for line wrapping and alignment
luci-app-passwall2/luasrc/model/cbi/passwall2/client/other.lua Added UI form fields for Clash API configuration (enable flag, address, port, token) within the version 1.12.0+ settings block

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

end
end


Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

There's an extra blank line here that should be removed for consistency with the rest of the codebase.

Suggested change

Copilot uses AI. Check for mistakes.
config.experimental.clash_api = {}
end
end
config.experimental.clash_api.external_controller = clash_api_address .. ":" .. tostring(clash_api_port)
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

The port value should be converted to a number using tonumber() to ensure type consistency. Although Lua will perform automatic type conversion when concatenating strings, it's better to be explicit about type conversions for port numbers.

Copilot uses AI. Check for mistakes.
translate("Split handshake into multiple TCP segments. Enhances obfuscation. May increase delay. Use only if needed."))
o.default = 0

-- clash api enable option
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

The comment should follow Lua comment conventions. Consider using proper capitalization and punctuation for consistency with other comments in the codebase.

Suggested change
-- clash api enable option
-- Clash API enable option.

Copilot uses AI. Check for mistakes.
o = s:option(Flag, "fragment", "TLS TCP " .. translate("Fragment"),
translate("Split handshake into multiple TCP segments. Enhances obfuscation. May increase delay. Use only if needed."))
o.default = 0

Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

This line has trailing whitespace which should be removed for consistency with code style standards.

Copilot uses AI. Check for mistakes.
o.default = 0

o = s:option(Value, "clash_api_address", translate("Clash API Address"))
o.default = "0.0.0.0"
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

Binding the Clash API to "0.0.0.0" by default exposes it to all network interfaces, potentially allowing unauthorized access from the network. Consider defaulting to "127.0.0.1" (localhost only) for better security, and let users explicitly choose to expose it to the network if needed.

Suggested change
o.default = "0.0.0.0"
o.default = "127.0.0.1"

Copilot uses AI. Check for mistakes.
o:depends("clash_api_enable", true)

o = s:option(Value, "clash_api_token", translate("Clash API Token"))
o.default = ""
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

The token field allows an empty default value, which means the API can be enabled without authentication. Consider adding a validation warning or making the token required when the API is enabled to prevent unauthorized access.

Suggested change
o.default = ""
o.default = ""
o.rmempty = false

Copilot uses AI. Check for mistakes.
Comment on lines +1917 to +1920
if not config.experimental.clash_api then
config.experimental.clash_api = {}
end
end
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

The nested check for config.experimental.clash_api on line 1917 will always be false because config.experimental is being created on line 1916, so it will never have a clash_api property at that point. This nested check should be removed or restructured. Since you're creating a new config.experimental table, you should directly create the clash_api table within it without the nested check.

Suggested change
if not config.experimental.clash_api then
config.experimental.clash_api = {}
end
end
end
if not config.experimental.clash_api then
config.experimental.clash_api = {}
end

Copilot uses AI. Check for mistakes.
Comment on lines +1914 to +1925
if clash_api_enable == "1" then
if not config.experimental then
config.experimental = {}
if not config.experimental.clash_api then
config.experimental.clash_api = {}
end
end
config.experimental.clash_api.external_controller = clash_api_address .. ":" .. tostring(clash_api_port)
if clash_api_token and clash_api_token ~= "" then
config.experimental.clash_api.secret = clash_api_token
end
end
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

The Clash API configuration is only set when the experimental table doesn't exist (line 1915). However, the experimental table is already created at line 1785 when initializing the config. This means the Clash API configuration will never be applied because the condition if not config.experimental then will always be false. The logic should check if config.experimental exists and then add the clash_api configuration to it, or create it if it doesn't exist.

Copilot uses AI. Check for mistakes.
Comment on lines +1910 to +1923
local clash_api_enable = singbox_settings.clash_api_enable or 0
local clash_api_port = singbox_settings.clash_api_port or 9090
local clash_api_address = singbox_settings.clash_api_address or "0.0.0.0"
local clash_api_token = singbox_settings.clash_api_token or ""
if clash_api_enable == "1" then
if not config.experimental then
config.experimental = {}
if not config.experimental.clash_api then
config.experimental.clash_api = {}
end
end
config.experimental.clash_api.external_controller = clash_api_address .. ":" .. tostring(clash_api_port)
if clash_api_token and clash_api_token ~= "" then
config.experimental.clash_api.secret = clash_api_token
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

When clash_api_enable is set to "1", this code exposes the Clash-compatible control API on clash_api_address .. ":" .. clash_api_port but treats clash_api_token as optional, so by default it can listen on 0.0.0.0:9090 without any authentication. An unauthenticated Clash API on a router allows any host that can reach this port (e.g., any LAN client or, if the firewall is opened, WAN clients) to arbitrarily control Sing-Box routing and proxy behavior. To avoid this, require a non-empty clash_api_token when binding to non-loopback addresses and/or default the address to 127.0.0.1 when no token is set, and reject configurations that would expose an unauthenticated controller on all interfaces.

Copilot uses AI. Check for mistakes.
@lwb1978
Copy link
Contributor

lwb1978 commented Dec 28, 2025

@xiaorouji 这个pr不建议合并,
1.pr质量很差,修改了一大堆与pr内容无关的代码格式,影响整体代码的可读性。(估计是用ai来写代码,一通复制粘贴)
2.sing-box 的api也不建议添加,sing-box的api可以直接绕过pw的逻辑来控制sing-box的一些配置,会得出不一样的结果,homeporxy那边也已多次关闭了api 的pr。
3.当sing-box作为普通节点客户端时,可能会同时启动多个sing-box,届时会启动多个api端口,该pr显然并没有或并不知道需要处理这个情况。

@yswtrue
Copy link
Author

yswtrue commented Dec 29, 2025

格式化应该是装了lua插件自动格式化了,的确没有考虑多个singbox的情况,我都适用singbox做分流,感觉还可以。
主要是现在没有一个活跃连接的洁面不好看分流规则是怎样的,只能看日志和查看生成后的singbox配置文件,比较麻烦,所以想加一个clash api,方便面版上看活跃连接

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.

2 participants