feat: add proxy plugin mode for CLI HTTP transport#1181
Merged
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:
📝 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, util integration (SharedTransport/NewHTTPClient), 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% 69.19% +0.26%
==========================================
Files 629 633 +4
Lines 58762 59582 +820
==========================================
+ Hits 40503 41229 +726
- Misses 14952 15025 +73
- Partials 3307 3328 +21 ☔ 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@7213eeea4a09648a9fe0380a90758bee1907b70e🧩 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
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
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/proxyplugin/config_test.go (1)
271-276: 💤 Low valueConsider backfilling earlier tests to use
resetLoadState().The new helper consolidates the three-line reset pattern (
loadOnce = sync.Once{},loadCfg = nil,loadErr = nil) that earlier tests (lines 56-58, 75-77, 115-117, etc.) duplicate inline. Updating them to useresetLoadState()would improve consistency and reduce maintenance surface.🤖 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 271 - 276, Several earlier tests duplicate the three-line cache reset pattern; replace those inline resets (the sequence `loadOnce = sync.Once{}`, `loadCfg = nil`, `loadErr = nil`) with a call to the new helper resetLoadState() to consolidate behavior. Search test files for occurrences of that exact three-line pattern (examples near the tests that currently set loadOnce/loadCfg/loadErr) and replace each block with a single call to resetLoadState(); ensure tests still import the same package scope so resetLoadState() is callable from those test functions.
🤖 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 `@internal/proxyplugin/config_test.go`:
- Around line 271-276: Several earlier tests duplicate the three-line cache
reset pattern; replace those inline resets (the sequence `loadOnce =
sync.Once{}`, `loadCfg = nil`, `loadErr = nil`) with a call to the new helper
resetLoadState() to consolidate behavior. Search test files for occurrences of
that exact three-line pattern (examples near the tests that currently set
loadOnce/loadCfg/loadErr) and replace each block with a single call to
resetLoadState(); ensure tests still import the same package scope so
resetLoadState() is callable from those test functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a74602ff-809f-4d93-8953-7bfea52acc1b
📒 Files selected for processing (10)
cmd/config/init_interactive.gocmd/doctor/doctor.gointernal/proxyplugin/config.gointernal/proxyplugin/config_test.gointernal/proxyplugin/tls_ca.gointernal/proxyplugin/transport.gointernal/proxyplugin/transport_test.gointernal/registry/remote.gointernal/util/proxy.gointernal/util/proxy_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/proxyplugin/tls_ca.go
- internal/proxyplugin/transport_test.go
- internal/proxyplugin/config.go
liangshuo-1
approved these changes
Jun 2, 2026
This was referenced Jun 2, 2026
liangshuo-1
added a commit
that referenced
this pull request
Jun 2, 2026
Resolve internal/registry/remote.go import conflict by keeping BOTH sides: the typed meta model (this branch) and the proxy-aware transport client (main #1181/#1213). The changes are orthogonal — fetchRemoteMerged now uses transport.NewHTTPClient(fetchTimeout) over the typed MergedRegistry. Only the import block conflicted; the transport client line auto-merged. Change-Id: Ia61ab32c39e5b4eadb05e7efe46ff0c8dcfce568
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
3 participants
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
New Features
Bug Fixes / UX