feat: add helper runtime and packaging readiness#114
Conversation
Connect the dormant D022-D024 foundations to an opt-in TLS 1.3 helper runtime with fixed endpoints and mutually signed application messages. Require explicit pairing and namespace authorization, exact Syncthing and host-bind preflight, protected credential lifecycle handling, bounded recovery, and fail-closed operation reconstruction. Keep existing installations dormant unless both operator configuration values are present, and keep upload, download, and roundtrip evidence unset.
Add the explicit rootful Docker host-bind installer and CI proofs for install, upgrade, downgrade, and forward recovery using immutable local image IDs. Document supported and unsupported deployment rows, compatibility, privacy, existing-user, retention, and rollback boundaries. Keep publication and production rollout deferred, and keep upload, download, and roundtrip evidence explicitly unset.
|
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:
📝 WalkthroughWalkthroughThe PR adds an opt-in, operator-configured diagnostics runtime with signed CBOR capability flows, namespace authorization and recovery, TLS 1.3 serving, hardened Docker packaging, expanded Syncthing validation, documentation, tests, and CI coverage. ChangesDiagnostics runtime and packaging
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Mark the explicit Docker installer and both runtime packaging proof entrypoints executable so GitHub Actions can invoke the standard-Linux gate directly.
Filter the runtime constraint comparison to persistent bind mounts and remove Docker's empty template line while retaining the separate exact tmpfs assertion.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
notify/diagnostics_contract_model_test.go (1)
369-378: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winNew
diagnostics_capability_protocol.goisn't checked to stay operationally dormant.This PR adds
capabilityProtocolCarrieras an allowed home for capability strings/roundtrip domains (lines 292-324), putting it in the same architectural role asdiagnostics_namespace_protocol.go,diagnostics_upload_protocol.go, anddiagnostics_response_protocol.go— all of which are enforced here to never containListenAndServe/http.Server/net.Listen/RelayClient/SyncthingClient. The new file is missing from this list, so the exact invariant this test suite exists to enforce doesn't cover it.♻️ Proposed fix
for _, path := range []string{ filepath.Join(repoRoot, "notify", "diagnostics_pairing_manager.go"), filepath.Join(repoRoot, "notify", "diagnostics_pairing_crypto.go"), filepath.Join(repoRoot, "notify", "diagnostics_pairing_store.go"), filepath.Join(repoRoot, "notify", "diagnostics_namespace_protocol.go"), filepath.Join(repoRoot, "notify", "diagnostics_upload_protocol.go"), filepath.Join(repoRoot, "notify", "diagnostics_upload_attestor.go"), filepath.Join(repoRoot, "notify", "diagnostics_response_protocol.go"), filepath.Join(repoRoot, "notify", "diagnostics_response_foundation.go"), + filepath.Join(repoRoot, "notify", "diagnostics_capability_protocol.go"), } {🤖 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 `@notify/diagnostics_contract_model_test.go` around lines 369 - 378, Extend the file list in the operational-dormancy test to include diagnostics_capability_protocol.go alongside the other protocol files. Keep the existing forbidden-symbol checks unchanged so capabilityProtocolCarrier receives the same enforcement against ListenAndServe, http.Server, net.Listen, RelayClient, and SyncthingClient.
🧹 Nitpick comments (2)
notify/diagnostics_capability_protocol_test.go (1)
154-180: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest name promises "WrongTuple" coverage it doesn't exercise.
TestDiagnosticsCapabilityRejectsStaleAndWrongTupleonly asserts onvalidateDiagnosticsCapabilityClock(staleness). There's no case that mutates the homeserver/folder/key-ID tuple and assertsdiagnosticsCapabilityMatchesBindingrejects it — the "WrongTuple" half of the name is untested. Given this decoder gate is the core of the capability binding contract, worth closing the gap.♻️ Suggested addition
wrongFolder := diagnosticsUploadBinding{ homeserverBinding: bytes.Repeat([]byte{0x83}, 32), folderBinding: bytes.Repeat([]byte{0xAA}, 32), // mismatched vs. query's field 6 appPublicKey: context.appPublicKey, // ... } if diagnosticsCapabilityMatchesBinding(message, wrongFolder) { t.Fatal("capability query matched an unrelated binding tuple") }🤖 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 `@notify/diagnostics_capability_protocol_test.go` around lines 154 - 180, Extend TestDiagnosticsCapabilityRejectsStaleAndWrongTuple to construct a diagnosticsUploadBinding using the decoded message’s app key but a different folder binding, then assert diagnosticsCapabilityMatchesBinding returns false. Keep the existing stale-clock assertion and use the query’s field-6 value as the baseline so the test specifically covers rejection of a mismatched homeserver/folder/key-ID tuple.notify/diagnostics_runtime_namespace.go (1)
205-298: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftNarrow the mutex scope around
authorize()
runtime.mutexstays locked throughopenDiagnosticsNamespaceRoot,ScanFixedLayout*,preflightNamespaceHandle(with a 3s Syncthing timeout), and the immutable read/write path. That still serializes every namespace authorization behind one slow mount or Syncthing response, so unrelated folders on the same runtime can stall.Consider moving the shared-state lock to just the in-memory checks and revalidating before commit.
🤖 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 `@notify/diagnostics_runtime_namespace.go` around lines 205 - 298, Reduce the scope of runtime.mutex in diagnosticsNamespaceRuntime.authorize so it covers only the in-memory rate-limit, session lookup, and state-validation operations, not filesystem access, scans, preflight, or immutable record work. Release the lock before openDiagnosticsNamespaceRoot and reacquire it immediately before the authorization commit, revalidating the relevant session/state and preserving idempotent authorizationAlreadyApplied behavior to prevent stale results or concurrent commits.Source: Path instructions
🤖 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 `@notify/diagnostics_pairing_lifecycle.go`:
- Around line 572-694: Move the prepare(plan) invocation in
observeLifecycleCapability outside manager.mutex, while retaining the lock for
state validation and the final atomic commit. Restructure the method so the lock
is released before prepareLifecycleCommit-related work, then reacquired or
otherwise revalidate the transition under the store update before applying
changes; preserve all existing digest, stage, confirmation, and readiness
checks.
---
Outside diff comments:
In `@notify/diagnostics_contract_model_test.go`:
- Around line 369-378: Extend the file list in the operational-dormancy test to
include diagnostics_capability_protocol.go alongside the other protocol files.
Keep the existing forbidden-symbol checks unchanged so capabilityProtocolCarrier
receives the same enforcement against ListenAndServe, http.Server, net.Listen,
RelayClient, and SyncthingClient.
---
Nitpick comments:
In `@notify/diagnostics_capability_protocol_test.go`:
- Around line 154-180: Extend TestDiagnosticsCapabilityRejectsStaleAndWrongTuple
to construct a diagnosticsUploadBinding using the decoded message’s app key but
a different folder binding, then assert diagnosticsCapabilityMatchesBinding
returns false. Keep the existing stale-clock assertion and use the query’s
field-6 value as the baseline so the test specifically covers rejection of a
mismatched homeserver/folder/key-ID tuple.
In `@notify/diagnostics_runtime_namespace.go`:
- Around line 205-298: Reduce the scope of runtime.mutex in
diagnosticsNamespaceRuntime.authorize so it covers only the in-memory
rate-limit, session lookup, and state-validation operations, not filesystem
access, scans, preflight, or immutable record work. Release the lock before
openDiagnosticsNamespaceRoot and reacquire it immediately before the
authorization commit, revalidating the relevant session/state and preserving
idempotent authorizationAlreadyApplied behavior to prevent stale results or
concurrent commits.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ba8be8d-3781-46c0-84d4-365a502b911e
⛔ Files ignored due to path filters (1)
notify/go.sumis excluded by!**/*.sumand included by**/*
📒 Files selected for processing (46)
.github/dependabot.yml.github/workflows/ci.ymlPRIVACY.mddocs/architecture.mddocs/helper-runtime-packaging-readiness.mdnotify/Dockerfilenotify/README.mdnotify/diagnostics_capability_protocol.gonotify/diagnostics_capability_protocol_test.gonotify/diagnostics_contract_model_test.gonotify/diagnostics_device_id.gonotify/diagnostics_device_id_test.gonotify/diagnostics_namespace_filesystem_linux.gonotify/diagnostics_namespace_filesystem_unsupported.gonotify/diagnostics_namespace_installer.gonotify/diagnostics_namespace_installer_linux.gonotify/diagnostics_namespace_linux_test.gonotify/diagnostics_namespace_protocol.gonotify/diagnostics_operator.gonotify/diagnostics_pairing_lifecycle.gonotify/diagnostics_pairing_permissions_unix.gonotify/diagnostics_pairing_permissions_windows.gonotify/diagnostics_pairing_store.gonotify/diagnostics_pairing_test.gonotify/diagnostics_response_foundation.gonotify/diagnostics_runtime_config.gonotify/diagnostics_runtime_config_test.gonotify/diagnostics_runtime_ignore.gonotify/diagnostics_runtime_integration_test.gonotify/diagnostics_runtime_mount.gonotify/diagnostics_runtime_namespace.gonotify/diagnostics_runtime_server.gonotify/diagnostics_runtime_state.gonotify/diagnostics_runtime_tls.gonotify/diagnostics_runtime_tls_test.gonotify/go.modnotify/main.gonotify/relay.gonotify/relay_test.gonotify/scripts/diagnostics-docker.shnotify/syncthing.gonotify/tests/m4-docker/Dockerfilenotify/tests/runtime-packaging/mock/Dockerfilenotify/tests/runtime-packaging/mock/main.gonotify/tests/runtime-packaging/run-linux-host.shnotify/tests/runtime-packaging/run.sh
👮 Files not reviewed due to content moderation or server errors (8)
- notify/diagnostics_operator.go
- notify/diagnostics_runtime_server.go
- notify/diagnostics_runtime_state.go
- notify/diagnostics_runtime_tls.go
- notify/main.go
- notify/relay.go
- notify/relay_test.go
- notify/tests/runtime-packaging/mock/main.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*
⚙️ CodeRabbit configuration file
**/*: VaultSync syncs private Obsidian notes through Syncthing. Treat data loss,
privacy leaks, security regressions, and broken sync behavior as high priority.
Do not nitpick formatting unless it affects maintainability, correctness, or public API clarity.
Flag any accidental logging, telemetry, crash reporting, or network transfer of note contents,
vault paths, filenames with private context, API keys, APNs tokens, relay keys, or security-scoped bookmark data.
Files:
notify/tests/runtime-packaging/mock/Dockerfilenotify/diagnostics_pairing_permissions_unix.gonotify/diagnostics_device_id_test.gonotify/diagnostics_pairing_permissions_windows.gonotify/Dockerfilenotify/diagnostics_response_foundation.gonotify/diagnostics_device_id.gonotify/diagnostics_namespace_protocol.gonotify/diagnostics_namespace_filesystem_unsupported.gonotify/relay_test.gonotify/diagnostics_runtime_mount.gonotify/diagnostics_runtime_ignore.gonotify/diagnostics_pairing_test.gonotify/tests/runtime-packaging/mock/main.godocs/architecture.mdnotify/diagnostics_runtime_tls.gonotify/diagnostics_capability_protocol.gonotify/diagnostics_runtime_config.gonotify/relay.gonotify/tests/m4-docker/Dockerfilenotify/diagnostics_capability_protocol_test.godocs/helper-runtime-packaging-readiness.mdnotify/diagnostics_runtime_tls_test.gonotify/diagnostics_namespace_linux_test.gonotify/README.mdnotify/go.modnotify/diagnostics_runtime_config_test.gonotify/diagnostics_contract_model_test.gonotify/diagnostics_pairing_store.gonotify/diagnostics_namespace_filesystem_linux.gonotify/diagnostics_runtime_state.goPRIVACY.mdnotify/diagnostics_namespace_installer.gonotify/scripts/diagnostics-docker.shnotify/tests/runtime-packaging/run.shnotify/diagnostics_pairing_lifecycle.gonotify/syncthing.gonotify/diagnostics_runtime_server.gonotify/diagnostics_runtime_namespace.gonotify/diagnostics_operator.gonotify/diagnostics_namespace_installer_linux.gonotify/tests/runtime-packaging/run-linux-host.shnotify/main.gonotify/diagnostics_runtime_integration_test.go
notify/**/*.{sh,go}
📄 CodeRabbit inference engine (README.md)
The optional notify sidecar must support server-side wake-up requests and must not receive notes, file or folder names, or vault structure.
Files:
notify/diagnostics_pairing_permissions_unix.gonotify/diagnostics_device_id_test.gonotify/diagnostics_pairing_permissions_windows.gonotify/diagnostics_response_foundation.gonotify/diagnostics_device_id.gonotify/diagnostics_namespace_protocol.gonotify/diagnostics_namespace_filesystem_unsupported.gonotify/relay_test.gonotify/diagnostics_runtime_mount.gonotify/diagnostics_runtime_ignore.gonotify/diagnostics_pairing_test.gonotify/tests/runtime-packaging/mock/main.gonotify/diagnostics_runtime_tls.gonotify/diagnostics_capability_protocol.gonotify/diagnostics_runtime_config.gonotify/relay.gonotify/diagnostics_capability_protocol_test.gonotify/diagnostics_runtime_tls_test.gonotify/diagnostics_namespace_linux_test.gonotify/diagnostics_runtime_config_test.gonotify/diagnostics_contract_model_test.gonotify/diagnostics_pairing_store.gonotify/diagnostics_namespace_filesystem_linux.gonotify/diagnostics_runtime_state.gonotify/diagnostics_namespace_installer.gonotify/scripts/diagnostics-docker.shnotify/tests/runtime-packaging/run.shnotify/diagnostics_pairing_lifecycle.gonotify/syncthing.gonotify/diagnostics_runtime_server.gonotify/diagnostics_runtime_namespace.gonotify/diagnostics_operator.gonotify/diagnostics_namespace_installer_linux.gonotify/tests/runtime-packaging/run-linux-host.shnotify/main.gonotify/diagnostics_runtime_integration_test.go
**/*.{md,sh,go,swift}
📄 CodeRabbit inference engine (README.md)
Keep Relay-side request observation and wake-ups received on the iPhone as separate diagnostics evidence; one must not be treated as proof of the other.
Files:
notify/diagnostics_pairing_permissions_unix.gonotify/diagnostics_device_id_test.gonotify/diagnostics_pairing_permissions_windows.gonotify/diagnostics_response_foundation.gonotify/diagnostics_device_id.gonotify/diagnostics_namespace_protocol.gonotify/diagnostics_namespace_filesystem_unsupported.gonotify/relay_test.gonotify/diagnostics_runtime_mount.gonotify/diagnostics_runtime_ignore.gonotify/diagnostics_pairing_test.gonotify/tests/runtime-packaging/mock/main.godocs/architecture.mdnotify/diagnostics_runtime_tls.gonotify/diagnostics_capability_protocol.gonotify/diagnostics_runtime_config.gonotify/relay.gonotify/diagnostics_capability_protocol_test.godocs/helper-runtime-packaging-readiness.mdnotify/diagnostics_runtime_tls_test.gonotify/diagnostics_namespace_linux_test.gonotify/README.mdnotify/diagnostics_runtime_config_test.gonotify/diagnostics_contract_model_test.gonotify/diagnostics_pairing_store.gonotify/diagnostics_namespace_filesystem_linux.gonotify/diagnostics_runtime_state.goPRIVACY.mdnotify/diagnostics_namespace_installer.gonotify/scripts/diagnostics-docker.shnotify/tests/runtime-packaging/run.shnotify/diagnostics_pairing_lifecycle.gonotify/syncthing.gonotify/diagnostics_runtime_server.gonotify/diagnostics_runtime_namespace.gonotify/diagnostics_operator.gonotify/diagnostics_namespace_installer_linux.gonotify/tests/runtime-packaging/run-linux-host.shnotify/main.gonotify/diagnostics_runtime_integration_test.go
notify/**/*.go
⚙️ CodeRabbit configuration file
notify/**/*.go: Review goroutine lifecycle, context cancellation, HTTP timeouts, signal handling, debounce behavior,
Syncthing REST API polling, relay API calls, error classification, and API-key handling.
Flag leaked request bodies, note metadata, Syncthing API keys, relay keys, or APNs-related secrets.
Files:
notify/diagnostics_pairing_permissions_unix.gonotify/diagnostics_device_id_test.gonotify/diagnostics_pairing_permissions_windows.gonotify/diagnostics_response_foundation.gonotify/diagnostics_device_id.gonotify/diagnostics_namespace_protocol.gonotify/diagnostics_namespace_filesystem_unsupported.gonotify/relay_test.gonotify/diagnostics_runtime_mount.gonotify/diagnostics_runtime_ignore.gonotify/diagnostics_pairing_test.gonotify/tests/runtime-packaging/mock/main.gonotify/diagnostics_runtime_tls.gonotify/diagnostics_capability_protocol.gonotify/diagnostics_runtime_config.gonotify/relay.gonotify/diagnostics_capability_protocol_test.gonotify/diagnostics_runtime_tls_test.gonotify/diagnostics_namespace_linux_test.gonotify/diagnostics_runtime_config_test.gonotify/diagnostics_contract_model_test.gonotify/diagnostics_pairing_store.gonotify/diagnostics_namespace_filesystem_linux.gonotify/diagnostics_runtime_state.gonotify/diagnostics_namespace_installer.gonotify/diagnostics_pairing_lifecycle.gonotify/syncthing.gonotify/diagnostics_runtime_server.gonotify/diagnostics_runtime_namespace.gonotify/diagnostics_operator.gonotify/diagnostics_namespace_installer_linux.gonotify/main.gonotify/diagnostics_runtime_integration_test.go
notify/Dockerfile
⚙️ CodeRabbit configuration file
notify/Dockerfile: Check container security, minimal runtime surface, non-root execution where practical,
HEALTHCHECK behavior, reproducible builds, and safe handling of runtime secrets.
Files:
notify/Dockerfile
docs/**/*.md
📄 CodeRabbit inference engine (docs/troubleshooting.md)
docs/**/*.md: After applying each troubleshooting fix, retry from the app to confirm whether the issue is resolved.
For a missing Device ID or “Sync Engine Not Running,” force-close and reopen VaultSync, keep it foregrounded for 20–30 seconds, confirm the Device ID, rescan the vault, and reboot if necessary.
vaultsync-notifymust read the Syncthing API key fromconfig.xml; permission or authentication failures should be diagnosed as wrong-file or read-permission problems before changing credentials.
Runvaultsync-notifyas the user owning Syncthing’s mode-0600config.xml, or configure the appropriate containerPUID/PGID.
SetSYNCTHING_CONFIGwhen the Syncthing configuration is not auto-detected, and remove or correctSYNCTHING_API_KEYoverrides that replace automatic detection.
For relay failures, verify internet access,RELAY_URL, server egress rules, relay health with curl, and the app’s relay diagnostics; confirm delivery through an updated “Last Trigger Received” timestamp.
A relay HTTP 429 from--doctorcounts as success because it proves the trigger endpoint is reachable; an inactive subscription is a warning fixed through app subscription or provisioning.
Peer-state warnings from--doctormust not fail the command: offline peers are normal, and--healthcheckmust skip peer-state diagnostics.
For missing pending shares, verify the desktop folder shares the exact iOS Device ID, ensure Syncthing is online and unpaused, refresh pending shares, and re-share if needed.
For APNs issues, retry APNs registration, verify a token appears, retry provisioning, and confirm delivery with “Last Trigger Received”; notification permission is not required for silent wake-ups.
When connecting Obsidian storage, chooseOn My iPhone → Obsidianor the vault root containing.obsidian, then confirm detected vaults or pending shares.
VaultSync must not move, recreate, or delete vault folders automatically; recovery from moved or deleted folders requires ma...
Files:
docs/architecture.mddocs/helper-runtime-packaging-readiness.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Review public documentation for technical accuracy, privacy/security claims, App Store-facing wording,
setup correctness, and consistency with the free app plus optional Cloud Relay subscription model.
Files:
docs/architecture.mddocs/helper-runtime-packaging-readiness.mdnotify/README.mdPRIVACY.md
.github/workflows/**/*.yml
⚙️ CodeRabbit configuration file
.github/workflows/**/*.yml: Review CI for correct Go test tags, Xcode/iOS simulator assumptions, secret scoping,
dependency integrity, permissions, Docker publishing safety, and unnecessary privilege escalation.
Files:
.github/workflows/ci.yml
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: psimaker/vaultsync
Timestamp: 2026-07-14T05:52:52.914Z
Learning: Do not infer publication, registry digests, SBOM or vulnerability results, production rollout, production rollback evidence, upload, download, or roundtrip evidence from this source-readiness milestone.
🪛 ast-grep (0.44.1)
notify/tests/runtime-packaging/mock/main.go
[warning] 98-104: TLS certificate verification is disabled by setting InsecureSkipVerify: true on the tls.Config. This makes the connection vulnerable to man-in-the-middle attacks because the server's certificate chain and host name are not validated. Remove InsecureSkipVerify (or set it to false) and provide a proper RootCAs pool to trust custom certificates instead.
Context: tls.Config{
// #nosec G402 -- this isolated proof reads the self-signed endpoint's
// public-key pin; production clients verify the configured SPKI pin.
InsecureSkipVerify: true,
MinVersion: tls.VersionTLS13,
MaxVersion: tls.VersionTLS13,
}
Note: [CWE-295] Improper Certificate Validation.
(tls-insecure-skip-verify-go)
[warning] 79-84: This http.Server is constructed without a ReadTimeout. Without a read timeout, a slow or malicious client can hold connections open indefinitely (e.g. a Slowloris attack), exhausting server resources and causing a denial of service. Set ReadTimeout (and ideally ReadHeaderTimeout, WriteTimeout, and IdleTimeout) on the http.Server to bound how long the server waits while reading a request.
Context: http.Server{
Addr: listenAddress,
Handler: mux,
ReadHeaderTimeout: 5 * time.Second,
IdleTimeout: 10 * time.Second,
}
Note: [CWE-400] Uncontrolled Resource Consumption.
(http-server-missing-read-timeout-go)
notify/scripts/diagnostics-docker.sh
[error] 40-40: eval is invoked on a variable, parameter expansion, or command-substitution result, which re-parses the value as shell code. If any part of that value is attacker-controlled (arguments, environment, file contents, network output), it allows arbitrary command execution. Do not eval dynamic data: invoke the command directly with proper quoting (e.g. "$cmd" "$arg"), use arrays for argument lists (cmd=(prog --flag "$value"); "${cmd[@]}"), or restrict input to a validated allowlist before running it.
Context: eval "variable_value=${$variable_name:-}"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(eval-on-variable-bash)
notify/tests/runtime-packaging/run.sh
[warning] 17-17: A credential-bearing variable (e.g. PASSWORD, PASSWD, SECRET, TOKEN, API_KEY) is assigned a hardcoded string literal. Secrets committed to a script are exposed in source control, process listings, and shell history, and cannot be rotated without a code change. Read the value from a secrets manager or an injected environment variable at runtime instead (e.g. PASSWORD="${DB_PASSWORD:?must be set}"), and never commit the literal.
Context: api_key=runtime-packaging-api-key-sentinel
Note: [CWE-798] Use of Hard-coded Credentials.
(hardcoded-password-assignment-bash)
notify/tests/runtime-packaging/run-linux-host.sh
[warning] 18-18: A credential-bearing variable (e.g. PASSWORD, PASSWD, SECRET, TOKEN, API_KEY) is assigned a hardcoded string literal. Secrets committed to a script are exposed in source control, process listings, and shell history, and cannot be rotated without a code change. Read the value from a secrets manager or an injected environment variable at runtime instead (e.g. PASSWORD="${DB_PASSWORD:?must be set}"), and never commit the literal.
Context: api_key=runtime-linux-api-key-sentinel
Note: [CWE-798] Use of Hard-coded Credentials.
(hardcoded-password-assignment-bash)
notify/diagnostics_runtime_integration_test.go
[warning] 1007-1007: TLS certificate verification is disabled by setting InsecureSkipVerify: true on the tls.Config. This makes the connection vulnerable to man-in-the-middle attacks because the server's certificate chain and host name are not validated. Remove InsecureSkipVerify (or set it to false) and provide a proper RootCAs pool to trust custom certificates instead.
Context: tls.Config{InsecureSkipVerify: true, MinVersion: tls.VersionTLS12, MaxVersion: tls.VersionTLS12}
Note: [CWE-295] Improper Certificate Validation.
(tls-insecure-skip-verify-go)
🪛 OpenGrep (1.25.0)
notify/tests/runtime-packaging/mock/main.go
[ERROR] 99-105: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 99-105: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
notify/diagnostics_runtime_integration_test.go
[ERROR] 1008-1008: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 1008-1008: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
🔇 Additional comments (45)
notify/diagnostics_runtime_tls_test.go (1)
1-95: LGTM!notify/diagnostics_pairing_permissions_unix.go (1)
29-36: LGTM!notify/diagnostics_pairing_permissions_windows.go (1)
22-25: LGTM!.github/dependabot.yml (1)
59-72: LGTM!.github/workflows/ci.yml (1)
106-118: LGTM!Also applies to: 145-161, 194-196, 252-252
PRIVACY.md (1)
3-3: LGTM!Also applies to: 76-143, 159-169, 181-189
notify/tests/runtime-packaging/run-linux-host.sh (1)
1-214: LGTM!docs/architecture.md (1)
94-199: LGTM!docs/helper-runtime-packaging-readiness.md (1)
1-243: LGTM!notify/Dockerfile (1)
1-21: LGTM! Confirmed via web search that Go 1.26 and Alpine 3.24 are both real, current releases, so the pinned digests target valid, up-to-date base images.notify/README.md (1)
119-120: LGTM!Also applies to: 132-215, 249-249, 267-267
notify/diagnostics_response_foundation.go (1)
56-59: LGTM!notify/scripts/diagnostics-docker.sh (2)
1-378: Rest of the installer — supported-host/storage checks, folder/config canonicalization, device/inode pinning, mount-alias handling, and the command dispatch table — looks solid and matches the documented support matrix.
105-152: 🔒 Security & Privacy
listen_addressis already range-checked downstream.notify/diagnostics_runtime_config.gorejects non-loopback/link-local/private/CGNAT bind addresses beforenet.Listen, so the shell script’s character-only preflight is not the enforcement point.> Likely an incorrect or invalid review comment.notify/tests/runtime-packaging/mock/Dockerfile (1)
1-10: LGTM!notify/tests/runtime-packaging/run.sh (1)
1-207: LGTM!notify/diagnostics_capability_protocol.go (1)
1-220: LGTM!notify/diagnostics_pairing_lifecycle.go (2)
232-232: LGTM!Also applies to: 268-268, 285-285, 324-324, 817-817, 832-838
840-870: LGTM!notify/diagnostics_pairing_test.go (1)
721-787: LGTM!Also applies to: 974-977
notify/diagnostics_namespace_protocol.go (1)
635-700: LGTM!notify/diagnostics_namespace_filesystem_linux.go (1)
374-385: LGTM!Also applies to: 540-546
notify/diagnostics_namespace_installer.go (1)
4-4: LGTM!Also applies to: 36-51, 65-73
notify/diagnostics_namespace_installer_linux.go (2)
156-194: 🔒 Security & PrivacyPath-based reopen after the trusted-parent recheck.
recoverDiagnosticsNamespaceExplicitre-verifiesdiagnosticsNamespaceTrustedParentStillSelectedat Line 163, then reopens the root by joiningrequest.parentPathinto a fresh string path (Line 166-167) instead of anchoring off the already-verifiedparent *os.Roothandle. That leaves a narrow window between the check and the open where the parent directory could be swapped out from under it. This mirrors the pre-existing root-creation path's mitigation idiom (check → path-open → re-check), so it may be an accepted tradeoff — worth confirming whetheropenDiagnosticsNamespaceRootcould instead be anchored toparent(e.g.parent.OpenRoot(diagnosticsNamespaceRootName)) to remove the gap rather than only detecting it after the fact.
17-155: LGTM!Also applies to: 196-227
notify/diagnostics_namespace_linux_test.go (1)
13-15: LGTM!Also applies to: 51-63, 271-323, 680-698
notify/diagnostics_capability_protocol_test.go (1)
1-152: LGTM!notify/diagnostics_contract_model_test.go (2)
284-334: LGTM!Also applies to: 360-368
336-359: 🎯 Functional CorrectnessNo missing default-entrypoint files The referenced
notify/docker-compose.yml,notify/scripts/install.sh,notify/scripts/install.ps1,notify/scripts/bootstrap.sh, andnotify/Dockerfilefiles are present, so this test won’t fail on missing paths.> Likely an incorrect or invalid review comment.notify/diagnostics_device_id.go (1)
1-66: LGTM!notify/diagnostics_pairing_store.go (1)
69-95: LGTM!Also applies to: 116-122, 473-482, 543-545
notify/diagnostics_namespace_filesystem_unsupported.go (1)
37-40: LGTM!notify/diagnostics_runtime_config.go (1)
1-249: LGTM!notify/diagnostics_runtime_namespace.go (3)
1-204: LGTM!Also applies to: 300-393, 404-495, 553-821
521-552: 🗄️ Data Integrity & IntegrationRecovery path already requires an existing namespace root. It returns unsupported unless a root record exists for the folder, and the helper path revalidates the on-disk root manifest and current helper epoch/key; a separate
NamespaceInitialAppKeyIDcheck isn’t needed here.> Likely an incorrect or invalid review comment.
395-402: 🩺 Stability & AvailabilityDiscarded errors here are harmless.
diagnosticsNamespaceAuthorizationPathsreturns a fixed-size array, so theepoch <= 1branch can't panic, and a zero-valuediagnosticsNamespacePathis rejected byReadImmutablerather than treated as a real namespace path.> Likely an incorrect or invalid review comment.notify/diagnostics_device_id_test.go (1)
1-56: LGTM!notify/diagnostics_runtime_config_test.go (1)
1-306: LGTM!notify/diagnostics_runtime_mount.go (2)
53-64: 🔒 Security & Privacy | ⚡ Quick winTest-only bypass lives inside the production binding check.
mountBindingMatchesreturnstrueunconditionally whenevermountPathOverrides[alias]is non-empty, skipping the digest comparison entirely. The comment says this field is "unexported and cannot be loaded from config," but that's an invariant ofloadDiagnosticsRuntimeConfigelsewhere, not something this function enforces itself — any future change to the loader (or another same-package caller) that populates this map would silently disable the mount-identity check that gates access to the real vault namespace.Consider isolating the override behind a build tag (e.g. a
_test.go-only setter) or at least asserting in this function that overrides can only apply when a dedicated "test mode" flag is also set, so the bypass can't be reached by a production code path without an explicit, auditable switch.
19-38: 🔒 Security & Privacy | 🏗️ Heavy liftConfirm this namespace/mount-binding feature is an intentional exception to the "no vault structure" sidecar boundary.
Per coding guidelines,
notify/**/*.{sh,go}must keep the sidecar limited to server-side wake-up requests and "must not receive notes, file or folder names, or vault structure."diagnosticsRuntimeMountBindingtakesfolderPathdirectly, andmountBindingMatchesis invoked (per the cross-file snippet fromdiagnostics_runtime_server.go) with the livefolder.Pathfetched from Syncthing's REST API during preflight — i.e., the running notify binary does hold and hash the real vault folder path in-process, even though only the digest ever leaves the host.This is likely an intentional, disclosed scope expansion for the new opt-in diagnostics runtime rather than an oversight, but given guidelines are meant to be treated as authoritative, it's worth an explicit confirmation that this new capability is understood as exempt from that specific sidecar constraint.
Based on learnings, the coding guideline for
notify/**/*.{sh,go}states: "The optional notify sidecar must support server-side wake-up requests and must not receive notes, file or folder names, or vault structure."Also applies to: 53-71
Source: Coding guidelines
notify/diagnostics_runtime_ignore.go (1)
1-93: LGTM! Verified against Syncthing's own documentedexpandedignore-pattern semantics (gobwas/glob syntax, any-order prefix modifiers, first-match-wins) and confirmed thecalmh/globpin matches the exact fork/version Syncthing itself vendors.notify/syncthing.go (1)
8-8: LGTM! VerifiedErrUseLastResponsesemantics against Go'snet/httpdocumentation — the redirect response is returned unfollowed with a non-200 status, so it correctly surfaces as anHTTPStatusErrorrather than leaking data to a redirect target. The bounded-JSON boundary handling ingetJSONBounded(exact-limit case) is also correct.Also applies to: 39-48, 197-217, 234-280, 339-377
notify/diagnostics_runtime_integration_test.go (1)
1-1253: LGTM!notify/tests/m4-docker/Dockerfile (1)
1-11: LGTM!notify/go.mod (1)
4-8: LGTM! Confirmed via web search that this is the exactcalmh/globfork/version Syncthing itself vendors, so pinning it here gives byte-for-byte parity with Syncthing's own ignore-pattern grammar.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Move lifecycle and namespace preparation outside global mutexes while binding each commit to the exact re-read credential and root state. Anchor namespace opens to the verified parent, require mount digests even in test overrides, and extend dormancy and tuple regression coverage.
Require an exact active pairing authorization before allocating the per-folder namespace lock, then re-read it after lock acquisition before any slow work. Add a regression proving a validly signed but unpaired folder tuple leaves the lock map empty.
1 participant
Summary
Safety and compatibility
f41f597d3ceca73da102e5e447382dfae07d2e08.Verification
go test ./... -count=1go test -race ./... -count=1go vet ./...go mod verifygovulncheck ./...— no vulnerabilities found7faae6481492a99b434c47eb7bcd480dc22c2255: old → new → old → new packaging proof with byte-identical credential state; new local image indexsha256:6ae51aae1080c537fcd753f6597bf90cc394186917863115ab8fc79312194e91Existing-user, migration, and rollback
No migration runs for existing users. Opt-in state is separate from Syncthing configuration and the synchronized namespace. Downgrading to the old helper makes diagnostics unavailable without rewriting the preserved state; re-upgrading the same reviewed image revalidates the same identity and SPKI pin. Backups, versions, conflicts, remote history, and tombstones can retain synchronized artifacts and are never represented as cleanup success.
Not in this PR
This PR does not publish or deploy a helper, add app runtime calls, establish real-device upload/download/roundtrip evidence, change Relay v1, or make VaultSync 2.0 release-ready.