fix(security): add proxy-aware transport with MITM warning and opt-out#247
fix(security): add proxy-aware transport with MITM warning and opt-out#247maochengwei1024-create merged 1 commit intomainfrom
Conversation
|
|
github-actions
bot
added
the
size/L
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds a proxy utility and tests, and updates several HTTP transport initializations to use the new util base/fallback transports and emit a one-time proxy warning where clients are constructed. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Client
participant Factory as client factory
participant Util as util package
participant Env as Proxy Env
participant Net as Network
CLI->>Factory: request HTTP client
Factory->>Util: WarnIfProxied(os.Stderr)
Util->>Env: DetectProxyEnv()
Env-->>Util: proxy key/value (maybe empty)
alt proxy detected and not disabled
Util-->>Factory: prints one-time warning (redacted)
end
Factory->>Util: NewBaseTransport() / FallbackTransport()
Util-->>Factory: *http.Transport (proxy configured or disabled)
CLI->>Net: HTTP request using returned Transport
Net-->>CLI: HTTP response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR introduces proxy-aware HTTP transport across the CLI by replacing all Key changes:
Confidence Score: 5/5Safe to merge; all previous P1 concerns are resolved and remaining findings are test-robustness suggestions only. All three previously flagged P1/P2 issues (connection-pool fragmentation, missing WarnIfProxied in the Lark SDK path, os.Setenv without cleanup) have been addressed. The only remaining findings are P2 test-robustness gaps: three warn-tests that assert a warning IS emitted do not clear LARK_CLI_NO_PROXY first, so they could produce vacuously passing or spuriously failing results in CI environments where that var is pre-set. These do not affect production behaviour and do not block merge. internal/util/proxy_test.go — three test functions (TestWarnIfProxied_WithProxy, TestWarnIfProxied_OnlyOnce, TestWarnIfProxied_RedactsCredentials) should clear LARK_CLI_NO_PROXY via t.Setenv before asserting warning output. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[HTTP Request] --> B{cachedHttpClientFunc\nor cachedLarkClientFunc}
B --> C[util.WarnIfProxied\nsync.Once guard]
C --> D{LARK_CLI_NO_PROXY set?}
D -- yes --> E[Once consumed, silent]
D -- no --> F{Proxy env var detected?}
F -- no --> G[Once consumed, silent]
F -- yes --> H[Emit redacted warning to stderr]
H --> I[util.NewBaseTransport\nclone of DefaultTransport]
E --> I
G --> I
I --> J{LARK_CLI_NO_PROXY set?}
J -- yes --> K[t.Proxy = nil / no proxy]
J -- no --> L[t.Proxy = ProxyFromEnvironment]
K --> M[Transport middleware chain]
L --> M
M --> N[Outbound HTTP request]
subgraph Fallback path
O[Transport decorator Base == nil] --> P[util.FallbackTransport singleton]
P --> M
end
Reviews (2): Last reviewed commit: "fix(security): replace http.DefaultTrans..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@892420862f097584a483ade985e9296902dce51e🧩 Skill updatenpx skills add larksuite/cli#fix/proxy-mitm-hardening -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: 2
🧹 Nitpick comments (1)
internal/cmdutil/transport.go (1)
21-25: Cache the nil-Basefallback transport instead of cloning per call.
util.NewBaseTransport()returns a fresh clone ofhttp.DefaultTransport. In the nil-Basepath, this means a new connection pool on every request, andRetryTransportcreates a different transport for each retry attempt (lines 37 and 53). Although all current instantiations infactory_default.go(lines 78–90, 103–110) and tests explicitly setBase, this fallback pattern creates an inefficiency footgun if any caller omits it. Cache one fallback transport and reuse it across all calls, applying the same treatment to:
RetryTransport.base()(lines 21–25)UserAgentTransport.RoundTrip()(lines 64–70)SecurityHeaderTransport.base()(lines 79–83)SecurityPolicyTransport.base()ininternal/auth/transport.go(lines 24–29)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmdutil/transport.go` around lines 21 - 25, The code currently calls util.NewBaseTransport() each time a transport fallback is needed, which creates a new connection pool per call; fix this by introducing a package-level cached base transport (initialized once, e.g., via sync.Once or in init()) and return that cached transport whenever t.Base is nil. Update RetryTransport.base(), UserAgentTransport.RoundTrip() (nil-Base path), SecurityHeaderTransport.base(), and SecurityPolicyTransport.base() to use the shared cachedBaseTransport instead of calling util.NewBaseTransport() each time so all retries and requests reuse the same connection pool.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cmdutil/factory_default.go`:
- Around line 77-79: The proxy warning is only emitted in cachedHttpClientFunc,
so cachedLarkClientFunc (used by f.LarkClient()) can build its own HTTP stack
without calling util.WarnIfProxied; add a call to util.WarnIfProxied(os.Stderr)
inside the cachedLarkClientFunc initialization (or move the single-call into a
shared initialization used by both closures) so the MITM warning is emitted for
the SDK client path as well; locate cachedLarkClientFunc and
cachedHttpClientFunc and ensure util.WarnIfProxied is invoked before creating
the transport (util.NewBaseTransport()) in the Lark client path.
In `@internal/util/proxy_test.go`:
- Around line 175-190: In TestNewBaseTransport_RespectsNoProxyEnv replace the
direct call to os.Setenv(EnvNoProxy, "") with t.Setenv(EnvNoProxy, "") so the
test uses testing.T's environment cleanup stack; locate the call in the
TestNewBaseTransport_RespectsNoProxyEnv function (which calls NewBaseTransport)
and update it to t.Setenv to avoid leaking LARK_CLI_NO_PROXY across tests.
---
Nitpick comments:
In `@internal/cmdutil/transport.go`:
- Around line 21-25: The code currently calls util.NewBaseTransport() each time
a transport fallback is needed, which creates a new connection pool per call;
fix this by introducing a package-level cached base transport (initialized once,
e.g., via sync.Once or in init()) and return that cached transport whenever
t.Base is nil. Update RetryTransport.base(), UserAgentTransport.RoundTrip()
(nil-Base path), SecurityHeaderTransport.base(), and
SecurityPolicyTransport.base() to use the shared cachedBaseTransport instead of
calling util.NewBaseTransport() each time so all retries and requests reuse the
same connection pool.
🪄 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: 31d0fe64-bf87-4563-9583-55b0bb680700
📒 Files selected for processing (6)
internal/auth/transport.gointernal/cmdutil/factory_default.gointernal/cmdutil/transport.gointernal/update/update.gointernal/util/proxy.gointernal/util/proxy_test.go
…ansport to mitigate MITM risk All HTTP clients previously used http.DefaultTransport which silently respects HTTP_PROXY/HTTPS_PROXY env vars, allowing credentials to transit through untrusted proxies. This adds a proxy detection warning and an opt-out switch (LARK_CLI_NO_PROXY=1) so security-sensitive users can disable proxy entirely. - Redact proxy credentials in warning output (handles both scheme-prefixed and bare URL formats) - Suppress warning when LARK_CLI_NO_PROXY is already set - Use FallbackTransport singleton for nil-Base fallback paths to preserve connection pooling - Emit proxy warning on both HTTP client and Lark SDK client paths Change-Id: Ibed7d0470409c73fbd42bccac6673f9fc5e87a83
maochengwei1024-create
force-pushed
the
fix/proxy-mitm-hardening
branch
from
a752404 to
8924208
Compare
3 participants
Summary
http.DefaultTransportusage withutil.NewBaseTransport(), which clones DefaultTransport and supportsLARK_CLI_NO_PROXY=1to disable proxy entirelyLARK_CLI_NO_PROXYTest plan
go build ./...passesinternal/cmdutil,internal/auth,internal/update)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores