feat: MasterDnsVPN sidecar (DNS-tunnel client, VPN-mode capable)#18
Conversation
Integrate masterking32/MasterDnsVPN (DNS-tunneling VPN) as a bundled native-binary sidecar, using the hawkff fork that adds an Android FD_CONTROL_UNIX_SOCKET protect hook (wire-compatible with libcore's protect_path server: SCM_RIGHTS fd + 0x01 status). - MasterDnsVpnBean (TYPE_MASTERDNSVPN=25): full field set (domains, encryption method/key, resolvers, balancing strategy, packet duplication, compression, MTU bounds, local DNS, log level, advanced JSON override); needExternal=true, canMapping=false (binary connects to resolvers directly with protected sockets). - MasterDnsVpnFmt.buildMasterDnsVpnConfig: emits the client JSON config (uppercase keys, FD_CONTROL_UNIX_SOCKET=protect_path, loopback SOCKS5 no-auth) + resolverLines() for the resolvers file; advancedJson merged last. - ProxyEntity/TypeMap/KryoConverters wired; settings activity mapped. - BoxInstance: init builds config; launch writes config+resolvers files and runs libmasterdnsvpn.so -json <cfg> -resolvers <file>. - buildScript/lib/masterdnsvpn.sh: cross-compile the fork (branch android-vpnservice-protect-hook) for all 4 ABIs -> app/executableSo/<abi>/libmasterdnsvpn.so. - PluginManager.initNativeInternal + Executable.EXECUTABLES + add_profile_menu + full preferences UI (MasterDnsVpnSettingsActivity, arrays, strings). VPN-mode capable (fork protects upstream sockets). Sidecar-only; no sing-box core change.
- buildMasterDnsVpnConfig: wrap advancedJson parse in try-catch (malformed override must not crash config build / VPN start; logged + ignored). - Default encryption method 1 (XOR) -> 0 (None) in Bean/DataStore/XML: avoid false security (XOR with an empty key); user picks a cipher + key explicitly. - Fix KDoc to match the actual launch flags (-json <file>, not -json-base64). - masterdnsvpn.sh: validate Go >= 1.25 before building. - Simplify serverAddress placeholder in the settings init.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds MasterDnsVPN as a new external proxy profile type ( ChangesMasterDnsVPN Proxy Profile Type Integration
SingBoxOptions Recursive Map Hack Configuration
Sequence Diagram(s)sequenceDiagram
participant User as User/Import
participant MainActivity
participant BoxInstance
participant MasterDnsVpnFmt
participant libmasterdnsvpn as libmasterdnsvpn.so
participant BaseService
rect rgba(100, 149, 237, 0.5)
Note over User,MainActivity: Profile Import via masterdns:// URI
User->>MainActivity: Intent with masterdns:// scheme
MainActivity->>MasterDnsVpnFmt: parseMasterDnsVpn(url)
MasterDnsVpnFmt-->>MainActivity: MasterDnsVpnBean
end
rect rgba(144, 238, 144, 0.5)
Note over BaseService,libmasterdnsvpn: Sidecar Startup
BaseService->>BoxInstance: launch()
BoxInstance->>MasterDnsVpnFmt: buildMasterDnsVpnConfig(port, protectPath)
MasterDnsVpnFmt-->>BoxInstance: JSON config
BoxInstance->>libmasterdnsvpn: start -json config.json -resolvers resolvers.txt
BoxInstance-->>BaseService: launch() done
BaseService->>BoxInstance: awaitExternalProcessesReady()
loop TCP probe until ready or timeout
BoxInstance->>libmasterdnsvpn: connect LOCALHOST:socks_port
libmasterdnsvpn-->>BoxInstance: accept
end
BoxInstance-->>BaseService: ready
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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: 1
🤖 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/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.kt`:
- Around line 249-257: The filenames for configFile and resolversFile are
generated using only SystemClock.elapsedRealtime(), which causes collisions when
multiple MasterDnsVPN sidecars start within the same millisecond, leading to
file overwrites. Replace the current filename generation approach with a
collision-safe method such as using File.createTempFile() or appending a unique
identifier (like UUID.randomUUID()) in addition to the timestamp when
constructing the filenames for both the config file and the resolvers file,
ensuring each sidecar instance gets uniquely named artifacts in the cache
directory.
🪄 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: 7a833eab-9903-43cc-bc2f-d8ba766626f6
📒 Files selected for processing (18)
app/src/main/AndroidManifest.xmlapp/src/main/java/io/nekohasekai/sagernet/Constants.ktapp/src/main/java/io/nekohasekai/sagernet/bg/Executable.ktapp/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.ktapp/src/main/java/io/nekohasekai/sagernet/database/DataStore.ktapp/src/main/java/io/nekohasekai/sagernet/database/ProxyEntity.ktapp/src/main/java/io/nekohasekai/sagernet/fmt/KryoConverters.javaapp/src/main/java/io/nekohasekai/sagernet/fmt/TypeMap.ktapp/src/main/java/io/nekohasekai/sagernet/fmt/masterdnsvpn/MasterDnsVpnBean.javaapp/src/main/java/io/nekohasekai/sagernet/fmt/masterdnsvpn/MasterDnsVpnFmt.ktapp/src/main/java/io/nekohasekai/sagernet/plugin/PluginManager.ktapp/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationFragment.ktapp/src/main/java/io/nekohasekai/sagernet/ui/profile/MasterDnsVpnSettingsActivity.ktapp/src/main/res/menu/add_profile_menu.xmlapp/src/main/res/values/arrays.xmlapp/src/main/res/values/strings.xmlapp/src/main/res/xml/masterdnsvpn_preferences.xmlbuildScript/lib/masterdnsvpn.sh
…mp files - Add a MasterDnsVPN build step to the combined 'sidecars' job across ci/build/ preview/release: builds libmasterdnsvpn.so (4 ABIs) from the fork ref pinned via MDV_REF, caches it with the other sidecars (cache key + status hash include masterdnsvpn.sh and MDV_REF), and the build job's verify covers libmasterdnsvpn.so. Go 1.26.4 already satisfies the fork's Go 1.25+ requirement, so no extra setup-go. - BoxInstance: namespace the MasterDnsVPN config/resolvers temp files by port (unique per chain entry) to avoid same-millisecond collisions (CodeRabbit).
|
@coderabbitai review Addressed the temp-file collision finding: MasterDnsVPN config/resolvers files are now namespaced by the (unique-per-chain-entry) local port, so two sidecars can't overwrite each other. Also wired the masterdnsvpn build into the combined 'sidecars' CI job across all 4 workflows (build + cache + verify), pinned via MDV_REF. Note: this feature was validated on an x86_64 Android emulator before review — the binary parses the generated config, opens SOCKS, and the FD_CONTROL_UNIX_SOCKET protect hook sends upstream socket fds to a stub server using the exact 0x01 + SCM_RIGHTS protocol libcore's protect_server implements. |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 @.github/workflows/build.yml:
- Line 12: The workflow file `.github/workflows/build.yml` defines and uses the
variable `MDV_REF`, but the build script `buildScript/lib/masterdnsvpn.sh`
expects the variable to be named `MDVPN_REF`. This mismatch means the build
reference pinning is ineffective because the script never receives the value.
Rename all instances of `MDV_REF` to `MDVPN_REF` throughout the workflow file:
at the definition on line 12 (anchor), and at all usage points on lines 55,
67-69, and 97 (siblings) to align with what the build script actually reads.
- Line 51: The actions/cache steps are using a floating version tag `@v5` instead
of being pinned to a specific commit SHA, which violates the stated policy.
Replace the floating tag `@v5` with a pinned commit SHA for the actions/cache
action. This change needs to be applied at all locations where actions/cache is
used in the workflow file to ensure consistent policy compliance across all
cache steps.
In @.github/workflows/ci.yml:
- Line 12: The environment variable name is inconsistent between the CI workflow
and the build script. The workflow uses MDV_REF at lines 12, 60, 72-74, and 107,
but the build script (./run lib masterdnsvpn) expects MDVPN_REF. Rename all
occurrences of MDV_REF to MDVPN_REF throughout the workflow file to ensure the
cache keys and the actual compiled source reference are controlled by the same
variable and match what the build script reads.
- Line 56: The `actions/cache@v5` references are unpinned semantic version
references that violate policy and should be replaced with immutable commit SHAs
for reproducibility and security. Pin both occurrences of `actions/cache@v5` to
their corresponding commit SHA (look up the commit SHA for the v5 release of the
actions/cache repository) by changing the reference from `actions/cache@v5` to
`actions/cache@<commit-sha>`.
In @.github/workflows/preview.yml:
- Line 8: The preview workflow defines a variable called MDV_REF but the
masterdnsvpn.sh script expects the variable to be named MDVPN_REF, causing the
workflow's value to not be read by the script. Rename all occurrences of MDV_REF
to MDVPN_REF throughout the preview.yml workflow file at line 8, lines 51, lines
63-65, and line 93 to ensure the variable naming is consistent and the script
actually uses the pinned branch/commit value during the build execution.
In @.github/workflows/release.yml:
- Line 53: The actions/cache action references in the release workflow are
unpinned (using `@v5` tag). Replace the unpinned version reference with an
immutable commit SHA reference for the actions/cache action at both affected
locations (line 53 and line 95). Look for instances of `uses: actions/cache@v5`
and change them to use a full commit SHA instead of the version tag to ensure
security and reproducibility.
- Line 14: The release workflow uses the environment variable name `MDV_REF` but
the masterdnsvpn.sh script reads `MDVPN_REF`, causing a mismatch that makes ref
pinning ineffective. Choose one consistent variable name and update all
occurrences across both the workflow configuration (where `MDV_REF` is set) and
the buildScript/lib/masterdnsvpn.sh script (where it is read) to use the same
variable name end-to-end, ensuring the release ref is properly passed and cache
keys are correctly generated.
🪄 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: e31a00cc-0544-48fb-8597-a5b13b4e3d40
📒 Files selected for processing (5)
.github/workflows/build.yml.github/workflows/ci.yml.github/workflows/preview.yml.github/workflows/release.ymlapp/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.kt
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/res/xml/masterdnsvpn_preferences.xml (1)
116-119: ⚡ Quick winConstrain
mdvLogLevelto valid options.
mdvLogLevelis currently free-text, unlike your other enum-like settings. Invalid values can flow into generated config and break sidecar startup; prefer a menu-backed preference with fixed entries.Suggested XML change
- <EditTextPreference - app:key="mdvLogLevel" - app:title="`@string/mdv_log_level`" - app:useSimpleSummaryProvider="true" /> + <moe.matsuri.nb4a.ui.SimpleMenuPreference + app:defaultValue="INFO" + app:entries="`@array/mdv_log_level`" + app:entryValues="`@array/mdv_log_level_value`" + app:key="mdvLogLevel" + app:title="`@string/mdv_log_level`" + app:useSimpleSummaryProvider="true" />🤖 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/src/main/res/xml/masterdnsvpn_preferences.xml` around lines 116 - 119, The mdvLogLevel preference currently uses EditTextPreference which allows arbitrary free-text input that can break sidecar startup. Replace the EditTextPreference element with app:key="mdvLogLevel" with a ListPreference that has fixed app:entries and app:entryValues attributes defining the valid log level options, ensuring only valid values can be selected instead of allowing free-text input.
🤖 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 `@buildScript/lib/masterdnsvpn.sh`:
- Around line 24-25: The script currently reads the MDVPN_REF variable, but the
workflow defines and caches using MDV_REF, causing a mismatch between the cache
key reference and the actual built reference. Update the MDVPN_REF variable
assignment to read from the MDV_REF workflow variable instead, ensuring
consistency between what the workflow sets and what the script actually uses for
the build reference.
---
Nitpick comments:
In `@app/src/main/res/xml/masterdnsvpn_preferences.xml`:
- Around line 116-119: The mdvLogLevel preference currently uses
EditTextPreference which allows arbitrary free-text input that can break sidecar
startup. Replace the EditTextPreference element with app:key="mdvLogLevel" with
a ListPreference that has fixed app:entries and app:entryValues attributes
defining the valid log level options, ensuring only valid values can be selected
instead of allowing free-text input.
🪄 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: 1fd56c20-2bff-45ec-bf6a-f578817913ca
📒 Files selected for processing (22)
.github/workflows/build.yml.github/workflows/ci.yml.github/workflows/preview.yml.github/workflows/release.ymlapp/src/main/AndroidManifest.xmlapp/src/main/java/io/nekohasekai/sagernet/Constants.ktapp/src/main/java/io/nekohasekai/sagernet/bg/Executable.ktapp/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.ktapp/src/main/java/io/nekohasekai/sagernet/database/DataStore.ktapp/src/main/java/io/nekohasekai/sagernet/database/ProxyEntity.ktapp/src/main/java/io/nekohasekai/sagernet/fmt/KryoConverters.javaapp/src/main/java/io/nekohasekai/sagernet/fmt/TypeMap.ktapp/src/main/java/io/nekohasekai/sagernet/fmt/masterdnsvpn/MasterDnsVpnBean.javaapp/src/main/java/io/nekohasekai/sagernet/fmt/masterdnsvpn/MasterDnsVpnFmt.ktapp/src/main/java/io/nekohasekai/sagernet/plugin/PluginManager.ktapp/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationFragment.ktapp/src/main/java/io/nekohasekai/sagernet/ui/profile/MasterDnsVpnSettingsActivity.ktapp/src/main/res/menu/add_profile_menu.xmlapp/src/main/res/values/arrays.xmlapp/src/main/res/values/strings.xmlapp/src/main/res/xml/masterdnsvpn_preferences.xmlbuildScript/lib/masterdnsvpn.sh
The workflows set MDV_REF but masterdnsvpn.sh reads MDVPN_REF, so the ref pin was ineffective (script used its hardcoded default) and the cache key referenced a var that didn't drive the build. Rename to MDVPN_REF so the workflow env actually selects the fork ref and the cache invalidates on change.
|
@coderabbitai review Fixed the ref-name mismatch: renamed the workflow env MDV_REF -> MDVPN_REF to match what buildScript/lib/masterdnsvpn.sh reads (${MDVPN_REF:-...}), so the ref pin now actually selects the fork branch and the cache key invalidates on change. On 'pin actions/cache by commit SHA': acknowledged but declining for this PR — every action across all 4 workflows is tag-pinned (actions/checkout@v5, setup-java@v5, cache@v5, etc.); SHA-pinning is a repo-wide supply-chain hardening change, not specific to this feature, and would be inconsistent to apply to only the cache action. Better as a separate dedicated PR. |
|
Kicking off a full review now. ✅ Action performedFull review finished. |
Enable the (previously commented) upload-artifact step so each CI run publishes the assembled OSS debug APKs (all ABIs) for download — useful for on-device testing.
advancedJson is merged last, so a user could override FD_CONTROL_UNIX_SOCKET / LISTEN_IP / LISTEN_PORT / PROTOCOL_TYPE / SOCKS5_AUTH and silently break VPN-mode socket protection or the loopback SOCKS contract. Skip those keys (logged) so the override can only set tuning fields.
|
@coderabbitai review Latest commits since the last review:
Also: the paired MasterDnsVPN client change now only requires ENCRYPTION_KEY when DATA_ENCRYPTION_METHOD != 0, fixing the default-profile startup case. End-to-end validated against a live server: ChaCha20 tunnel establishes and real traffic egresses through the server. |
|
✅ Action performedFull review finished. |
|
Status on the latest review's inline findings (most are stale against the current head Already fixed (commit 22b10d5 / fbf4a88, after those comments were posted):
Declining (documented):
The feature is end-to-end validated against a live MasterDnsVPN server (ChaCha20 tunnel over DNS delegation v.nvim.space; real HTTP egress through the server). Leaving this PR open (not merging) per plan. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@buildScript/lib/masterdnsvpn.sh`:
- Around line 24-25: Replace the mutable branch reference in MDVPN_REF with a
specific commit hash to ensure reproducible builds. The variable currently
defaults to "android-vpnservice-protect-hook" which is a branch name that can
change over time without any modifications to this repository's code. Pin it to
a specific commit SHA instead so that the same source code is always used for
builds, improving reproducibility and supply-chain security. Additionally, check
lines 44-54 for any similar mutable references and apply the same commit pinning
approach to those locations as well.
🪄 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: 3a5d5541-62d8-4e04-838a-8b11ed3d9c0b
📒 Files selected for processing (22)
.github/workflows/build.yml.github/workflows/ci.yml.github/workflows/preview.yml.github/workflows/release.ymlapp/src/main/AndroidManifest.xmlapp/src/main/java/io/nekohasekai/sagernet/Constants.ktapp/src/main/java/io/nekohasekai/sagernet/bg/Executable.ktapp/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.ktapp/src/main/java/io/nekohasekai/sagernet/database/DataStore.ktapp/src/main/java/io/nekohasekai/sagernet/database/ProxyEntity.ktapp/src/main/java/io/nekohasekai/sagernet/fmt/KryoConverters.javaapp/src/main/java/io/nekohasekai/sagernet/fmt/TypeMap.ktapp/src/main/java/io/nekohasekai/sagernet/fmt/masterdnsvpn/MasterDnsVpnBean.javaapp/src/main/java/io/nekohasekai/sagernet/fmt/masterdnsvpn/MasterDnsVpnFmt.ktapp/src/main/java/io/nekohasekai/sagernet/plugin/PluginManager.ktapp/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationFragment.ktapp/src/main/java/io/nekohasekai/sagernet/ui/profile/MasterDnsVpnSettingsActivity.ktapp/src/main/res/menu/add_profile_menu.xmlapp/src/main/res/values/arrays.xmlapp/src/main/res/values/strings.xmlapp/src/main/res/xml/masterdnsvpn_preferences.xmlbuildScript/lib/masterdnsvpn.sh
Implement toUri()/parseMasterDnsVpn() so MasterDnsVPN profiles can be shared and imported via a friendly masterdns:// scheme, in addition to the existing universal sn://masterdnsvpn link. The encryption key is carried as URL userinfo; tunnel domains and resolvers as comma-joined query params; only non-default tuning values are emitted to keep links short (defaults re-applied on parse via initializeDefaultValues). - Wire parseMasterDnsVpn into parseProxies dispatch (Formats.kt) - Wire toUri into ProxyEntity.toStdLink so the standard QR/clipboard share menu uses the new scheme - Register masterdns scheme in the profile-import intent-filter for external deep links QR scanning already routes through RawUpdater.parseRaw -> parseProxies, so scanning a masterdns:// code now imports directly.
The custom SingBoxOptionSerializer obtained a delegate adapter from the same Gson instance it was registered on, so serializing a SingBoxOption re-entered the same type-hierarchy serializer and recursed until StackOverflowError, surfacing to the user as a 'stack size NNNNKB' connection failure. Serialize with a plain Gson tree instead, then merge _hack_config_map and _hack_custom_config recursively over the resulting JSON object (including nested SingBoxOption fields and collections) without re-entering the custom adapter.
Two issues prevented the MasterDnsVPN sidecar from coming up in-app: 1. The CGO/PIE Go client could fault (SIGSEGV) during its first protected upstream socket dial when launched in the VpnService process context, due to the Go runtime's signal-based async preemption. Launch it with GODEBUG=asyncpreemptoff=1; the sidecar is I/O-bound so scheduling latency is unaffected. 2. The service reported Connected immediately after launching sidecars, so the socks outbound could race a listener that had not bound yet. Add awaitExternalProcessesReady() to wait for each sidecar's local SOCKS port to accept connections first. MasterDnsVPN only listens after MTU probing and session setup (tens of seconds, with retries) so it gets a longer readiness window than sidecars that bind immediately.
- SingBoxOptionSerializer: also recurse into Map values that are SingBoxOption instances (e.g. handshake_for_server_name, fallback_for_alpn) so _hack_config_map merging matches the previous delegate-based behavior. - buildMasterDnsVpnConfig: fail fast when no tunnel domain is configured, mirroring the existing encryption-key validation. - toUri: refuse to build a share link for a profile with no tunnel domain instead of emitting an unusable link.
Update: MasterDnsVPN sidecar now starts reliably under VpnServicePushed three fixes (CI green, commit 31f3a0c): 1. 2.
3. VerifiedConnected end-to-end on a physical arm64 device (Android 13): tunnel Declined review findings
|
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/build.yml:
- Line 12: The MDVPN_REF environment variable currently uses a mutable branch
reference (android-vpnservice-protect-hook) which can change without affecting
the cache keys used at lines 55 and 97, leading to stale artifacts. Replace the
branch name in the MDVPN_REF variable with a specific commit SHA instead of the
branch reference. Ensure this commit SHA is also updated consistently wherever
MDVPN_REF is used in cache operations throughout the workflow.
In `@app/src/main/java/io/nekohasekai/sagernet/database/ProxyEntity.kt`:
- Around line 86-87: The new masterDnsVpnBean field added to the ProxyEntity
class modifies the database schema but requires a corresponding version bump and
migration. Locate the Room database version constant (currently at 8) and bump
it to 9. Then create a new migration that handles the schema change from version
8 to 9, which should include an ALTER TABLE statement to add the
masterDnsVpnBean column to the proxy_entities table. Ensure the column is
defined with an appropriate type and nullability to match the masterDnsVpnBean
property definition in ProxyEntity.
🪄 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: 458be052-57a3-4f91-92be-e0621d385df3
📒 Files selected for processing (25)
.github/workflows/build.yml.github/workflows/ci.yml.github/workflows/preview.yml.github/workflows/release.ymlapp/src/main/AndroidManifest.xmlapp/src/main/java/io/nekohasekai/sagernet/Constants.ktapp/src/main/java/io/nekohasekai/sagernet/bg/BaseService.ktapp/src/main/java/io/nekohasekai/sagernet/bg/Executable.ktapp/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.ktapp/src/main/java/io/nekohasekai/sagernet/database/DataStore.ktapp/src/main/java/io/nekohasekai/sagernet/database/ProxyEntity.ktapp/src/main/java/io/nekohasekai/sagernet/fmt/KryoConverters.javaapp/src/main/java/io/nekohasekai/sagernet/fmt/TypeMap.ktapp/src/main/java/io/nekohasekai/sagernet/fmt/masterdnsvpn/MasterDnsVpnBean.javaapp/src/main/java/io/nekohasekai/sagernet/fmt/masterdnsvpn/MasterDnsVpnFmt.ktapp/src/main/java/io/nekohasekai/sagernet/ktx/Formats.ktapp/src/main/java/io/nekohasekai/sagernet/plugin/PluginManager.ktapp/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationFragment.ktapp/src/main/java/io/nekohasekai/sagernet/ui/profile/MasterDnsVpnSettingsActivity.ktapp/src/main/java/moe/matsuri/nb4a/SingBoxOptions.javaapp/src/main/res/menu/add_profile_menu.xmlapp/src/main/res/values/arrays.xmlapp/src/main/res/values/strings.xmlapp/src/main/res/xml/masterdnsvpn_preferences.xmlbuildScript/lib/masterdnsvpn.sh
| var masterDnsVpnBean: MasterDnsVpnBean? = null, | ||
| ) : Serializable() { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ProxyEntity field change =="
rg -n "masterDnsVpnBean|TYPE_MASTERDNSVPN" app/src/main/java/io/nekohasekai/sagernet/database/ProxyEntity.kt
echo
echo "== Room database version/migrations =="
fd -a 'SagerDatabase.kt' | xargs -r rg -n "version\\s*=|AutoMigration|autoMigrations"
echo
echo "== Generated schema artifacts mentioning proxy_entities/masterDnsVpnBean (if present) =="
fd -a 'schemas' || true
rg -n "proxy_entities|masterDnsVpnBean" -g "**/*.json" || trueRepository: hawkff/NekoBoxForAndroid
Length of output: 1984
Bump Room schema version and add migration for the new masterDnsVpnBean column.
Line 86 adds masterDnsVpnBean as a persistent ProxyEntity field, which modifies the proxy_entities table schema. However, the database version remains 8 with migrations only up to 7→8. No version bump to 9 or corresponding migration exists, which will cause database open failures with a schema mismatch.
Suggested fix shape
-@Database(..., version = 8, autoMigrations = [
- ...
- AutoMigration(from = 7, to = 8)
-])
+@Database(..., version = 9, autoMigrations = [
+ ...
+ AutoMigration(from = 7, to = 8),
+ AutoMigration(from = 8, to = 9)
+])🤖 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/src/main/java/io/nekohasekai/sagernet/database/ProxyEntity.kt` around
lines 86 - 87, The new masterDnsVpnBean field added to the ProxyEntity class
modifies the database schema but requires a corresponding version bump and
migration. Locate the Room database version constant (currently at 8) and bump
it to 9. Then create a new migration that handles the schema change from version
8 to 9, which should include an ALTER TABLE statement to add the
masterDnsVpnBean column to the proxy_entities table. Ensure the column is
defined with an appropriate type and nullability to match the masterDnsVpnBean
property definition in ProxyEntity.
|
@coderabbitai review New commits since the last review:
|
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/moe/matsuri/nb4a/SingBoxOptions.java (1)
133-139: 💤 Low valueConsider caching the JsonArray reference.
childTree.getAsJsonArray()is called on each iteration. Extracting it before the loop would be slightly cleaner.♻️ Optional improvement
} else if (child instanceof Collection<?> && childTree.isJsonArray()) { int index = 0; + var childArray = childTree.getAsJsonArray(); for (Object item : (Collection<?>) child) { - if (index >= childTree.getAsJsonArray().size()) break; - applyHackConfig(item, childTree.getAsJsonArray().get(index)); + if (index >= childArray.size()) break; + applyHackConfig(item, childArray.get(index)); index++; }🤖 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/src/main/java/moe/matsuri/nb4a/SingBoxOptions.java` around lines 133 - 139, The method childTree.getAsJsonArray() is being called multiple times within the loop that iterates over the Collection. Extract this JsonArray reference once before the loop begins and store it in a local variable, then use that variable inside the loop instead of repeatedly calling getAsJsonArray() on the conditional check and in the applyHackConfig call.
🤖 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.
Nitpick comments:
In `@app/src/main/java/moe/matsuri/nb4a/SingBoxOptions.java`:
- Around line 133-139: The method childTree.getAsJsonArray() is being called
multiple times within the loop that iterates over the Collection. Extract this
JsonArray reference once before the loop begins and store it in a local
variable, then use that variable inside the loop instead of repeatedly calling
getAsJsonArray() on the conditional check and in the applyHackConfig call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12153985-57da-4496-ac2a-4360eddbfffe
📒 Files selected for processing (25)
.github/workflows/build.yml.github/workflows/ci.yml.github/workflows/preview.yml.github/workflows/release.ymlapp/src/main/AndroidManifest.xmlapp/src/main/java/io/nekohasekai/sagernet/Constants.ktapp/src/main/java/io/nekohasekai/sagernet/bg/BaseService.ktapp/src/main/java/io/nekohasekai/sagernet/bg/Executable.ktapp/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.ktapp/src/main/java/io/nekohasekai/sagernet/database/DataStore.ktapp/src/main/java/io/nekohasekai/sagernet/database/ProxyEntity.ktapp/src/main/java/io/nekohasekai/sagernet/fmt/KryoConverters.javaapp/src/main/java/io/nekohasekai/sagernet/fmt/TypeMap.ktapp/src/main/java/io/nekohasekai/sagernet/fmt/masterdnsvpn/MasterDnsVpnBean.javaapp/src/main/java/io/nekohasekai/sagernet/fmt/masterdnsvpn/MasterDnsVpnFmt.ktapp/src/main/java/io/nekohasekai/sagernet/ktx/Formats.ktapp/src/main/java/io/nekohasekai/sagernet/plugin/PluginManager.ktapp/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationFragment.ktapp/src/main/java/io/nekohasekai/sagernet/ui/profile/MasterDnsVpnSettingsActivity.ktapp/src/main/java/moe/matsuri/nb4a/SingBoxOptions.javaapp/src/main/res/menu/add_profile_menu.xmlapp/src/main/res/values/arrays.xmlapp/src/main/res/values/strings.xmlapp/src/main/res/xml/masterdnsvpn_preferences.xmlbuildScript/lib/masterdnsvpn.sh
The masterDnsVpnBean column was added to the proxy_entities entity without a schema version bump, so the exported schema and database version no longer matched the entity. Bump the database version 8 -> 9 and add AutoMigration(8, 9) so the new column is added in place, and commit the generated 9.json schema. Without this, existing installs fall back to a destructive rebuild (losing saved profiles) on upgrade.
The masterDnsVpnBean column was added to the ProxyEntity in an earlier commit without ever updating the exported v8 schema, so existing installs already carry the column at schema version 8. The explicit AutoMigration(8,9) then issued ALTER TABLE ADD COLUMN masterDnsVpnBean, which fails with 'duplicate column' on those databases and crash-loops the app on launch. Keep database version 9 (and 9.json) but remove the explicit 8->9 automigration so the existing fallbackToDestructiveMigration() handles the 8->9 transition, which rebuilds cleanly regardless of whether the column already exists. Verified on-device against a simulated v8-with-column database: no duplicate-column error, app launches, DB rebuilt to v9.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // opens its local SOCKS listener. Disabling async preemption avoids | ||
| // the fault; the sidecar workload is I/O-bound so scheduling latency | ||
| // is unaffected in practice. | ||
| val env = mutableMapOf("GODEBUG" to "asyncpreemptoff=1") |
There was a problem hiding this comment.
GODEBUG=asyncpreemptoff=1 disables preemption process-wide, not just during the protected dial
This env var turns off Go's signal-based goroutine preemption for the entire libmasterdnsvpn.so process lifetime, including all subsequent goroutines (e.g., the SOCKS listener, data forwarding loops). The stated reason is a fault during the first protected socket dial, but this mitigation is very broad. If the root cause is a signal-safety issue specific to the cgo/VpnService boundary during FD_CONTROL_UNIX_SOCKET sends, it would be worth confirming via GOTRACEBACK=crash logs rather than masking it globally — future Go releases may tighten runtime requirements in ways that asyncpreemptoff can't paper over.
| suspend fun awaitExternalProcessesReady() { | ||
| if (!::processes.isInitialized || processes.processCount == 0) return | ||
| val ports = config.externalIndex.flatMap { it.chain.keys }.distinct() | ||
| if (ports.isEmpty()) return | ||
|
|
||
| val hasMasterDnsVpn = config.externalIndex.any { idx -> | ||
| idx.chain.values.any { it.requireBean() is MasterDnsVpnBean } | ||
| } | ||
| val readinessTimeoutMs = if (hasMasterDnsVpn) { | ||
| maxOf(60_000L, DataStore.connectionTestTimeout.toLong()) | ||
| } else { | ||
| maxOf(1_000L, DataStore.connectionTestTimeout.toLong()) | ||
| } | ||
|
|
||
| withContext(Dispatchers.IO) { | ||
| val deadline = SystemClock.elapsedRealtime() + readinessTimeoutMs | ||
| val pending = ports.toMutableSet() | ||
| while (pending.isNotEmpty() && SystemClock.elapsedRealtime() < deadline) { | ||
| val iterator = pending.iterator() | ||
| while (iterator.hasNext()) { | ||
| val port = iterator.next() | ||
| try { | ||
| Socket().use { | ||
| it.connect(InetSocketAddress(LOCALHOST, port), 100) | ||
| } | ||
| iterator.remove() | ||
| } catch (_: IOException) { | ||
| // not ready yet | ||
| } | ||
| } | ||
| if (pending.isNotEmpty()) delay(50) | ||
| } | ||
| if (pending.isNotEmpty()) { | ||
| throw IOException("sidecar listener not ready on port(s): ${pending.joinToString()}") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
awaitExternalProcessesReady with a hard 1-second floor is a new failure mode for all existing sidecar protocols
Before this PR, startProcesses launched sidecars and returned immediately. Now awaitExternalProcessesReady blocks until every external port is connectable or the timeout fires, and then throws IOException. For Mieru, NaïVe, TrojanGo, and Hysteria profiles the effective floor is maxOf(1_000, connectionTestTimeout). On a busy low-end device those sidecars can occasionally take a few seconds to bind; previously that was harmless (the first sing-box dial would retry), but now it becomes a hard VPN-start failure. Consider a graceful path (log a warning rather than throw) for non-MasterDnsVPN sidecar timeout, or at least document the new failure contract.
Clarify that TYPE_AWG=26 deliberately skips 25, which is reserved for the MasterDnsVPN sidecar type on PR #18, so persisted type IDs stay stable when both branches merge. (Greptile)
…te resolvers Addresses CodeRabbit/Greptile review on PR #18: - Pin all actions/cache@v5 to commit 27d5ce7 (v5.0.5) across build/ci/preview/ release workflows. (CodeRabbit Major, policy) - Pin the MasterDnsVPN sidecar source to an immutable commit: masterdnsvpn.sh now honors MDVPN_COMMIT (default 7750c77) and fetches it explicitly (git init + fetch <sha> + checkout FETCH_HEAD, since --branch rejects a SHA). The workflows set MDVPN_COMMIT and key the sidecar cache on it for integrity. (CodeRabbit Major, Greptile P1) - buildMasterDnsVpn now fails fast when no resolvers are configured, matching the existing domain/key validation, instead of writing an empty resolvers file the sidecar rejects at runtime. (Greptile P2)
|
@coderabbitai review Pushed e40bd18 addressing the live review findings: Fixed:
Not changed (with rationale):
|
|
The rationale on the non-changes is well-documented — ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/io/nekohasekai/sagernet/database/SagerDatabase.kt (1)
19-26:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd a non-destructive 8→9 migration before shipping.
Bumping to
version = 9without an8 -> 9migration means upgrades from v8 will hitfallbackToDestructiveMigration()(Line 42), wiping user data (proxy_entities, groups, rules). Please add a guarded manual migration formasterDnsVpnBean(check column existence viaPRAGMA table_infobeforeALTER TABLE) and register it in the builder.Suggested direction
+import androidx.room.migration.Migration +import androidx.sqlite.db.SupportSQLiteDatabase ... abstract class SagerDatabase : RoomDatabase() { companion object { + private val MIGRATION_8_9 = object : Migration(8, 9) { + override fun migrate(database: SupportSQLiteDatabase) { + var hasColumn = false + database.query("PRAGMA table_info(`proxy_entities`)").use { c -> + val nameIdx = c.getColumnIndex("name") + while (c.moveToNext()) { + if (c.getString(nameIdx) == "masterDnsVpnBean") { + hasColumn = true + break + } + } + } + if (!hasColumn) { + database.execSQL( + "ALTER TABLE `proxy_entities` ADD COLUMN `masterDnsVpnBean` BLOB" + ) + } + } + } ... Room.databaseBuilder(SagerNet.application, SagerDatabase::class.java, Key.DB_PROFILE) + .addMigrations(MIGRATION_8_9) .setJournalMode(JournalMode.TRUNCATE)🤖 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/src/main/java/io/nekohasekai/sagernet/database/SagerDatabase.kt` around lines 19 - 26, The database version has been bumped to 9 without a corresponding migration from version 8, which will trigger a destructive migration and wipe user data. Add a new AutoMigration entry in the autoMigrations array for version 8 to 9, or implement a manual migration that safely adds the new masterDnsVpnBean column. The migration should use PRAGMA table_info to check if the masterDnsVpnBean column already exists before attempting to execute the ALTER TABLE statement, ensuring the migration is non-destructive and handles edge cases gracefully.
🤖 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 @.github/workflows/ci.yml:
- Line 34: The cache steps at lines 34, 57, 95, 104, and 120 using actions/cache
can write caches during pull_request events, which creates a security
vulnerability where untrusted PR content can poison caches used by trusted runs.
For each actions/cache usage, split the operation into two separate steps: use
actions/cache/restore to restore the cache on all events, and use
actions/cache/save to save the cache only on trusted events (add `if:
github.event_name == 'push'` condition to the save step). This prevents pull
requests from writing to the cache while still allowing cache restoration during
PR runs.
---
Outside diff comments:
In `@app/src/main/java/io/nekohasekai/sagernet/database/SagerDatabase.kt`:
- Around line 19-26: The database version has been bumped to 9 without a
corresponding migration from version 8, which will trigger a destructive
migration and wipe user data. Add a new AutoMigration entry in the
autoMigrations array for version 8 to 9, or implement a manual migration that
safely adds the new masterDnsVpnBean column. The migration should use PRAGMA
table_info to check if the masterDnsVpnBean column already exists before
attempting to execute the ALTER TABLE statement, ensuring the migration is
non-destructive and handles edge cases gracefully.
🪄 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: 8fc55d61-65fd-4e4e-a1f3-38652b1bbe2c
📒 Files selected for processing (8)
.github/workflows/build.yml.github/workflows/ci.yml.github/workflows/preview.yml.github/workflows/release.ymlapp/schemas/io.nekohasekai.sagernet.database.SagerDatabase/9.jsonapp/src/main/java/io/nekohasekai/sagernet/database/SagerDatabase.ktapp/src/main/java/io/nekohasekai/sagernet/fmt/masterdnsvpn/MasterDnsVpnFmt.ktbuildScript/lib/masterdnsvpn.sh
✅ Files skipped from review due to trivial changes (1)
- app/schemas/io.nekohasekai.sagernet.database.SagerDatabase/9.json
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/io/nekohasekai/sagernet/fmt/masterdnsvpn/MasterDnsVpnFmt.kt
- buildScript/lib/masterdnsvpn.sh
| - name: LibCore Cache | ||
| id: cache | ||
| uses: actions/cache@v5 | ||
| uses: actions/cache@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 |
There was a problem hiding this comment.
Harden cache steps against PR-based cache poisoning.
These actions/cache steps can write caches during pull_request runs, which can let untrusted PR content influence caches later restored by trusted runs. Split restore/save and gate save to trusted events only.
Suggested pattern (apply to each cache target)
- - name: Sidecars Cache
- id: cache
- uses: actions/cache@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5
+ - name: Sidecars Cache (restore)
+ id: cache
+ uses: actions/cache/restore@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5
with:
path: |
app/executableSo
key: ${{ hashFiles('.github/workflows/*', 'sidecars_status') }}-${{ env.MIERU_VERSION }}-${{ env.HYSTERIA_VERSION }}-${{ env.MDVPN_COMMIT }}-sidecars-ci
+
+ - name: Sidecars Cache (save)
+ if: github.event_name != 'pull_request' && steps.cache.outputs.cache-hit != 'true'
+ uses: actions/cache/save@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5
+ with:
+ path: |
+ app/executableSo
+ key: ${{ steps.cache.outputs.cache-primary-key }}Also applies to: 57-57, 95-95, 104-104, 120-120
🧰 Tools
🪛 zizmor (1.25.2)
[error] 34-34: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
🤖 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 @.github/workflows/ci.yml at line 34, The cache steps at lines 34, 57, 95,
104, and 120 using actions/cache can write caches during pull_request events,
which creates a security vulnerability where untrusted PR content can poison
caches used by trusted runs. For each actions/cache usage, split the operation
into two separate steps: use actions/cache/restore to restore the cache on all
events, and use actions/cache/save to save the cache only on trusted events (add
`if: github.event_name == 'push'` condition to the save step). This prevents
pull requests from writing to the cache while still allowing cache restoration
during PR runs.
Source: Linters/SAST tools
|
Re: cache-poisoning hardening (split
This PR's scope was the MasterDnsVPN sidecar; the cache pinning (immutable SHA) is done. The restore/save split will be tracked separately so it can be applied uniformly across all workflows and branches in one reviewed change. |
Picks up the protected-UDP-resolver context propagation fix on the MasterDnsVPN fork branch, so the sidecar build uses the cancellation-aware dial path.
# Conflicts: # .github/workflows/build.yml # .github/workflows/ci.yml # .github/workflows/preview.yml # .github/workflows/release.yml # app/schemas/io.nekohasekai.sagernet.database.SagerDatabase/9.json # app/src/main/java/io/nekohasekai/sagernet/database/ProxyEntity.kt # app/src/main/java/io/nekohasekai/sagernet/plugin/PluginManager.kt # app/src/main/java/moe/matsuri/nb4a/SingBoxOptions.java
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/release.yml:
- Around line 17-18: The `contents: write` permission is currently set at the
workflow level, which grants write access to all jobs unnecessarily. Remove the
`permissions: contents: write` section from the workflow-level configuration and
instead add it only to the specific job that handles release publication. This
ensures only the job that actually publishes the release has write access to
repository contents, following the principle of least privilege.
🪄 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: 49441cfd-c130-48b6-952a-50c3a13b821d
📒 Files selected for processing (15)
.github/workflows/build.yml.github/workflows/ci.yml.github/workflows/preview.yml.github/workflows/release.ymlapp/schemas/io.nekohasekai.sagernet.database.SagerDatabase/9.jsonapp/src/main/AndroidManifest.xmlapp/src/main/java/io/nekohasekai/sagernet/database/ProxyEntity.ktapp/src/main/java/io/nekohasekai/sagernet/fmt/KryoConverters.javaapp/src/main/java/io/nekohasekai/sagernet/fmt/TypeMap.ktapp/src/main/java/io/nekohasekai/sagernet/plugin/PluginManager.ktapp/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationFragment.ktapp/src/main/java/moe/matsuri/nb4a/SingBoxOptions.javaapp/src/main/res/menu/add_profile_menu.xmlapp/src/main/res/values/strings.xmlbuildScript/lib/masterdnsvpn.sh
🚧 Files skipped from review as they are similar to previous changes (9)
- app/src/main/res/menu/add_profile_menu.xml
- app/src/main/java/io/nekohasekai/sagernet/plugin/PluginManager.kt
- app/src/main/java/io/nekohasekai/sagernet/fmt/TypeMap.kt
- app/src/main/res/values/strings.xml
- app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationFragment.kt
- app/src/main/AndroidManifest.xml
- app/src/main/java/io/nekohasekai/sagernet/fmt/KryoConverters.java
- app/src/main/java/io/nekohasekai/sagernet/database/ProxyEntity.kt
- buildScript/lib/masterdnsvpn.sh
Workflow-level contents:write granted a write-capable token to every job. Default to read-only and grant write only to the publish job that uploads the release. (CodeRabbit)
|
Addressed the workflow-permissions finding in f702e80: On the cache-poisoning hardening (split |
awaitExternalProcessesReady previously threw IOException (hard VPN-start failure) for any sidecar that didn't bind within maxOf(1s, connectionTestTimeout). MasterDnsVPN genuinely needs that gate (it crashed if dialed before binding), but Mieru/Naive/TrojanGo/Hysteria were historically fire-and-forget with first-dial retry. Now only MasterDnsVPN throws on timeout; the others log a warning and continue. (Greptile, PR #18 follow-up)
1 participant
Draft — do not merge yet. Depends on the paired MasterDnsVPN client change
(Android VpnService socket-protection hook) landing first, since this branch's build
script pulls the client from that branch.
Summary
Integrates
masterking32/MasterDnsVPN(DNS-tunneling VPN) as a bundled native-binarysidecar — a new protocol type — reusing the Mieru/Hysteria2 sidecar pattern. VPN-mode
capable: the forked client protects its upstream resolver sockets from the TUN via
FD_CONTROL_UNIX_SOCKET, pointed at libcore's existingprotect_pathserver.Emulator validation (x86_64) — PASSED
MainActivityresumed, no crash (newTYPE_MASTERDNSVPNlibmasterdnsvpn.so(built from the fork) extracted to nativeLibraryDir, executable.buildMasterDnsVpnConfig+ a resolvers file: the clientparses every field (Protocol SOCKS5, Encryption, Least-Loss strategy, domains,
"Loaded Resolvers: 2 endpoints"), starts, and reaches out to the resolvers.
(MTU test fails only because the test domain has no real server — expected.)
FD_CONTROL_UNIX_SOCKETset to a stub server, the stubreceived
PROTECT REQUEST: payload=1 byte, fds=[8],[9],[12]...— i.e. every upstreamresolver socket fd is sent for protection, using the exact
0x01+ SCM_RIGHTS wireprotocol that libcore's
protect_serverimplements. This is the proof VPN mode won't loop.Changes
MasterDnsVpnBean(TYPE_MASTERDNSVPN=25): full field set;needExternal=true,canMapping=false.MasterDnsVpnFmt.buildMasterDnsVpnConfig(JSON, uppercase keys, FD_CONTROL_UNIX_SOCKET,loopback SOCKS5 no-auth; advancedJson merged with try-catch) +
resolverLines().libmasterdnsvpn.so -json <cfg> -resolvers <file>.buildScript/lib/masterdnsvpn.sh: cross-compile the fork (4 ABIs); validates Go >= 1.25.Defaults / security
must pick a cipher + key that matches their server.
NOT done in this PR (follow-ups)
sidecarsjob needs amasterdnsvpnbuild step + verify across the 4workflows. Deferred because the gh token lacks
workflowscope (and SSH push is currentlyunavailable). To be added when push access to
.github/workflows/is restored.the prior commit was fully emulator-validated and
these changes don't alter the validated runtime path structurally.
No sing-box core change.
Greptile Summary
This PR integrates
MasterDnsVPNas a new native-binary sidecar protocol (TYPE_MASTERDNSVPN=25), following the Mieru/Hysteria2 pattern. It adds the full data model, JSON config builder, share-link serialization, settings UI, CI wiring, and a cross-compilation build script for all four Android ABIs.MasterDnsVpnBean+buildMasterDnsVpnConfiggenerate a loopback SOCKS5 config with VPN-mode socket protection viaFD_CONTROL_UNIX_SOCKET, andawaitExternalProcessesReadyblocks VPN startup until the listener is connectable (up to 60 s for MasterDnsVPN, 1 s floor for others).masterdnsvpn.shcross-compiles the forked Go client; all four workflow files now include the sidecar in cache keys and artifact verification, andactions/cachewas pinned to a commit SHA.proxy_entitiesgains amasterDnsVpnBean BLOBcolumn (schema version 8 → 9), but theautoMigrationslist inSagerDatabase.ktstill stops atfrom = 7, to = 8— the missingAutoMigration(from = 8, to = 9)entry means devices upgrading from version 8 will hitfallbackToDestructiveMigration()and lose all saved proxies.Confidence Score: 3/5
Not safe to merge: the database schema bumps from 8 to 9 but the AutoMigration entry for that step is still missing, meaning any user upgrading from the prior release will have all proxy profiles silently wiped by Room's destructive-migration fallback.
The core protocol implementation is solid and follows the established sidecar pattern well. However, the missing AutoMigration(from=8, to=9) in SagerDatabase.kt is a live data-loss path for users upgrading from the previous release, unaddressed across three prior rounds of review. The new awaitExternalProcessesReady call in startProcesses also introduces a hard IOException failure path for all existing sidecar types (Mieru, NaïVe, TrojanGo, Hysteria) that was not present before this PR.
app/src/main/java/io/nekohasekai/sagernet/database/SagerDatabase.kt (AutoMigration 8→9 entry still absent) and app/src/main/java/io/nekohasekai/sagernet/bg/proto/BoxInstance.kt (awaitExternalProcessesReady now throws for all sidecar protocols).
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant BS as BaseService participant BI as BoxInstance participant GP as GuardedProcessPool participant MC as libmasterdnsvpn.so participant LP as libcore protect_path BS->>BI: init() BI->>BI: buildMasterDnsVpnConfig(port, protectPath) Note over BI: validates domains, key, resolvers BI->>BI: loadConfig() → Libcore.newSingBoxInstance BS->>BI: launch() BI->>GP: "start([libmasterdnsvpn.so, -json cfg, -resolvers file], GODEBUG=asyncpreemptoff=1)" GP->>MC: spawn process BS->>BI: awaitExternalProcessesReady() loop every 50ms until port connectable or 60s timeout BI->>BI: Socket().connect(127.0.0.1:port, 100ms) end Note over BI: throws IOException if timeout MC->>LP: FD_CONTROL_UNIX_SOCKET protect request (SCM_RIGHTS) LP-->>MC: protected fd returned MC->>MC: DNS tunnel established MC-->>BI: SOCKS5 listener bound on 127.0.0.1:port BI->>BI: box.start() Note over BI: sing-box dials 127.0.0.1:port via socks outbound%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant BS as BaseService participant BI as BoxInstance participant GP as GuardedProcessPool participant MC as libmasterdnsvpn.so participant LP as libcore protect_path BS->>BI: init() BI->>BI: buildMasterDnsVpnConfig(port, protectPath) Note over BI: validates domains, key, resolvers BI->>BI: loadConfig() → Libcore.newSingBoxInstance BS->>BI: launch() BI->>GP: "start([libmasterdnsvpn.so, -json cfg, -resolvers file], GODEBUG=asyncpreemptoff=1)" GP->>MC: spawn process BS->>BI: awaitExternalProcessesReady() loop every 50ms until port connectable or 60s timeout BI->>BI: Socket().connect(127.0.0.1:port, 100ms) end Note over BI: throws IOException if timeout MC->>LP: FD_CONTROL_UNIX_SOCKET protect request (SCM_RIGHTS) LP-->>MC: protected fd returned MC->>MC: DNS tunnel established MC-->>BI: SOCKS5 listener bound on 127.0.0.1:port BI->>BI: box.start() Note over BI: sing-box dials 127.0.0.1:port via socks outboundComments Outside Diff (3)
app/src/main/java/io/nekohasekai/sagernet/database/SagerDatabase.kt, line 20-26 (link)The database version bumps from 8 to 9, but the
autoMigrationslist only goes up tofrom = 7, to = 8. BecausefallbackToDestructiveMigration()is set (line 42), Room will wipe the entireproxy_entitiestable for any user upgrading from DB version 8 — all proxy profiles are silently deleted. Adding a nullable BLOB column is exactly what Room AutoMigration handles automatically;AutoMigration(from = 8, to = 9)just needs to be appended to the list alongside the schema JSON file already committed as9.json.app/src/main/java/io/nekohasekai/sagernet/database/SagerDatabase.kt, line 20-26 (link)The database version bumps from 8 to 9, but the
autoMigrationslist only goes up tofrom = 7, to = 8. BecausefallbackToDestructiveMigration()is set on line 42, Room will drop and recreate theproxy_entitiestable for any user whose device holds DB version 8 — silently deleting every saved proxy profile. Adding a nullable BLOB column (formasterDnsVpnBean) is exactly the case Room AutoMigration handles automatically with zero extra code; anAutoMigration(from = 8, to = 9)annotation alongside the already-committed9.jsonschema file is all that is needed.app/src/main/java/io/nekohasekai/sagernet/database/SagerDatabase.kt, line 18-26 (link)AutoMigration(from = 8, to = 9)will silently wipe all proxy profiles on upgradeThe database version bumps from 8 to 9, but the
autoMigrationslist stops atfrom = 7, to = 8. BecausefallbackToDestructiveMigration()is set (line 42), Room will drop and recreate theproxy_entitiestable for any user whose device holds DB version 8 — silently deleting every saved proxy profile. Adding a nullable BLOB column is exactly what Room AutoMigration handles without extra code; appendingAutoMigration(from = 8, to = 9)alongside the committed9.jsonschema file is all that is needed.Reviews (6): Last reviewed commit: "ci: scope release.yml contents:write to ..." | Re-trigger Greptile