feat: add proxy plugin mode for CLI HTTP transport#1181
Open
JackZhao10086 wants to merge 4 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a proxy-plugin subsystem: config model and cached loader (file + env with env precedence), strict loopback-proxy validation and credential redaction, optional extra CA injection into TLS, lazy fail-closed transport construction, integration into util SharedTransport(), and deterministic tests. ChangesProxy Plugin Configuration and Transport Integration
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
the
size/L
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1181 +/- ##
==========================================
+ Coverage 68.92% 68.94% +0.01%
==========================================
Files 629 632 +3
Lines 58762 58936 +174
==========================================
+ Hits 40503 40631 +128
- Misses 14952 14979 +27
- Partials 3307 3326 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@6d161b025358870016b7d1422342eaa12121cf10🧩 Skill updatenpx skills add larksuite/cli#feat/sec_mode -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@internal/secplugin/config.go`:
- Around line 196-198: The URL path check currently allows a trailing slash
(u.Path == "/"); update the validation to reject any path, query, or fragment
components. Change the condition that uses u.Path (and the error using
envvars.CliProxyAddress and redacted) so it returns an error when u.Path != ""
(not just when it's not "/"), and also add explicit checks to error when
u.RawQuery != "" or u.Fragment != "" to ensure no path/query/fragment are
allowed.
In `@internal/secplugin/README.md`:
- Line 76: The README currently states that LARKSUITE_CLI_PROXY_ADDRESS "must
not contain a path" but the validation also rejects query and fragment
components; update the documentation for the LARKSUITE_CLI_PROXY_ADDRESS
environment variable to explicitly list all rejected URL components (path,
query, and fragment) so it matches the actual validation behavior (refer to the
LARKSUITE_CLI_PROXY_ADDRESS symbol in the README and any related
validateProxyAddress or proxy address validation logic to ensure wording is
consistent).
In `@internal/secplugin/README.zh-CN.md`:
- Line 72: Update the README entry for LARKSUITE_CLI_PROXY_ADDRESS to list all
rejected URL components: explicitly state it must be a host[:port] (no path, no
query string, no fragment) and clarify any allowed schemes or default behavior;
reference the README line mentioning `LARKSUITE_CLI_PROXY_ADDRESS` and replace
the single "不能带路径" note with a concise bullet or sentence enumerating
"不能带路径、查询参数（query）及片段（fragment）" so the documentation matches the validation
behavior.
- Line 66: Remove the extra space in the README sentence so "设置了 代理相关环境变量"
becomes "设置了代理相关环境变量" in internal/secplugin/README.zh-CN.md; locate the line
containing the phrase "如果没有配置文件，但设置了 代理相关环境变量，也可以正常工作。" and delete the space
between "设置了" and "代理相关环境变量".
In `@internal/secplugin/tls_ca.go`:
- Around line 42-49: The code unconditionally sets t.TLSClientConfig.MinVersion
= tls.VersionTLS12 which can downgrade a cloned higher minimum (e.g.,
tls.VersionTLS13); update the logic in the TLSClientConfig setup (around
t.TLSClientConfig, Clone(), and MinVersion) to only set MinVersion when it is
zero or less than tls.VersionTLS12 (i.e., raise the floor to TLS1.2 but never
lower an existing higher MinVersion).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ec8f6af-8a51-4d07-af51-4b2668a6de0c
📒 Files selected for processing (9)
internal/envvars/envvars.gointernal/secplugin/README.mdinternal/secplugin/README.zh-CN.mdinternal/secplugin/config.gointernal/secplugin/config_test.gointernal/secplugin/tls_ca.gointernal/secplugin/tls_ca_test.gointernal/util/proxy.gointernal/util/proxy_test.go
|
|
||
| - `LARKSUITE_CLI_PROXY_ADDRESS` must use the `http` scheme only. | ||
| - The host of `LARKSUITE_CLI_PROXY_ADDRESS` must be `127.0.0.1`. | ||
| - `LARKSUITE_CLI_PROXY_ADDRESS` must not contain a path. |
There was a problem hiding this comment.
Document all rejected URL components.
The constraint states the proxy address "must not contain a path," but based on the PR objectives, the validation also rejects query and fragment components.
📝 Proposed fix to document all constraints
-- `LARKSUITE_CLI_PROXY_ADDRESS` must not contain a path.
+- `LARKSUITE_CLI_PROXY_ADDRESS` must not contain a path, query, or fragment.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - `LARKSUITE_CLI_PROXY_ADDRESS` must not contain a path. | |
| - `LARKSUITE_CLI_PROXY_ADDRESS` must not contain a path, query, or fragment. |
🤖 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 `@internal/secplugin/README.md` at line 76, The README currently states that
LARKSUITE_CLI_PROXY_ADDRESS "must not contain a path" but the validation also
rejects query and fragment components; update the documentation for the
LARKSUITE_CLI_PROXY_ADDRESS environment variable to explicitly list all rejected
URL components (path, query, and fragment) so it matches the actual validation
behavior (refer to the LARKSUITE_CLI_PROXY_ADDRESS symbol in the README and any
related validateProxyAddress or proxy address validation logic to ensure wording
is consistent).
|
|
||
| - `LARKSUITE_CLI_PROXY_ADDRESS` 只允许 `http` 协议。 | ||
| - `LARKSUITE_CLI_PROXY_ADDRESS` 的 host 必须是 `127.0.0.1`。 | ||
| - `LARKSUITE_CLI_PROXY_ADDRESS` 不能带路径。 |
There was a problem hiding this comment.
Document all rejected URL components.
The constraint states "不能带路径" (must not contain a path), but the validation also rejects query and fragment components per the PR objectives.
📝 Proposed fix to document all constraints
-- `LARKSUITE_CLI_PROXY_ADDRESS` 不能带路径。
+- `LARKSUITE_CLI_PROXY_ADDRESS` 不能带路径、查询参数或片段标识符。📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - `LARKSUITE_CLI_PROXY_ADDRESS` 不能带路径。 | |
| - `LARKSUITE_CLI_PROXY_ADDRESS` 不能带路径、查询参数或片段标识符。 |
🤖 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 `@internal/secplugin/README.zh-CN.md` at line 72, Update the README entry for
LARKSUITE_CLI_PROXY_ADDRESS to list all rejected URL components: explicitly
state it must be a host[:port] (no path, no query string, no fragment) and
clarify any allowed schemes or default behavior; reference the README line
mentioning `LARKSUITE_CLI_PROXY_ADDRESS` and replace the single "不能带路径" note
with a concise bullet or sentence enumerating "不能带路径、查询参数（query）及片段（fragment）"
so the documentation matches the validation behavior.
JackZhao10086
force-pushed
the
feat/sec_mode
branch
from
b073963 to
e14bf47
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/proxyplugin/config.go (1)
196-198:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject trailing slash in the proxy URL.
The current check allows
http://127.0.0.1:3128/, which includes a path component. The documented contract is to reject any path/query/fragment. Tighten the validation to align with the fail-closed design.🔧 Proposed fix
- if u.Path != "" && u.Path != "/" { + if u.Path != "" { return nil, fmt.Errorf("invalid %s %q: path is not allowed", envvars.CliProxyAddress, redacted) }🤖 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 `@internal/proxyplugin/config.go` around lines 196 - 198, The URL validation currently only rejects non-empty paths except "/" but must reject any path, query, or fragment; update the check in internal/proxyplugin/config.go to return an error when u.Path != "" || u.RawQuery != "" || u.Fragment != "" (preserving the existing error message that uses envvars.CliProxyAddress and redacted) so any URL with a path, query, or fragment is rejected per the documented contract.
🧹 Nitpick comments (1)
internal/proxyplugin/config_test.go (1)
53-58: 💤 Low valueConsolidate the repeated cache-reset boilerplate.
The three-line reset of
loadOnce/loadCfg/loadErris duplicated across all five tests here. AresetProxyPluginState()helper already exists intransport_test.gowithin this same package and resets exactly these globals (plusproxyPluginTransport, which is harmless to reset here). Reusing it removes the duplication.♻️ Example for one test; apply across the others
func TestLoad_MissingFileReturnsNil(t *testing.T) { t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) - loadOnce = sync.Once{} - loadCfg = nil - loadErr = nil - unsetProxyPluginEnv(t) + unsetProxyPluginEnv(t) + resetProxyPluginState()Note:
syncwould no longer be needed in this file after the change.🤖 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 `@internal/proxyplugin/config_test.go` around lines 53 - 58, Replace the duplicated three-line cache reset in each test (resetting loadOnce, loadCfg, loadErr) with a call to the existing helper resetProxyPluginState() (found in transport_test.go) and keep the per-test unsetProxyPluginEnv(t) calls; update tests like TestLoad_MissingFileReturnsNil to call resetProxyPluginState() instead of manually reassigning loadOnce/loadCfg/loadErr, and remove the now-unneeded sync import from this file.
🤖 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 `@internal/proxyplugin/config.go`:
- Line 236: applyExtraRootCA currently accepts CAPath from an untrusted env var
and calls vfs.ReadFile(caPath) after trimming/IsAbs checks, allowing arbitrary
absolute-file reads; update the function (in internal/proxyplugin/tls_ca.go) to
validate the incoming caPath with validate.SafeInputPath (or an equivalent
safe-path validator for env-provided absolute paths) before calling
vfs.ReadFile, returning an error if validation fails and only proceeding to read
and parse the CA when the path is deemed safe.
In `@internal/proxyplugin/transport.go`:
- Around line 38-53: The fail-closed branches in SharedTransport and
buildProxyPluginTransport currently return http.DefaultTransport or an empty
&http.Transport{} when http.DefaultTransport is not a *http.Transport, which
allows direct egress; instead create and return a type-agnostic erroring
RoundTripper (e.g. blockedRoundTripper with an err field and RoundTrip that
returns nil, err) and use that in the !ok branch in SharedTransport (where
Load() fails) and the corresponding guard in buildProxyPluginTransport so both
functions always return a failing RoundTripper (call blockedTransport or replace
its use with blockedRoundTripper instances carrying a descriptive error) to
preserve the fail-closed contract regardless of the concrete
http.DefaultTransport type.
---
Duplicate comments:
In `@internal/proxyplugin/config.go`:
- Around line 196-198: The URL validation currently only rejects non-empty paths
except "/" but must reject any path, query, or fragment; update the check in
internal/proxyplugin/config.go to return an error when u.Path != "" ||
u.RawQuery != "" || u.Fragment != "" (preserving the existing error message that
uses envvars.CliProxyAddress and redacted) so any URL with a path, query, or
fragment is rejected per the documented contract.
---
Nitpick comments:
In `@internal/proxyplugin/config_test.go`:
- Around line 53-58: Replace the duplicated three-line cache reset in each test
(resetting loadOnce, loadCfg, loadErr) with a call to the existing helper
resetProxyPluginState() (found in transport_test.go) and keep the per-test
unsetProxyPluginEnv(t) calls; update tests like TestLoad_MissingFileReturnsNil
to call resetProxyPluginState() instead of manually reassigning
loadOnce/loadCfg/loadErr, and remove the now-unneeded sync import from this
file.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: de25f8b7-2579-4ed0-827e-4e38dce62d04
📒 Files selected for processing (11)
internal/envvars/envvars.gointernal/proxyplugin/README.mdinternal/proxyplugin/README.zh-CN.mdinternal/proxyplugin/config.gointernal/proxyplugin/config_test.gointernal/proxyplugin/tls_ca.gointernal/proxyplugin/tls_ca_test.gointernal/proxyplugin/transport.gointernal/proxyplugin/transport_test.gointernal/util/proxy.gointernal/util/proxy_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/proxyplugin/README.md
- internal/proxyplugin/README.zh-CN.md
- internal/envvars/envvars.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/util/proxy_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add a proxy plugin mode for CLI HTTP(S) traffic, allowing requests to be forced through a local HTTP proxy with optional extra CA trust for proxy TLS interception. This also standardizes the public config names by removing
SECwording from environment variables and renaming the config file toproxy_config.json.Changes
internal/secpluginconfig loading for~/.lark-cli/proxy_config.json/$LARKSUITE_CLI_CONFIG_DIR/proxy_config.json, with environment variable overrides.LARKSUITE_CLI_PROXY_ENABLE,LARKSUITE_CLI_PROXY_ADDRESS, andLARKSUITE_CLI_CA_PATH.http://127.0.0.1:<port>when enabled.Test Plan
go test ./internal/envvars ./internal/secplugin ./internal/utilgofmt -l internal/envvars/envvars.go internal/secplugin/config.go internal/secplugin/tls_ca.go internal/secplugin/config_test.go internal/secplugin/tls_ca_test.go internal/util/proxy.go internal/util/proxy_test.goLARKSUITE_CLI_SEC_*,CliSec*,sec_config.jsonlark-cli <domain> <command>flow works as expectedRelated Issues
Summary by CodeRabbit