fix: harden clip server bind defaults - #21
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughClipboard serving now defaults to ChangesClipboard networking
Merge Risk: 🟡 Moderate · up to This change improves clipboard binding defaults, but unresolved URL and listen-address normalization cases can leave clipboard sharing misconfigured or unavailable. Resolve these configuration behaviors before merging. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Team
Run ID: e002e3d9-3c71-4052-ab04-2377d6b8b38a
📒 Files selected for processing (10)
README.mdcmd/mole/clip.gocmd/mole/clip_test.gocmd/mole/init.gocmd/mole/init_test.goexamples/mole.yamlinternal/clip/client.gointernal/clip/server.gointernal/config/config.gointernal/config/config_test.go
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
Fixes Applied Successfully\n\nApplied the three approved CodeRabbit findings.\n\nChanges:\n- warnings now use the effective listener address, including empty broad binds\n- mole init --help documents clipboard flags and supported environment variables\n- malformed clip_url values are rejected with a useful error\n\nFiles modified:\n- cmd/mole/clip.go\n- cmd/mole/clip_test.go\n- cmd/mole/init.go\n- cmd/mole/init_test.go\n- internal/config/config.go\n- internal/config/config_test.go\n\nCommit: f9680a3\n\nValidation passed: 159 tests, race tests, go vet, and diff checks. |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Team
Run ID: 019b2e25-0d1b-406e-9d3a-0cde32da287d
📒 Files selected for processing (6)
cmd/mole/clip.gocmd/mole/clip_test.gocmd/mole/init.gocmd/mole/init_test.gointernal/config/config.gointernal/config/config_test.go
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
Fixes Applied Successfully\n\nApplied the two approved CodeRabbit findings.\n\nChanges:\n- clip_url is validated even when clip_listen is explicitly configured\n- clip URL validation errors are sanitized and do not expose URL credentials\n\nCommit: a6883bc\n\nValidation passed: 159 tests, race tests, go vet, format checks, and diff checks. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/mole/init.go (1)
257-257: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrim
ClipListenbefore applying the default.When
MOLE_CLIP_LISTENcontains only whitespace, line 255 classifies it as not provided, but line 256 does not treat it as empty. Line 257 therefore does not applyconfig.DefaultClipListen;ans.ClipListenlater becomes empty.Normalize
in.ClipListenbefore this branch, or reject whitespace-only values.As per path instructions, “The command and flag definitions are user-facing API.”
Source: Path instructions
internal/config/config.go (1)
231-234: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject invalid clipboard port ranges in both
clipListenForURLhelpers.When the URL-derived listen address is used, port
0makesnet.Listenselect an ephemeral port, while a port above65535fails only whenrunClipServestarts. Validate the parsed port as1..65535before callingnet.JoinHostPortininternal/config/config.goandcmd/mole/init.go.Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Team
Run ID: c3fd1a6d-4221-4d02-b775-41d9eef9ed9a
📒 Files selected for processing (4)
cmd/mole/init.gocmd/mole/init_test.gointernal/config/config.gointernal/config/config_test.go
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.
| if strings.TrimSpace(clipConfig.ClipURL) != "" { | ||
| listen, err := clipListenForURL(clipConfig.ClipURL) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Store the normalized ClipURL.
strings.TrimSpace is applied only to the validation condition and the helper’s local copy. cfg.ClipURL keeps the original YAML value. A quoted URL with surrounding whitespace therefore derives ClipListen from one value while mole clip pull consumes another. A whitespace-only value also remains non-empty and bypasses validation.
Assign the trimmed value to cfg.ClipURL, or reject whitespace-only values.
As per path instructions, “Malformed or unknown configuration should fail with a useful error.”
Source: Path instructions
| if parsed.Hostname() == "" { | ||
| return "", errors.New("invalid clip URL: missing host") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🤖 get_repo_knowledge executed:
get_repo_knowledge Luqueee/mole /tmp/coderabbit-repo-knowledge/luqueee-mole-a4a89a3b/conventions
Length of output: 3058
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- internal/config/config.go ---'
sed -n '190,255p' internal/config/config.go
printf '%s\n' '--- cmd/mole/init.go ---'
sed -n '455,525p' cmd/mole/init.go
printf '%s\n' '--- ClipURL definitions and uses ---'
rg -n -C 3 'ClipURL|clip URL|parsed\.Hostname|ParseRequestURI|url\.Parse' --glob '*.go' .
printf '%s\n' '--- HTTP client/server bindings ---'
rg -n -C 3 'http\.(Get|Post|Client|NewRequest|ListenAndServe|Serve)|ListenAndServe|RoundTripper|Transport' --glob '*.go' .Repository: Luqueee/mole
Length of output: 37625
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '210,242p' internal/config/config.go
sed -n '480,512p' cmd/mole/init.go
rg -n -C 4 'ClipURL|parsed\.Hostname|url\.Parse|ParseRequestURI|http\.(Get|Post|NewRequest)|ListenAndServe' --glob '*.go' .Repository: Luqueee/mole
Length of output: 35910
🤖 get_repo_knowledge executed:
get_repo_knowledge Luqueee/mole /tmp/coderabbit-repo-knowledge/luqueee-mole-a4a89a3b
Length of output: 1904
🌐 Web query:
Go net/http transport supported URL schemes http https RoundTrip documentation
💡 Result:
In the Go net/http package, the http.Transport type is an implementation of the RoundTripper interface that natively supports http and https URL schemes [1][2]. Beyond these native schemes, Transport allows for the registration of custom protocol handlers using the RegisterProtocol method [3][4]. Key details regarding supported schemes and protocols: 1. Native Support: Transport natively handles http and https requests [1][2]. By default, it manages HTTP/1.1 for these schemes and may negotiate HTTP/2 or HTTP/3 depending on configuration [1][5][6]. 2. Custom Protocols: You can extend Transport to support additional schemes (e.g., ftp://, custom RPC protocols) by using the RegisterProtocol(scheme string, rt RoundTripper) method [3][4]. This allows you to define a custom RoundTripper implementation that will be invoked whenever a request with the registered scheme is encountered [4]. 3. Proxy Schemes: When configuring a proxy, Transport explicitly supports the schemes http, https, socks5, and socks5h [1][7]. The proxy type is determined by the scheme provided in the proxy URL [1][7]. 4. HTTP/2 and HTTP/3: While often treated as transport layers for http/https, Go also provides internal mechanisms to register handlers for "http/2" and "http/3" schemes explicitly in some contexts, typically used for advanced transport configuration and testing [7][4]. For precise control over which versions of HTTP are used for native schemes, you can configure the Protocols field on the Transport (e.g., using HTTP1, HTTP2, and UnencryptedHTTP2 constants) [5][8].
Citations:
- 1: https://go.googlesource.com/go/+/master/src/net/http/transport.go
- 2: https://github.com/golang/go/blob/go1.16.3/src/net/http/transport.go
- 3: https://pkg.go.dev/net/http
- 4: https://github.com/golang/go/blob/master/src/net/http/transport.go
- 5: https://pkg.go.dev/net/http@go1.26.7
- 6: https://tip.golang.org/src/net/http/doc.go
- 7: https://tip.golang.org/src/net/http/transport.go?m=text
- 8: https://tip.golang.org/src/net/http/http.go
🌐 Web query:
github Luqueee/mole internal/config config.go ClipURL cmd/mole/init.go
💡 Result:
The repository github.com/Luqueee/mole is an SSH port forwarder tool designed for developer workflows [1]. It features automatic port discovery on remote hosts, auto-reconnection, and integration with SSH config aliases [1]. Regarding the specific file paths and components you mentioned: - internal/config/config.go: This path is standard in Go projects for managing application configuration [2][3]. While specific internal code for Luqueee/mole was not surfaced in external documentation, projects of this type typically use libraries like Viper to handle configuration loading from files, environment variables, or CLI flags [4][3]. ClipURL is likely a configuration parameter or utility function within this context, potentially related to handling clipboard interactions or URL parsing for SSH connections. - cmd/mole/init.go: This file is typically used in Go CLI applications to initialize the application, set up flags, or perform bootstrap logic before the main command execution [1]. It is important to distinguish this project (Luqueee/mole) from other similarly named tools: - There is a well-known, unrelated tool called mole (github.com/davrodpin/mole) used for creating resilient SSH tunnels [5]. - There is also a macOS utility named Mole (mole.fit) used for system cleanup [6][7]. You can explore the source code directly at https://github.com/Luqueee/mole to see the exact implementation details of the config handling and initialization logic [1].
Citations:
- 1: https://github.com/Luqueee/mole
- 2: https://pkg.go.dev/github.com/jacoelho/rvw/internal/config
- 3: https://dev.to/techschoolguru/load-config-from-file-environment-variables-in-golang-with-viper-2j2d
- 4: https://github.com/spf13/viper
- 5: https://github.com/davrodpin/mole
- 6: https://formulae.brew.sh/formula/mole
- 7: https://mole.fit/docs
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- cmd/mole/clip.go ---'
sed -n '205,245p' cmd/mole/clip.go
printf '%s\n' '--- internal/clip/client.go ---'
sed -n '20,65p' internal/clip/client.go
printf '%s\n' '--- clip serve listen path ---'
sed -n '105,150p' cmd/mole/clip.goRepository: Luqueee/mole
Length of output: 4316
Reject non-HTTP(S) ClipURL values in both clipListenForURL helpers.
ftp://host:7777 passes both helpers because url.Parse succeeds and parsed.Hostname() is non-empty. The value reaches internal/clip.Client, whose default net/http transport rejects it during Pull. Validate parsed.Scheme as http or https before deriving the listen address.
📍 Affects 2 files
internal/config/config.go#L228-L229(this comment)cmd/mole/init.go#L498-L499
Source: Path instructions
Fixes Applied Successfully\n\nApplied the two approved CodeRabbit findings.\n\nChanges:\n- whitespace-only clipboard listen values now fall back correctly\n- URL-derived clipboard ports must be between 1 and 65535\n\nCommit: e4e7ba5\n\nValidation passed: 163 tests, race tests, go vet, format checks, and diff checks. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary\n- bind the unauthenticated clip server to loopback by default and warn on broad binds\n- make mole init derive a coherent private listen address from clip_url, while preserving explicit flags, environment values, and existing YAML settings\n- document Tailscale/WireGuard setup and add regression coverage\n\n## Validation\n- go test ./...\n- go test -race ./...\n- go vet ./...\n- git diff --check\n- CodeRabbit review: no findings\n\nFixes #17