feat: Add final_mask_settings to hosts#451
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/subscription/xray.py (1)
610-617:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPass
finalmaskas a dict here too — currently aFinalMaskPydantic instance can leak intojson.dumps.
_build_shadowsocks(lines 470–475) correctly convertsinbound.finalmaskto a dict viamodel_dump()before calling_stream_setting_config. This branch passesinbound.finalmaskthrough unchanged, so when the host has a structuredFinalMaskvalue (whichapp/models/subscription.pynow allows), it ends up assigned tostream_settings["finalmask"]as a Pydantic model. The eventualjson.dumps(self.config, ..., cls=UUIDEncoder)inrender()will raiseTypeErrorbecauseUUIDEncoderdoes not know how to serialize aFinalMaskinstance.Apply the same conversion as
_build_shadowsocks, ideally factored into a small helper to keep the two sites in sync:🐛 Proposed fix
+ `@staticmethod` + def _serialize_finalmask(finalmask): + if finalmask is None: + return None + if isinstance(finalmask, FinalMask): + return finalmask.model_dump() + return finalmask + @@ - outbound["streamSettings"] = self._stream_setting_config( - network=network, - security=security, - network_setting=network_setting, - tls_settings=tls_settings, - sockopt=sockopt, - finalmask=inbound.finalmask, - ) + outbound["streamSettings"] = self._stream_setting_config( + network=network, + security=security, + network_setting=network_setting, + tls_settings=tls_settings, + sockopt=sockopt, + finalmask=self._serialize_finalmask(inbound.finalmask), + )And reuse the helper in
_build_shadowsocks:- if inbound.finalmask is not None: - if isinstance(inbound.finalmask, FinalMask): - finalmask = inbound.finalmask.model_dump() - else: - finalmask = inbound.finalmask - outbound["streamSettings"] = self._stream_setting_config(network=inbound.network, finalmask=finalmask) + finalmask = self._serialize_finalmask(inbound.finalmask) + if finalmask is not None: + outbound["streamSettings"] = self._stream_setting_config(network=inbound.network, finalmask=finalmask)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/subscription/xray.py` around lines 610 - 617, The streamSettings branch is passing inbound.finalmask (a Pydantic FinalMask instance) directly into _stream_setting_config which can leak a model into config JSON; add a small helper (e.g., _finalmask_to_dict or reuse a wrapper) that calls inbound.finalmask.model_dump() if the value is a Pydantic model (or returns it unchanged if already a dict), then call that helper when building streamSettings (the same way _build_shadowsocks currently converts inbound.finalmask) so _stream_setting_config always receives a plain dict and no FinalMask instances reach json.dumps in render().app/subscription/links.py (1)
7-16:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
FinalMaskimport —_apply_finalmaskwill raiseNameErrorand CI is already failing.Line 170 references
FinalMask, but it is never imported in this file. Ruff (F821) and the Code Format Checker pipeline both fail on this. At runtime, any call to_apply_finalmaskwith a non-emptyinbound.finalmaskwill raiseNameError: name 'FinalMask' is not defined.🐛 Proposed fix
from app.models.subscription import ( GRPCTransportConfig, KCPTransportConfig, QUICTransportConfig, SubscriptionInboundData, TCPTransportConfig, TLSConfig, WebSocketTransportConfig, XHTTPTransportConfig, ) +from app.models.host import FinalMaskAlso worth factoring the
isinstance(..., FinalMask)branch with the identical one inapp/subscription/xray.py::_build_shadowsocksinto a small shared helper to avoid drift.Also applies to: 167-174
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/subscription/links.py` around lines 7 - 16, Import FinalMask into app/subscription/links.py so that references inside _apply_finalmask resolve; update the top import block to include FinalMask alongside the other models (e.g., GRPCTransportConfig, KCPTransportConfig, ...). Also consider extracting the isinstance(..., FinalMask) handling into a small shared helper used by both app/subscription/links.py::_apply_finalmask and app/subscription/xray.py::_build_shadowsocks to avoid duplicated logic and keep behavior consistent.
🧹 Nitpick comments (3)
app/core/hosts.py (1)
175-175: 💤 Low valueLGTM, with a small nit on the truthiness check.
host.final_mask_settingsis now a PydanticFinalMaskmodel, which is always truthy regardless of whether its fields are populated. If the intent is to fall back toinbound_config["finalmask"]only when the host explicitly hasn't set one, prefer an explicitis not Nonecheck (consistent with howhost.subscription_templatesis handled elsewhere in this function):♻️ Optional
- final_mask_settings = host.final_mask_settings if host.final_mask_settings else inbound_config.get("finalmask") + final_mask_settings = ( + host.final_mask_settings if host.final_mask_settings is not None else inbound_config.get("finalmask") + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/core/hosts.py` at line 175, The current fallback uses a truthiness check on host.final_mask_settings which is a Pydantic FinalMask model and always truthy; change the condition to an explicit None check so we only use inbound_config.get("finalmask") when the host has not set one (i.e., replace the truthiness check with "is not None" when assigning final_mask_settings in the same block that handles host.subscription_templates to mirror that logic).app/models/host.py (2)
37-52: 💤 Low valueNit: the
delay → intervalremap interacts withserialization_alias="delay".
intervalkeepsserialization_alias="delay", so:
- Input: both
{"interval": "..."}and the legacy{"delay": "..."}validate (good).- Output via
model_dump()(defaultby_alias=False) emits"interval"; onlymodel_dump(by_alias=True)emits"delay".In
app/subscription/xray.py::_build_shadowsocksand_build_outbound, theFinalMaskis dumped via plainmodel_dump(), so the output key will be"interval". Please confirm the consumer (xray-core fragment) actually expects"interval"here (it does for stream-level fragment, but please double-check thefinalMaskschema), and that the"delay"alias is only intended for an external API/UI layer. If both representations matter, prefervalidation_alias=AliasChoices("delay", "interval")so naming intent is explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/host.py` around lines 37 - 52, The delay→interval remap in XrayFragmentSettings uses serialization_alias="delay" on the interval Field while the before-validator delay_to_interval accepts input "delay", but code that dumps FinalMask via model_dump() in app/subscription/xray.py::_build_shadowsocks and _build_outbound will emit "interval" (not "delay"); either ensure the xray-core finalMask consumer accepts "interval" or make the model explicitly support both names for validation/serialization: replace serialization_alias with validation_alias=AliasChoices("delay","interval") on the interval Field (or remove the alias and always call model_dump(by_alias=True) where FinalMask is emitted) and keep the delay_to_interval validator on XrayFragmentSettings to preserve legacy input handling so both input and output names are consistent with the consumer.
191-202: 💤 Low valueMinor: redundant Enum coercion since
use_enum_values=True.With
use_enum_values=TrueonFinalMaskBaseModel,self.typeis already the string value (e.g."header-custom"), so wrapping the dict lookup key asFinalMaskTcpType(self.type)is round-trip work. You can just key the dict by the string values, or dropuse_enum_values=True. Same applies toFinalMaskUdpLayer.parse_settings.Also note:
FinalMaskTcpTypeonly has three members and all three are present in the mapping today, sosettings_modelis always non-None here — but the symmetrical UDP version does guard withif settings_model is not None. Adding the same guard for TCP would protect against future enum additions.♻️ Optional
- settings_model = { - FinalMaskTcpType.header_custom: FinalMaskTcpHeaderCustomSettings, - FinalMaskTcpType.fragment: XrayFragmentSettings, - FinalMaskTcpType.sudoku: FinalMaskSudokuSettings, - }.get(FinalMaskTcpType(self.type)) - self.settings = settings_model.model_validate(self.settings) - return self + settings_model = { + FinalMaskTcpType.header_custom.value: FinalMaskTcpHeaderCustomSettings, + FinalMaskTcpType.fragment.value: XrayFragmentSettings, + FinalMaskTcpType.sudoku.value: FinalMaskSudokuSettings, + }.get(self.type) + if settings_model is not None: + self.settings = settings_model.model_validate(self.settings) + return self🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/host.py` around lines 191 - 202, In parse_settings on the TCP model, avoid the redundant Enum coercion by keying the settings_model mapping with the enum's string values (e.g. use the literal keys matching use_enum_values like "header-custom", "fragment", "sudoku") instead of wrapping self.type with FinalMaskTcpType(self.type); also mirror the UDP guard by checking if settings_model is not None before calling settings_model.model_validate so future enum members don't cause an exception (refer to parse_settings, FinalMaskTcpType members, and the pattern used in FinalMaskUdpLayer.parse_settings).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/db/migrations/versions/f976bfcf4738_add_final_mask_settings_to_hosts_table.py`:
- Around line 1-16: The migration header docstring's "Revises:" value doesn't
match the down_revision variable; update the docstring line `Revises:
b7d9e1a2c3f4` to `Revises: af2d644dda44` so it matches the `down_revision =
'af2d644dda44'` (identify the migration by revision = 'f976bfcf4738') — only
change the docstring header to keep history consistent.
In `@app/models/host.py`:
- Around line 173-184: The union types FinalMaskTcpSettings and
FinalMaskUdpSettings are being prematurely coerced by Pydantic v2 smart-mode
union resolution before your parse_settings validator can re-dispatch by type,
so change parse_settings to run with mode="before" (re-dispatch on the raw dict
using the "type" field) for both the FinalMaskUdpLayer and FinalMaskTcpLayer
validators to select the correct settings_model before any union validation;
also update the dispatch dict used in parse_settings to include explicit
handlers for the six missing FinalMaskUdpType values (header_dtls, header_srtp,
header_utp, header_wechat, header_wireguard, mkcp_original) or add a clear
fallback that maps them to dict[str, Any] so settings are validated/kept
correctly instead of relying on union coercion.
---
Outside diff comments:
In `@app/subscription/links.py`:
- Around line 7-16: Import FinalMask into app/subscription/links.py so that
references inside _apply_finalmask resolve; update the top import block to
include FinalMask alongside the other models (e.g., GRPCTransportConfig,
KCPTransportConfig, ...). Also consider extracting the isinstance(...,
FinalMask) handling into a small shared helper used by both
app/subscription/links.py::_apply_finalmask and
app/subscription/xray.py::_build_shadowsocks to avoid duplicated logic and keep
behavior consistent.
In `@app/subscription/xray.py`:
- Around line 610-617: The streamSettings branch is passing inbound.finalmask (a
Pydantic FinalMask instance) directly into _stream_setting_config which can leak
a model into config JSON; add a small helper (e.g., _finalmask_to_dict or reuse
a wrapper) that calls inbound.finalmask.model_dump() if the value is a Pydantic
model (or returns it unchanged if already a dict), then call that helper when
building streamSettings (the same way _build_shadowsocks currently converts
inbound.finalmask) so _stream_setting_config always receives a plain dict and no
FinalMask instances reach json.dumps in render().
---
Nitpick comments:
In `@app/core/hosts.py`:
- Line 175: The current fallback uses a truthiness check on
host.final_mask_settings which is a Pydantic FinalMask model and always truthy;
change the condition to an explicit None check so we only use
inbound_config.get("finalmask") when the host has not set one (i.e., replace the
truthiness check with "is not None" when assigning final_mask_settings in the
same block that handles host.subscription_templates to mirror that logic).
In `@app/models/host.py`:
- Around line 37-52: The delay→interval remap in XrayFragmentSettings uses
serialization_alias="delay" on the interval Field while the before-validator
delay_to_interval accepts input "delay", but code that dumps FinalMask via
model_dump() in app/subscription/xray.py::_build_shadowsocks and _build_outbound
will emit "interval" (not "delay"); either ensure the xray-core finalMask
consumer accepts "interval" or make the model explicitly support both names for
validation/serialization: replace serialization_alias with
validation_alias=AliasChoices("delay","interval") on the interval Field (or
remove the alias and always call model_dump(by_alias=True) where FinalMask is
emitted) and keep the delay_to_interval validator on XrayFragmentSettings to
preserve legacy input handling so both input and output names are consistent
with the consumer.
- Around line 191-202: In parse_settings on the TCP model, avoid the redundant
Enum coercion by keying the settings_model mapping with the enum's string values
(e.g. use the literal keys matching use_enum_values like "header-custom",
"fragment", "sudoku") instead of wrapping self.type with
FinalMaskTcpType(self.type); also mirror the UDP guard by checking if
settings_model is not None before calling settings_model.model_validate so
future enum members don't cause an exception (refer to parse_settings,
FinalMaskTcpType members, and the pattern used in
FinalMaskUdpLayer.parse_settings).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69646fb6-43f0-40d7-8eb1-6adeee3ad1b5
📒 Files selected for processing (7)
app/core/hosts.pyapp/db/migrations/versions/f976bfcf4738_add_final_mask_settings_to_hosts_table.pyapp/db/models.pyapp/models/host.pyapp/models/subscription.pyapp/subscription/links.pyapp/subscription/xray.py
1 participant
Summary by CodeRabbit