refactor: centralize HTTP transport policies#2021
Conversation
github-actions
Bot
added
domain/ccm
|
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 introduces platform/external HTTP request classification, policy-routed clients, scoped extension middleware, SDK bootstrap bridging, stricter redirect handling, fail-closed transport cloning, external-client adoption across download paths, and preservation of typed transport errors. ChangesProvider-aware transport routing
Estimated code review effort: 5 (Critical) | ~100 minutes Sequence Diagram(s)sequenceDiagram
participant Command
participant Factory
participant HTTPPolicyRouter
participant ExtensionMiddleware
participant ExternalServer
Command->>Factory: Request ExternalHTTPClient
Factory->>HTTPPolicyRouter: Force external RequestClass
HTTPPolicyRouter->>ExtensionMiddleware: Apply eligible middleware
ExtensionMiddleware->>ExternalServer: Send external request
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@06d5b18d37158db0cad7bd30bcae39789a4ee279🧩 Skill updatenpx skills add larksuite/cli#refactor/http-transport-policy -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2021 +/- ##
==========================================
+ Coverage 75.14% 75.19% +0.04%
==========================================
Files 911 914 +3
Lines 96322 96905 +583
==========================================
+ Hits 72385 72863 +478
- Misses 18370 18410 +40
- Partials 5567 5632 +65 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/validate/url.go (2)
1-1: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftExtract a shared "hardened transport leaf" helper. Both sites independently implement the same clone-and-guard pattern (wrap
DialContext/DialTLSContext/DialTLSwith remote-IP validation, fail closed when safe cloning isn't possible), which has already caused the two implementations to diverge on error typing.
internal/validate/url.go#L195-271: extractnewDownloadTransportLeaf/configureDirectDownloadTransport's dial-wrapping/guard logic into a shared helper (e.g., ininternal/transportorinternal/validate) parameterized by the remote-IP predicate and proxy-handling policy (freeze-selected-proxy vs. disable-proxy).shortcuts/doc/doc_resource_cover.go#L727-801: adopt the same shared helper forcloneDocCoverTransport/newDocCoverTransportLeafinstead of re-implementing the dial/guard wiring, keeping its existing "always disable proxy" policy.🤖 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/validate/url.go` at line 1, Extract the duplicated transport clone-and-guard wiring from newDownloadTransportLeaf/configureDirectDownloadTransport and cloneDocCoverTransport/newDocCoverTransportLeaf into one shared helper. Parameterize it with the remote-IP validation predicate and proxy policy, preserving frozen selected-proxy behavior for downloads and always-disabled proxy behavior for document covers; ensure cloning failures fail closed and all DialContext/DialTLSContext/DialTLS wrappers use consistent error handling.
195-271: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftDuplicated fail-closed transport-cloning/IP-guard logic vs.
shortcuts/doc/doc_resource_cover.go.
newDownloadTransportLeaf/configureDirectDownloadTransportare structurally near-identical tonewDocCoverTransportLeafinshortcuts/doc/doc_resource_cover.go(dial wrapping, DialTLSContext/DialTLS guards, remote-IP validation). See consolidated comment below.🤖 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/validate/url.go` around lines 195 - 271, The direct transport cloning and dial/IP-validation logic in newDownloadTransportLeaf and configureDirectDownloadTransport duplicates newDocCoverTransportLeaf; consolidate these paths through the existing shared transport-wrapper helper or extract their common implementation into one reusable symbol. Preserve fail-closed handling, DialContext/DialTLSContext/DialTLS coverage, connection closing on validation failure, and the distinct proxy behavior.shortcuts/doc/doc_resource_cover.go (1)
727-801: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftDuplicated fail-closed transport-cloning/IP-guard logic vs.
internal/validate/url.go.
blockedDocCoverTransport/cloneDocCoverTransport/newDocCoverTransportLeafclosely mirrorblockedDownloadTransport/cloneDownloadTransport/newDownloadTransportLeafininternal/validate/url.go, including the new DialTLS-guard wrapping. See consolidated comment below.🤖 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 `@shortcuts/doc/doc_resource_cover.go` around lines 727 - 801, Replace the duplicated blocked transport, clone logic, and leaf IP-guard implementation in blockedDocCoverTransport, cloneDocCoverTransport, and newDocCoverTransportLeaf with the shared transport-cloning/validation utilities from internal/validate/url.go. Reuse the existing blockedDownloadTransport, cloneDownloadTransport, and newDownloadTransportLeaf behavior, including DialTLS and DialTLSContext guarding, while preserving the document-cover fail-closed behavior.
🤖 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/cmdutil/factory_default.go`:
- Around line 97-126: The four rejection paths in safeRedirectPolicy must return
appropriately classified errs errors instead of fmt.Errorf: use validation for
redirect-count or invalid-request cases, network for transport redirect
failures, and security-policy errors for HTTPS downgrade or credential-stripping
policy violations. Preserve the existing messages and behavior, and update the
redirect assertions in factory_http_test.go to verify both the typed error and
message.
In `@internal/transport/policy_router.go`:
- Around line 56-68: Replace the bare fmt.Errorf calls with appropriate errs.*
constructors for all affected transport guards: the nil-request and
invalid-class handling in internal/transport/policy_router.go at lines 56-68 and
81-94, and the redirect-location and blocked-redirect handling in
internal/transport/default_client.go at lines 55-80. Update the relevant
RoundTrip and redirect-checking logic while preserving their existing control
flow and messages.
In `@internal/validate/url.go`:
- Around line 185-193: Replace the bare fmt.Errorf used to initialize
blockedDownloadTransport.err in the transport-cloning fallback with
errs.NewInternalError(errs.SubtypeUnknown, ...), matching the typed error
construction used by blockedDocCoverTransport. Preserve the existing "cannot
safely clone download transport %T" message and formatting arguments.
---
Outside diff comments:
In `@internal/validate/url.go`:
- Line 1: Extract the duplicated transport clone-and-guard wiring from
newDownloadTransportLeaf/configureDirectDownloadTransport and
cloneDocCoverTransport/newDocCoverTransportLeaf into one shared helper.
Parameterize it with the remote-IP validation predicate and proxy policy,
preserving frozen selected-proxy behavior for downloads and always-disabled
proxy behavior for document covers; ensure cloning failures fail closed and all
DialContext/DialTLSContext/DialTLS wrappers use consistent error handling.
- Around line 195-271: The direct transport cloning and dial/IP-validation logic
in newDownloadTransportLeaf and configureDirectDownloadTransport duplicates
newDocCoverTransportLeaf; consolidate these paths through the existing shared
transport-wrapper helper or extract their common implementation into one
reusable symbol. Preserve fail-closed handling,
DialContext/DialTLSContext/DialTLS coverage, connection closing on validation
failure, and the distinct proxy behavior.
In `@shortcuts/doc/doc_resource_cover.go`:
- Around line 727-801: Replace the duplicated blocked transport, clone logic,
and leaf IP-guard implementation in blockedDocCoverTransport,
cloneDocCoverTransport, and newDocCoverTransportLeaf with the shared
transport-cloning/validation utilities from internal/validate/url.go. Reuse the
existing blockedDownloadTransport, cloneDownloadTransport, and
newDownloadTransportLeaf behavior, including DialTLS and DialTLSContext
guarding, while preserving the document-cover fail-closed behavior.
🪄 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 Plus
Run ID: b8af4ea3-abfb-4bed-b614-408a8887f761
📒 Files selected for processing (34)
cmd/config/init_interactive.gocmd/doctor/doctor.goextension/transport/sidecar/interceptor_test.goextension/transport/types.gointernal/auth/transport.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_http_test.gointernal/cmdutil/factory_proxy_warn_test.gointernal/cmdutil/testing_test.gointernal/cmdutil/transport.gointernal/cmdutil/transport_test.gointernal/core/types.gointernal/core/types_test.gointernal/registry/remote.gointernal/transport/config.gointernal/transport/default_client.gointernal/transport/extension.gointernal/transport/extension_test.gointernal/transport/policy_router.gointernal/transport/policy_router_test.gointernal/transport/shared.gointernal/transport/shared_test.gointernal/update/update.gointernal/validate/url.gointernal/validate/url_test.goshortcuts/apps/file_common.goshortcuts/doc/doc_resource_cover.goshortcuts/doc/doc_resource_cover_test.goshortcuts/im/helpers.goshortcuts/mail/helpers.goshortcuts/mail/signature_compose.goshortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.go
e052549 to
daf7052
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/validate/url_test.go (1)
150-152: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse trusted TLS settings for the legacy
DialTLStests.Both tests can reuse the
tls.Configfromserver.Client().Transport.(*http.Transport).TLSClientConfiginstead of settingInsecureSkipVerify; this preserves the same trusted connection behavior without the static-analysis finding.🤖 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/validate/url_test.go` around lines 150 - 152, The legacy DialTLS tests use InsecureSkipVerify instead of the trusted TLS configuration. In internal/validate/url_test.go lines 150-152 and shortcuts/doc/doc_resource_cover_test.go lines 745-747, reuse the TLSClientConfig from server.Client().Transport.(*http.Transport) in the tls.Dial configuration, preserving the existing trusted connection behavior without disabling certificate verification.Source: Linters/SAST tools
🤖 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/validate/url_test.go`:
- Around line 150-152: The legacy DialTLS tests use InsecureSkipVerify instead
of the trusted TLS configuration. In internal/validate/url_test.go lines 150-152
and shortcuts/doc/doc_resource_cover_test.go lines 745-747, reuse the
TLSClientConfig from server.Client().Transport.(*http.Transport) in the tls.Dial
configuration, preserving the existing trusted connection behavior without
disabling certificate verification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b155bfd-3bd6-4d32-9fa4-a443e1f38bcf
📒 Files selected for processing (34)
cmd/config/init_interactive.gocmd/doctor/doctor.goextension/transport/sidecar/interceptor_test.goextension/transport/types.gointernal/auth/transport.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_http_test.gointernal/cmdutil/factory_proxy_warn_test.gointernal/cmdutil/testing_test.gointernal/cmdutil/transport.gointernal/cmdutil/transport_test.gointernal/core/types.gointernal/core/types_test.gointernal/registry/remote.gointernal/transport/config.gointernal/transport/default_client.gointernal/transport/extension.gointernal/transport/extension_test.gointernal/transport/policy_router.gointernal/transport/policy_router_test.gointernal/transport/shared.gointernal/transport/shared_test.gointernal/update/update.gointernal/validate/url.gointernal/validate/url_test.goshortcuts/apps/file_common.goshortcuts/doc/doc_resource_cover.goshortcuts/doc/doc_resource_cover_test.goshortcuts/im/helpers.goshortcuts/mail/helpers.goshortcuts/mail/signature_compose.goshortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.go
🚧 Files skipped from review as they are similar to previous changes (29)
- internal/cmdutil/testing_test.go
- shortcuts/im/helpers.go
- internal/core/types.go
- internal/transport/config.go
- shortcuts/mail/signature_compose.go
- cmd/doctor/doctor.go
- internal/auth/transport.go
- internal/update/update.go
- internal/transport/shared.go
- shortcuts/mail/helpers.go
- internal/cmdutil/factory.go
- shortcuts/minutes/minutes_download.go
- shortcuts/minutes/minutes_download_test.go
- internal/transport/default_client.go
- extension/transport/sidecar/interceptor_test.go
- shortcuts/apps/file_common.go
- internal/core/types_test.go
- internal/cmdutil/factory_default.go
- internal/transport/extension.go
- internal/validate/url.go
- cmd/config/init_interactive.go
- internal/transport/shared_test.go
- shortcuts/doc/doc_resource_cover.go
- internal/transport/extension_test.go
- internal/cmdutil/transport.go
- internal/transport/policy_router_test.go
- internal/transport/policy_router.go
- internal/cmdutil/transport_test.go
- internal/cmdutil/factory_http_test.go
daf7052 to
8d4c615
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cmdutil/transport_test.go`:
- Around line 97-131: Update
TestBuildSDKTransportAppliesSecurityHeadersToEveryRequestClass and the
additionally referenced HTTP tests to use the required isolated setup: assign
LARKSUITE_CLI_CONFIG_DIR to t.TempDir(), create the test configuration, and
construct the factory via cmdutil.TestFactory(t, config) before creating clients
or mocking requests. Preserve each test’s existing assertions and request
behavior.
- Around line 97-99: Update
TestBuildSDKTransportAppliesSecurityHeadersToEveryRequestClass to capture the
current exttransport provider before calling exttransport.Register(nil), then
register a t.Cleanup callback that restores the captured provider after the
test.
🪄 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 Plus
Run ID: fb05899e-8033-4164-b39e-b1cceb000b47
📒 Files selected for processing (35)
cmd/config/init_interactive.gocmd/doctor/doctor.goextension/transport/sidecar/interceptor_test.goextension/transport/types.gointernal/auth/transport.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_http_test.gointernal/cmdutil/factory_proxy_warn_test.gointernal/cmdutil/testing_test.gointernal/cmdutil/transport.gointernal/cmdutil/transport_test.gointernal/core/types.gointernal/core/types_test.gointernal/registry/remote.gointernal/riskcontrol/transport.gointernal/transport/config.gointernal/transport/default_client.gointernal/transport/extension.gointernal/transport/extension_test.gointernal/transport/policy_router.gointernal/transport/policy_router_test.gointernal/transport/shared.gointernal/transport/shared_test.gointernal/update/update.gointernal/validate/url.gointernal/validate/url_test.goshortcuts/apps/file_common.goshortcuts/doc/doc_resource_cover.goshortcuts/doc/doc_resource_cover_test.goshortcuts/im/helpers.goshortcuts/mail/helpers.goshortcuts/mail/signature_compose.goshortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.go
🚧 Files skipped from review as they are similar to previous changes (28)
- shortcuts/apps/file_common.go
- internal/registry/remote.go
- internal/cmdutil/factory_proxy_warn_test.go
- shortcuts/minutes/minutes_download.go
- internal/cmdutil/testing_test.go
- shortcuts/mail/signature_compose.go
- extension/transport/types.go
- internal/transport/config.go
- extension/transport/sidecar/interceptor_test.go
- internal/auth/transport.go
- cmd/doctor/doctor.go
- shortcuts/mail/helpers.go
- internal/core/types.go
- internal/update/update.go
- internal/transport/shared.go
- shortcuts/im/helpers.go
- internal/core/types_test.go
- internal/transport/default_client.go
- internal/transport/extension.go
- shortcuts/doc/doc_resource_cover.go
- internal/cmdutil/transport.go
- internal/transport/shared_test.go
- internal/transport/policy_router_test.go
- internal/transport/extension_test.go
- internal/cmdutil/factory_default.go
- internal/validate/url.go
- internal/transport/policy_router.go
- internal/cmdutil/factory_http_test.go
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 (1)
internal/validate/url.go (1)
73-117: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftNew error paths use bare
fmt.Errorfinstead of typederrs.*constructors.
resolveDownloadHost,ValidateDownloadSourceURL,NewDownloadHTTPClient'sCheckRedirect,proxyAwareDownloadTransport.RoundTrip,pinDownloadRequestTargetToIP,canRetryDownloadTarget's callers, andvalidateConnRemoteIPall construct errors with barefmt.Errorf(e.g."local/internal host is not allowed","failed to resolve host","blocked download target: %w"). A prior review already flagged this exact pattern forblockedDownloadTransport.errin this file, recommendingerrs.NewInternalError(...)instead of a bare error. The same fix applies broadly here: validation failures (bad host/URL) should use the validation-error constructor, and resolver/network failures should use the network-error constructor, so callers that classify errors viaerrs.ProblemOfdon't see these as unclassified internal errors.🔧 Example fix for resolveDownloadHost
func resolveDownloadHost(ctx context.Context, rawHost string, lookupIP downloadLookupIPFunc) ([]net.IP, error) { host := strings.TrimSpace(strings.ToLower(rawHost)) if host == "" { - return nil, fmt.Errorf("URL host is required") + return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "host", "URL host is required") } if host == "localhost" || strings.HasSuffix(host, ".localhost") { - return nil, fmt.Errorf("local/internal host is not allowed") + return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "host", "local/internal host is not allowed") }Based on learnings, error-path tests must assert typed metadata via
errs.ProblemOfrather than message substrings — but that's only possible once these call sites actually return typed errors.Also applies to: 121-148, 165-221, 263-265, 289-358, 486-506
🤖 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/validate/url.go` around lines 73 - 117, Replace bare fmt.Errorf calls across ValidateDownloadSourceURL, resolveDownloadHost, NewDownloadHTTPClient.CheckRedirect, proxyAwareDownloadTransport.RoundTrip, pinDownloadRequestTargetToIP, canRetryDownloadTarget callers, and validateConnRemoteIP with the appropriate errs constructors. Use validation errors for malformed or blocked URL/host targets and network errors for DNS, connection, or transport failures; preserve wrapped causes where applicable. Update related error-path tests to assert typed metadata through errs.ProblemOf rather than matching messages.Source: Coding guidelines
🧹 Nitpick comments (1)
internal/validate/url.go (1)
414-457: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTriplicated dial-wrap-and-validate logic across
DialContext/DialTLSContext/DialTLS.The three blocks wrapping
cloned.DialContext,cloned.DialTLSContext, andcloned.DialTLSeach repeat the same dial → check err →validateConnRemoteIP→ close-on-fail sequence. Extracting a small generic helper would reduce duplication; functionally this is correct, including the newDialTLSbranch that closes the gap left when onlyDialTLSContextwas covered.♻️ Suggested consolidation
+func wrapDialWithIPValidation[F ~func(string, string) (net.Conn, error)](orig F) F { + return func(network, addr string) (net.Conn, error) { + conn, err := orig(network, addr) + if err != nil { + return nil, err + } + if err := validateConnRemoteIP(conn); err != nil { + conn.Close() + return nil, err + } + return conn, nil + } +}🤖 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/validate/url.go` around lines 414 - 457, Refactor configureDirectDownloadTransport to extract the repeated dial, error-check, validateConnRemoteIP, and close-on-failure sequence into a small reusable helper. Update the DialContext, DialTLSContext, and DialTLS wrappers to delegate through that helper while preserving their existing callback signatures and behavior.
🤖 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/validate/url_internal_test.go`:
- Around line 96-106: Update TestProxiedDownloadRejectsRestrictedResolvedTarget
and TestProxiedPlainHTTPHostnameRejectsLocalProxy to inspect errors through
errs.ProblemOf, asserting the expected category, subtype, and parameter instead
of matching err.Error() substrings. Ensure resolveDownloadHost and RoundTrip
expose the corresponding typed errs errors so these assertions validate
structured metadata while preserving existing cause checks.
In `@internal/validate/url.go`:
- Around line 236-257: Update proxiedTransportForTLSServer so cloned transports
do not assign the downstream serverName to transport.TLSClientConfig.ServerName.
Preserve the proxy TLS configuration separately, allowing Go’s HTTPS-proxy
handshake to use the proxy hostname while retaining serverName only for
downstream transport selection and caching.
---
Outside diff comments:
In `@internal/validate/url.go`:
- Around line 73-117: Replace bare fmt.Errorf calls across
ValidateDownloadSourceURL, resolveDownloadHost,
NewDownloadHTTPClient.CheckRedirect, proxyAwareDownloadTransport.RoundTrip,
pinDownloadRequestTargetToIP, canRetryDownloadTarget callers, and
validateConnRemoteIP with the appropriate errs constructors. Use validation
errors for malformed or blocked URL/host targets and network errors for DNS,
connection, or transport failures; preserve wrapped causes where applicable.
Update related error-path tests to assert typed metadata through errs.ProblemOf
rather than matching messages.
---
Nitpick comments:
In `@internal/validate/url.go`:
- Around line 414-457: Refactor configureDirectDownloadTransport to extract the
repeated dial, error-check, validateConnRemoteIP, and close-on-failure sequence
into a small reusable helper. Update the DialContext, DialTLSContext, and
DialTLS wrappers to delegate through that helper while preserving their existing
callback signatures and behavior.
🪄 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 Plus
Run ID: c9e87b1e-f0f6-4be1-8f86-7b04cef4f661
📒 Files selected for processing (7)
internal/cmdutil/factory_default.gointernal/cmdutil/transport_test.gointernal/transport/default_client.gointernal/transport/extension_test.gointernal/validate/url.gointernal/validate/url_internal_test.gointernal/validate/url_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/transport/default_client.go
- internal/validate/url_test.go
- internal/cmdutil/factory_default.go
- internal/cmdutil/transport_test.go
Route platform and external requests through explicit policy chains, preserve transport decorators during safe download cloning, and harden bootstrap redirect and proxy boundaries.
b787454 to
06d5b18
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
internal/cmdutil/transport_test.go (1)
97-99: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMissing provider capture/restore reintroduces a previously flagged issue.
exttransport.Register(nil)overwrites the global extension provider with no capture/restore, unlike every other test in this file (133-196, 303-368, etc.) which all useprevious := exttransport.GetProvider()+t.Cleanup. This exact line range was flagged before and reported addressed, but the fix isn't present here.🩺 Proposed fix
func TestBuildSDKTransportAppliesSecurityHeadersToEveryRequestClass(t *testing.T) { - exttransport.Register(nil) + previous := exttransport.GetProvider() + exttransport.Register(nil) + t.Cleanup(func() { exttransport.Register(previous) }) received := make(chan http.Header, 2)🤖 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/cmdutil/transport_test.go` around lines 97 - 99, Update TestBuildSDKTransportAppliesSecurityHeadersToEveryRequestClass to capture the current extension provider before calling exttransport.Register(nil), then register a t.Cleanup callback that restores the captured provider, matching the established pattern used by the other tests in this file.
🧹 Nitpick comments (4)
internal/validate/url_internal_test.go (1)
130-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid falling back to the real resolver in a unit test.
net.DefaultResolver.LookupIPmakes this test depend on ambient DNS for any host other thanpublic.example. Returning a fixed IP (or an error) for unexpected hosts keeps the test hermetic and fails fast instead of hanging in offline CI sandboxes.🤖 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/validate/url_internal_test.go` around lines 130 - 135, Update the lookupIP test helper to avoid calling net.DefaultResolver.LookupIP for unexpected hosts; return a fixed deterministic IP or an explicit error instead, while preserving the public.example mapping so the test remains hermetic and fails fast.internal/auth/transport.go (1)
36-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a decorator contract test for
SecurityPolicyTransport.Both new methods are uncovered. A small test asserting that
transport.CloneHTTPTransportForRequestClass(or a directWithBaseRoundTripper/BaseRoundTripperround-trip) preserves the security policy layer would make a regression here fail loudly, sincepolicy_router.gorelies on this contract to keep the platform error-parsing layer while swapping the leaf*http.Transport.Note that
BaseRoundTripper()never returns nil (it falls back totransport.Fallback()), so the nil-guard intransformHTTPTransportForRequestClasswill never trip for this type — worth asserting the fallback path explicitly too.🤖 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/auth/transport.go` around lines 36 - 44, The SecurityPolicyTransport decorator methods lack coverage for preserving the security-policy layer while replacing the leaf transport. Add a focused contract test using WithBaseRoundTripper/BaseRoundTripper or CloneHTTPTransportForRequestClass that verifies the wrapped policy remains intact, and explicitly assert BaseRoundTripper falls back to transport.Fallback() when no base transport is configured.Sources: Coding guidelines, Linters/SAST tools
internal/transport/extension_test.go (1)
62-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore the previously registered provider instead of
nil.Other tests in this file capture
exttransport.GetProvider()and restore it in cleanup; these two reset the global registry tonil, which silently drops any provider installed by a package-level or earlier setup. Making all cleanups symmetric removes an ordering-dependent failure mode.♻️ Suggested change
+ previousProvider := exttransport.GetProvider() interceptor := &testHeaderInterceptor{} exttransport.Register(testProvider{interceptor: interceptor}) - t.Cleanup(func() { exttransport.Register(nil) }) + t.Cleanup(func() { exttransport.Register(previousProvider) })Also applies to: 138-144
🤖 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/transport/extension_test.go` around lines 62 - 64, The test cleanup around the provider registration must restore the previously registered provider rather than resetting it to nil. In each affected test, capture exttransport.GetProvider() before calling exttransport.Register, then have t.Cleanup restore that captured provider, matching the existing cleanup pattern elsewhere in the file.internal/transport/extension.go (1)
100-101: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
BaseRoundTripper()inRoundTripso a nilBasecannot panic.
BaseRoundTripper()already treats a nilBaseasShared(), andWithBaseRoundTripperaccepts a nil argument without substitution — so a rebuilt middleware with nil base would nil-deref here instead of falling back.♻️ Proposed change
req = req.WithContext(origCtx) - resp, err := m.Base.RoundTrip(req) + resp, err := m.BaseRoundTripper().RoundTrip(req)🤖 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/transport/extension.go` around lines 100 - 101, Update RoundTrip to invoke BaseRoundTripper() instead of dereferencing m.Base directly when forwarding the request. Preserve the existing context restoration and error handling while ensuring a nil base falls back to the shared round tripper.
🤖 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/cmdutil/factory_http_test.go`:
- Around line 321-325: Update the error assertions in the safeRedirectPolicy
test to obtain the typed problem via errs.ProblemOf(err) and assert its
redirect-error metadata, while retaining the existing HTTPS downgrade condition
as appropriate. Do not rely solely on err.Error() string matching; ensure the
test fails if safeRedirectPolicy returns an untyped error.
In `@internal/cmdutil/transport_test.go`:
- Around line 303-368: Update TestNewDefaultInstallsSDKBootstrapSecurityPolicy
to preserve and restore http.DefaultClient’s Transport and CheckRedirect while
exercising NewDefault’s global rewiring. Arrange the test transport so the final
http.DefaultClient.Do request is delivered to the roundTripFunc network, and
ensure non-SDK bootstrap traffic remains unmodified while SDK bootstrap policy
behavior is still asserted.
In `@internal/transport/default_client.go`:
- Around line 55-61: Update the bootstrap policy failure paths in the transport
initialization flow to return typed errors via
errs.NewInternalError(errs.SubtypeUnknown, ...), replacing the plain fmt.Errorf
results. Preserve the existing messages and wrap or otherwise retain the
upstream cause for the nil transport result when available.
In `@internal/transport/extension_test.go`:
- Around line 444-449: Update
TestSDKBootstrapRedirectGuardChecksLocationAfterExtensionPostHook and
TestSDKBootstrapTransportFailsClosedWithoutPlatformPolicy to inspect typed
metadata via errs.ProblemOf instead of only matching err.Error(). Assert the
expected category, subtype, and param for the cross-origin policy/access_denied
and missing-policy failures, following the existing assertions in the sibling
tests near lines 204-208 and 229-232, while preserving the external-call count
checks.
In `@internal/validate/url_internal_test.go`:
- Around line 78-82: Replace the unsynchronized proxyCalled flag in both
affected tests with a concurrency-safe atomic.Bool (or the existing
buffered-channel pattern), storing the value in the httptest handler and loading
it after RoundTrip before asserting that the proxy was not contacted.
In `@shortcuts/doc/doc_resource_cover_test.go`:
- Around line 731-734: Update the RoundTrip error assertion in the relevant
document-cover test to inspect errs.ProblemOf(err) and verify the expected
category, subtype errs.SubtypeUnknown, and parameter metadata for the
fail-closed clone error. Also assert that the original error is preserved as the
cause, while retaining the existing message check only if needed for context.
In `@shortcuts/doc/doc_resource_cover.go`:
- Around line 787-801: Extend the tests for the cloning logic that wraps
cloned.DialTLS to directly cover both outcomes: when
validateDocCoverConnRemoteIP rejects the connection, assert the connection is
closed and the returned error is the expected typed policy error; when
origDialTLS returns an error, assert the wrapper preserves that exact underlying
error.
- Around line 739-750: Update both reconstruction paths in the transport
transformation logic around newDocCoverTransportLeaf so they preserve the
configured proxy while still applying target-pinning safeguards. Remove the
behavior that clears cloned.Proxy, and ensure transports rebuilt from the
structural TransformHTTPTransport capability and *http.Transport.Clone retain
external proxy routing.
In `@shortcuts/mail/signature_compose.go`:
- Line 231: Add a test covering the signature image download flow around
ExternalHTTPClient, using a server response with a platform-shaped error payload
and asserting the download succeeds without platform protocol parsing. Ensure
the test exercises the external-client routing contract and would fail if the
implementation reverted to HttpClient().
---
Duplicate comments:
In `@internal/cmdutil/transport_test.go`:
- Around line 97-99: Update
TestBuildSDKTransportAppliesSecurityHeadersToEveryRequestClass to capture the
current extension provider before calling exttransport.Register(nil), then
register a t.Cleanup callback that restores the captured provider, matching the
established pattern used by the other tests in this file.
---
Nitpick comments:
In `@internal/auth/transport.go`:
- Around line 36-44: The SecurityPolicyTransport decorator methods lack coverage
for preserving the security-policy layer while replacing the leaf transport. Add
a focused contract test using WithBaseRoundTripper/BaseRoundTripper or
CloneHTTPTransportForRequestClass that verifies the wrapped policy remains
intact, and explicitly assert BaseRoundTripper falls back to
transport.Fallback() when no base transport is configured.
In `@internal/transport/extension_test.go`:
- Around line 62-64: The test cleanup around the provider registration must
restore the previously registered provider rather than resetting it to nil. In
each affected test, capture exttransport.GetProvider() before calling
exttransport.Register, then have t.Cleanup restore that captured provider,
matching the existing cleanup pattern elsewhere in the file.
In `@internal/transport/extension.go`:
- Around line 100-101: Update RoundTrip to invoke BaseRoundTripper() instead of
dereferencing m.Base directly when forwarding the request. Preserve the existing
context restoration and error handling while ensuring a nil base falls back to
the shared round tripper.
In `@internal/validate/url_internal_test.go`:
- Around line 130-135: Update the lookupIP test helper to avoid calling
net.DefaultResolver.LookupIP for unexpected hosts; return a fixed deterministic
IP or an explicit error instead, while preserving the public.example mapping so
the test remains hermetic and fails fast.
🪄 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 Plus
Run ID: 5061fa93-5da8-4179-9568-99ccd2bbc898
📒 Files selected for processing (38)
cmd/config/init_interactive.gocmd/doctor/doctor.goextension/transport/sidecar/interceptor_test.goextension/transport/types.gointernal/auth/transport.gointernal/client/client.gointernal/client/client_test.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_http_test.gointernal/cmdutil/factory_proxy_warn_test.gointernal/cmdutil/testing_test.gointernal/cmdutil/transport.gointernal/cmdutil/transport_test.gointernal/core/types.gointernal/core/types_test.gointernal/registry/remote.gointernal/riskcontrol/transport.gointernal/transport/config.gointernal/transport/default_client.gointernal/transport/extension.gointernal/transport/extension_test.gointernal/transport/policy_router.gointernal/transport/policy_router_test.gointernal/transport/shared.gointernal/transport/shared_test.gointernal/update/update.gointernal/validate/url.gointernal/validate/url_internal_test.gointernal/validate/url_test.goshortcuts/apps/file_common.goshortcuts/doc/doc_resource_cover.goshortcuts/doc/doc_resource_cover_test.goshortcuts/im/helpers.goshortcuts/mail/helpers.goshortcuts/mail/signature_compose.goshortcuts/minutes/minutes_download.goshortcuts/minutes/minutes_download_test.go
🚧 Files skipped from review as they are similar to previous changes (17)
- internal/transport/config.go
- internal/cmdutil/testing_test.go
- shortcuts/mail/helpers.go
- cmd/doctor/doctor.go
- shortcuts/minutes/minutes_download.go
- internal/cmdutil/factory_proxy_warn_test.go
- shortcuts/minutes/minutes_download_test.go
- internal/registry/remote.go
- shortcuts/apps/file_common.go
- internal/transport/shared_test.go
- cmd/config/init_interactive.go
- internal/client/client_test.go
- extension/transport/sidecar/interceptor_test.go
- extension/transport/types.go
- internal/transport/shared.go
- internal/client/client.go
- internal/transport/policy_router_test.go
| err = safeRedirectPolicy(redirect, []*http.Request{original, previous}) | ||
| if err == nil || !strings.Contains(err.Error(), "HTTPS") { | ||
| t.Fatalf("safeRedirectPolicy() error = %v, want later-hop HTTPS downgrade rejection", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the typed redirect error metadata.
This error path only checks the message, so a regression to an untyped error would pass.
Proposed test fix
err = safeRedirectPolicy(redirect, []*http.Request{original, previous})
if err == nil || !strings.Contains(err.Error(), "HTTPS") {
t.Fatalf("safeRedirectPolicy() error = %v, want later-hop HTTPS downgrade rejection", err)
}
+requireRedirectProblem(t, err, errs.CategoryPolicy, errs.SubtypeAccessDenied)As per coding guidelines, “Error-path tests must assert typed metadata through errs.ProblemOf.”
📝 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.
| err = safeRedirectPolicy(redirect, []*http.Request{original, previous}) | |
| if err == nil || !strings.Contains(err.Error(), "HTTPS") { | |
| t.Fatalf("safeRedirectPolicy() error = %v, want later-hop HTTPS downgrade rejection", err) | |
| } | |
| } | |
| err = safeRedirectPolicy(redirect, []*http.Request{original, previous}) | |
| if err == nil || !strings.Contains(err.Error(), "HTTPS") { | |
| t.Fatalf("safeRedirectPolicy() error = %v, want later-hop HTTPS downgrade rejection", err) | |
| } | |
| requireRedirectProblem(t, err, errs.CategoryPolicy, errs.SubtypeAccessDenied) | |
| } |
🤖 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/cmdutil/factory_http_test.go` around lines 321 - 325, Update the
error assertions in the safeRedirectPolicy test to obtain the typed problem via
errs.ProblemOf(err) and assert its redirect-error metadata, while retaining the
existing HTTPS downgrade condition as appropriate. Do not rely solely on
err.Error() string matching; ensure the test fails if safeRedirectPolicy returns
an untyped error.
Source: Coding guidelines
| type bootstrapPolicyTamperingInterceptor struct{} | ||
|
|
||
| func (bootstrapPolicyTamperingInterceptor) PreRoundTrip(req *http.Request) func(*http.Response, error) { | ||
| req.Header.Set(HeaderSource, "extension-value") | ||
| req.Header.Set(riskcontrol.HeaderOSType, "extension-value") | ||
| return nil | ||
| } | ||
|
|
||
| func TestNewDefaultInstallsSDKBootstrapSecurityPolicy(t *testing.T) { | ||
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) | ||
| oldTransport := http.DefaultClient.Transport | ||
| oldCheckRedirect := http.DefaultClient.CheckRedirect | ||
| t.Cleanup(func() { | ||
| http.DefaultClient.Transport = oldTransport | ||
| http.DefaultClient.CheckRedirect = oldCheckRedirect | ||
| }) | ||
|
|
||
| previous := exttransport.GetProvider() | ||
| exttransport.Register(&platformOnlyStubProvider{ | ||
| stubTransportProvider: &stubTransportProvider{ | ||
| interceptor: bootstrapPolicyTamperingInterceptor{}, | ||
| }, | ||
| }) | ||
| t.Cleanup(func() { exttransport.Register(previous) }) | ||
|
|
||
| var received http.Header | ||
| network := roundTripFunc(func(req *http.Request) (*http.Response, error) { | ||
| received = req.Header.Clone() | ||
| return &http.Response{ | ||
| StatusCode: http.StatusNoContent, | ||
| Body: http.NoBody, | ||
| Request: req, | ||
| }, nil | ||
| }) | ||
| http.DefaultClient.Transport = network | ||
| http.DefaultClient.CheckRedirect = nil | ||
| _ = NewDefault(nil, InvocationContext{}) | ||
|
|
||
| req, err := http.NewRequest( | ||
| http.MethodPost, | ||
| "https://open.feishu.cn/callback/ws/endpoint", | ||
| strings.NewReader(`{"app_secret":"secret"}`), | ||
| ) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| resp, err := http.DefaultClient.Do(req) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| resp.Body.Close() | ||
|
|
||
| if got := received.Get(HeaderSource); got != SourceValue { | ||
| t.Fatalf("%s = %q, want trusted value %q", HeaderSource, got, SourceValue) | ||
| } | ||
| if got := received.Get(riskcontrol.HeaderOSType); got != "" { | ||
| t.Fatalf("%s = %q, want extension value stripped", riskcontrol.HeaderOSType, got) | ||
| } | ||
| if got := received.Get(HeaderBuild); got != DetectBuildKind() { | ||
| t.Fatalf("%s = %q, want %q", HeaderBuild, got, DetectBuildKind()) | ||
| } | ||
| if got := received.Get(HeaderUserAgent); got != UserAgentValue() { | ||
| t.Fatalf("%s = %q, want %q", HeaderUserAgent, got, UserAgentValue()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how InstallSDKTransportBridge and buildSDKPlatformTransportWithBase touch global HTTP state.
rg -n -B3 -A20 'func InstallSDKTransportBridge' internal/transport
rg -n -B3 -A20 'func buildSDKPlatformTransportWithBase' internal/cmdutil
rg -n 'http\.DefaultTransport|http\.DefaultClient' internal/transport internal/cmdutilRepository: larksuite/cli
Length of output: 9068
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== default_client.go outline =="
ast-grep outline internal/transport/default_client.go --view expanded || true
echo
echo "== default_client.go relevant sections =="
sed -n '1,290p' internal/transport/default_client.go
echo
echo "== factory_default.go relevant sections =="
sed -n '180,270p' internal/cmdutil/factory_default.go
echo
echo "== transport.go outline =="
ast-grep outline internal/cmdutil/transport.go --view expanded || true
echo
echo "== transport.go relevant sections =="
sed -n '1,220p' internal/cmdutil/transport.go
echo
echo "== search NewDefault/InstallSDKTransportBridge usages =="
rg -n -B2 -A5 '\bNewDefault\b|\bInstallSDKTransportBridge\b' internal cmd test integration 2>/dev/null || trueRepository: larksuite/cli
Length of output: 29768
Confirm global http.DefaultClient rewiring is intentional and reset during tests.
NewDefault calls InstallSDKTransportBridge, whose sdkBootstrapHTTPClient alias is http.DefaultClient; the bridge wraps that client’s transport/check-redirect unconditionally, even if the test previously replaced Transport with network. Guard this test around the replacement so the final http.DefaultClient.Do request actually goes through the test network, and be clear that any non-SDK bootstrap traffic from http.DefaultClient is unmodified.
🤖 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/cmdutil/transport_test.go` around lines 303 - 368, Update
TestNewDefaultInstallsSDKBootstrapSecurityPolicy to preserve and restore
http.DefaultClient’s Transport and CheckRedirect while exercising NewDefault’s
global rewiring. Arrange the test transport so the final http.DefaultClient.Do
request is delivered to the roundTripFunc network, and ensure non-SDK bootstrap
traffic remains unmodified while SDK bootstrap policy behavior is still
asserted.
| if buildPlatformPolicy == nil { | ||
| return nil, fmt.Errorf("SDK bootstrap transport policy is not configured") | ||
| } | ||
| base = buildPlatformPolicy(base) | ||
| if base == nil { | ||
| return nil, fmt.Errorf("SDK bootstrap transport policy returned a nil transport") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect forbidigo configuration and existing CheckRedirect exception patterns.
fd -H '^\.golangci' -x cat -n
rg -n --type=go -C2 'nolint:forbidigo' internal/transport internal/cmdutil | head -60
rg -n --type=go 'fmt\.Errorf' internal/transport/default_client.goRepository: larksuite/cli
Length of output: 11727
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '1,190p' internal/transport/default_client.go | cat -n
echo "== errs.NewInternalError usage examples =="
rg -n --type=go 'errs\.NewInternal_ERROR|errs\.NewInternalError|SubtypeUnknown|NewSecurityPolicyError' internal | head -120
echo "== go.mod errs package =="
rg -n 'internal/errors|errors' go.modRepository: larksuite/cli
Length of output: 12354
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate errs package files =="
git ls-files | rg '(^|/)errs/|errors' | head -80
echo "== errs constructors and subtype enum definitions =="
rg -n --type=go 'func New(Internal|SecurityPolicy|API)Error|type Subtype|SubtypeUnknown|category |unknown|internal|security policy' errs internal -g '*.go' | head -160
echo "== golangci forbidigo exact pattern scan for current file =="
python3 - <<'PY'
from pathlib import Path
p=Path("internal/transport/default_client.go")
txt=p.read_text()
import re
for i,line in enumerate(txt.splitlines(),1):
if 'fmt.Errorf' in line or 'errors.New' in line:
print(f"{p}:{i}:{line}")
PYRepository: larksuite/cli
Length of output: 16697
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== errs package constructors =="
for f in errs/*.go; do echo "--- $f"; sed -n '1,220p' "$f"; done
echo "== golangci config exact forbidigo allowlist around errs-no-bare-wrap =="
sed -n '131,150p' .golangci.yml || trueRepository: larksuite/cli
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type=go 'func New(?:Internal|SecurityPolicy|API)Error|type InternalError|type SecurityPolicyError|WithCause' errs/types.go errs/types_test.go | head -120Repository: larksuite/cli
Length of output: 2158
Return typed errs.* errors from the bootstrap policy failure paths.
These are terminal request failures that bypass the existing typed redirect/cross-origin handling later in RoundTrip; use errs.NewInternalError(errs.SubtypeUnknown, ...) and preserve any upstream cause where available.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 60-60: internal/transport/default_client.go#L60
Added line #L60 was not covered by tests
🤖 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/transport/default_client.go` around lines 55 - 61, Update the
bootstrap policy failure paths in the transport initialization flow to return
typed errors via errs.NewInternalError(errs.SubtypeUnknown, ...), replacing the
plain fmt.Errorf results. Preserve the existing messages and wrap or otherwise
retain the upstream cause for the nil transport result when available.
Source: Coding guidelines
| if err == nil || !strings.Contains(err.Error(), "cross-origin redirect") { | ||
| t.Fatalf("Do() error = %v, want post-hook Location rejection", err) | ||
| } | ||
| if got := externalCalls.Load(); got != 0 { | ||
| t.Fatalf("post-hook redirect target calls = %d, want 0", got) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert typed error metadata, not just message substrings.
TestSDKBootstrapRedirectGuardChecksLocationAfterExtensionPostHook and TestSDKBootstrapTransportFailsClosedWithoutPlatformPolicy only match on err.Error(). The sibling tests at lines 204-208 and 229-232 already assert errs.ProblemOf category/subtype; doing the same here keeps the fail-closed and cross-origin guards pinned to their classification (policy/access_denied and whichever category the missing-policy guard uses) rather than to wording.
As per coding guidelines: "Error-path tests must assert typed metadata through errs.ProblemOf (category, subtype, and param)".
Also applies to: 728-733
🤖 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/transport/extension_test.go` around lines 444 - 449, Update
TestSDKBootstrapRedirectGuardChecksLocationAfterExtensionPostHook and
TestSDKBootstrapTransportFailsClosedWithoutPlatformPolicy to inspect typed
metadata via errs.ProblemOf instead of only matching err.Error(). Assert the
expected category, subtype, and param for the cross-origin policy/access_denied
and missing-policy failures, following the existing assertions in the sibling
tests near lines 204-208 and 229-232, while preserving the external-call count
checks.
Source: Coding guidelines
| proxyCalled := false | ||
| proxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | ||
| proxyCalled = true | ||
| w.WriteHeader(http.StatusNoContent) | ||
| })) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
proxyCalled is written from the server goroutine and read from the test goroutine.
Both tests set the flag inside the httptest handler and read it after RoundTrip, with no synchronization. If the guard ever regresses and the proxy is hit, go test -race reports a data race instead of the intended assertion failure. Use atomic.Bool (or a buffered channel, as the other tests here do).
♻️ Suggested change
- proxyCalled := false
+ var proxyCalled atomic.Bool
proxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
- proxyCalled = true
+ proxyCalled.Store(true)
w.WriteHeader(http.StatusNoContent)
}))
@@
- if proxyCalled {
+ if proxyCalled.Load() {Also applies to: 119-123
🤖 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/validate/url_internal_test.go` around lines 78 - 82, Replace the
unsynchronized proxyCalled flag in both affected tests with a concurrency-safe
atomic.Bool (or the existing buffered-channel pattern), storing the value in the
httptest handler and loading it after RoundTrip before asserting that the proxy
was not contacted.
| _, err = client.Transport.RoundTrip(req) | ||
| if err == nil || !strings.Contains(err.Error(), "cannot safely clone document cover transport") { | ||
| t.Fatalf("RoundTrip() error = %v, want fail-closed clone error", err) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the typed metadata of the fail-closed clone error.
blockedDocCoverTransport carries errs.NewInternalError(errs.SubtypeUnknown, ...); a substring-only assertion won't fail if the constructor is downgraded to a bare error. The equivalent test in internal/validate/url_test.go already checks errs.ProblemOf.
💚 Proposed addition
_, err = client.Transport.RoundTrip(req)
if err == nil || !strings.Contains(err.Error(), "cannot safely clone document cover transport") {
t.Fatalf("RoundTrip() error = %v, want fail-closed clone error", err)
}
+ problem, ok := errs.ProblemOf(err)
+ if !ok || problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeUnknown {
+ t.Fatalf("RoundTrip() problem = %#v, %v; want internal/unknown", problem, ok)
+ }As per coding guidelines, "Error-path tests must assert typed metadata through errs.ProblemOf (category, subtype, and param) and verify cause preservation rather than relying only on message substrings."
📝 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.
| _, err = client.Transport.RoundTrip(req) | |
| if err == nil || !strings.Contains(err.Error(), "cannot safely clone document cover transport") { | |
| t.Fatalf("RoundTrip() error = %v, want fail-closed clone error", err) | |
| } | |
| _, err = client.Transport.RoundTrip(req) | |
| if err == nil || !strings.Contains(err.Error(), "cannot safely clone document cover transport") { | |
| t.Fatalf("RoundTrip() error = %v, want fail-closed clone error", err) | |
| } | |
| problem, ok := errs.ProblemOf(err) | |
| if !ok || problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeUnknown { | |
| t.Fatalf("RoundTrip() problem = %#v, %v; want internal/unknown", problem, ok) | |
| } |
🤖 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 `@shortcuts/doc/doc_resource_cover_test.go` around lines 731 - 734, Update the
RoundTrip error assertion in the relevant document-cover test to inspect
errs.ProblemOf(err) and verify the expected category, subtype
errs.SubtypeUnknown, and parameter metadata for the fail-closed clone error.
Also assert that the original error is preserved as the cause, while retaining
the existing message check only if needed for context.
Source: Coding guidelines
| if source, ok := base.(interface { | ||
| TransformHTTPTransport(func(*http.Transport) (http.RoundTripper, bool)) (http.RoundTripper, bool) //nolint:forbidigo // structural capability preserves the caller's external transport graph. | ||
| }); ok { | ||
| rebuilt, transformed := source.TransformHTTPTransport(newDocCoverTransportLeaf) | ||
| if transformed && rebuilt != nil { | ||
| return rebuilt | ||
| } | ||
| } else if source, ok := base.(*http.Transport); ok && source != nil { | ||
| rebuilt, transformed := newDocCoverTransportLeaf(source.Clone()) | ||
| if transformed && rebuilt != nil { | ||
| return rebuilt | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Preserve the configured external proxy while hardening the transport.
Both reconstruction paths invoke newDocCoverTransportLeaf, which clears cloned.Proxy at Line 759. Consequently, a proxy configured on ExternalHTTPClient() is silently bypassed and the cover download uses direct egress. Rebuild this leaf with the proxy-aware target-pinning safeguards instead of disabling the proxy.
This contradicts the PR contract that external downloads preserve proxy routing.
🤖 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 `@shortcuts/doc/doc_resource_cover.go` around lines 739 - 750, Update both
reconstruction paths in the transport transformation logic around
newDocCoverTransportLeaf so they preserve the configured proxy while still
applying target-pinning safeguards. Remove the behavior that clears
cloned.Proxy, and ensure transports rebuilt from the structural
TransformHTTPTransport capability and *http.Transport.Clone retain external
proxy routing.
| if cloned.DialTLS != nil { | ||
| origDialTLS := cloned.DialTLS | ||
| cloned.DialTLS = func(network, addr string) (net.Conn, error) { | ||
| conn, err := origDialTLS(network, addr) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if err := validateDocCoverConnRemoteIP(conn); err != nil { | ||
| conn.Close() | ||
| return nil, err | ||
| } | ||
| return conn, nil | ||
| } | ||
| } | ||
| return cloned, true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Add direct coverage for the new DialTLS guard.
Test that an unsafe remote IP closes the returned connection and yields the expected typed policy error; also verify an underlying dial error remains preserved. These security-boundary lines are currently uncovered.
As per coding guidelines, “Every behavior change must have an accompanying test.”
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 792-792: shortcuts/doc/doc_resource_cover.go#L792
Added line #L792 was not covered by tests
[warning] 798-798: shortcuts/doc/doc_resource_cover.go#L798
Added line #L798 was not covered by tests
🤖 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 `@shortcuts/doc/doc_resource_cover.go` around lines 787 - 801, Extend the tests
for the cloning logic that wraps cloned.DialTLS to directly cover both outcomes:
when validateDocCoverConnRemoteIP rejects the connection, assert the connection
is closed and the returned error is the expected typed policy error; when
origDialTLS returns an error, assert the wrapper preserves that exact underlying
error.
Sources: Coding guidelines, Linters/SAST tools
| } | ||
|
|
||
| httpClient, err := runtime.Factory.HttpClient() | ||
| httpClient, err := runtime.Factory.ExternalHTTPClient() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the external-client routing contract.
Add a test where the server returns a platform-shaped error payload and verify the signature image download succeeds without platform protocol parsing. That test will fail if this line is reverted to HttpClient().
As per coding guidelines, “Every behavior change must have an accompanying test.”
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 231-231: shortcuts/mail/signature_compose.go#L231
Added line #L231 was not covered by tests
🤖 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 `@shortcuts/mail/signature_compose.go` at line 231, Add a test covering the
signature image download flow around ExternalHTTPClient, using a server response
with a platform-shaped error payload and asserting the download succeeds without
platform protocol parsing. Ensure the test exercises the external-client routing
contract and would fail if the implementation reverted to HttpClient().
Sources: Coding guidelines, Linters/SAST tools
1 participant
Summary
Centralize outbound HTTP transport construction and routing so platform-owned endpoints and external destinations use explicit, consistent policies. The change preserves existing provider and factory behavior while making proxy, redirect, security-header, and download handling easier to reason about and test.
Changes
Compatibility Notes
Test Plan
make buildGOFLAGS=-p=4 make unit-testgo vet ./...gofmt -l .produces no outputgo mod tidyleavesgo.modandgo.sumunchangedgo run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/mainmake sidecar-testlark-cli --versionRelated Issues
Summary by CodeRabbit