feat: add secure proxy mode and SEC_AUTH credentials#683
Open
JackZhao10086 wants to merge 4 commits intomainfrom
Open
feat: add secure proxy mode and SEC_AUTH credentials#683JackZhao10086 wants to merge 4 commits intomainfrom
JackZhao10086 wants to merge 4 commits intomainfrom
Conversation
github-actions
Bot
added
the
size/L
|
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 secplugin package and credential provider that load SEC config (file + env), enforce proxy/CA constraints, resolve accounts with strict-mode identity rules, emit sentinel tokens in SEC_AUTH mode, integrate the plugin into transport selection (fail-closed on errors), and add tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Prov as secplugin\nProvider
participant Config as secplugin\nConfig Loader
participant Transport as HTTP\nTransport
participant Proxy as SEC\nProxy (127.0.0.1)
CLI->>Prov: ResolveAccount()
Prov->>Config: Load() (sec_config.json + LARKSUITE_CLI_SEC_*)
Config-->>Prov: Config
Prov-->>CLI: Account (AppID/Brand/DefaultAs, SupportedIdentities)
CLI->>Prov: ResolveToken(type=UAT/TAT)
Prov-->>CLI: Token (SentinelUAT/SentinelTAT, Source="secplugin", Scopes=[])
CLI->>Transport: Send request (placeholder token)
Transport->>Proxy: Route via configured proxy (127.0.0.1)
Note over Proxy: Proxy swaps sentinel tokens for real creds
Proxy-->>Transport: Response
Transport-->>CLI: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #683 +/- ##
==========================================
+ Coverage 63.04% 63.12% +0.08%
==========================================
Files 437 440 +3
Lines 38847 39137 +290
==========================================
+ Hits 24491 24706 +215
- Misses 12184 12237 +53
- Partials 2172 2194 +22 ☔ 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@7a01c406c2a5ce5eb0cb34bb9e2c7ae858429d61🧩 Skill updatenpx skills add larksuite/cli#feat/sec_plugin -y -g |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
extension/credential/secplugin/provider_test.go (1)
51-54: Replace magic numbers with the namedIdentitySupportconstants.Hard-coding
1and2forSupportedIdentitiescouples the test to the current bit values. Importingextension/credentialand asserting againstcredential.SupportsBot/credential.SupportsUsermakes the intent explicit and keeps the test honest if those constants are ever reordered.♻️ Proposed change
- // StrictMode=bot => SupportsBot only. - if acct.SupportedIdentities != 2 { - t.Fatalf("acct.SupportedIdentities = %d, want %d (SupportsBot)", acct.SupportedIdentities, 2) - } + // StrictMode=bot => SupportsBot only. + if acct.SupportedIdentities != credential.SupportsBot { + t.Fatalf("acct.SupportedIdentities = %d, want SupportsBot", acct.SupportedIdentities) + }- // StrictMode=user => SupportsUser only (bit 1). - if acct.SupportedIdentities != 1 { - t.Fatalf("acct.SupportedIdentities = %d, want %d (SupportsUser)", acct.SupportedIdentities, 1) - } + // StrictMode=user => SupportsUser only. + if acct.SupportedIdentities != credential.SupportsUser { + t.Fatalf("acct.SupportedIdentities = %d, want SupportsUser", acct.SupportedIdentities) + }Also applies to: 94-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/credential/secplugin/provider_test.go` around lines 51 - 54, Replace the magic numeric checks of acct.SupportedIdentities with the named IdentitySupport constants from the credential package: import the extension/credential package and assert against credential.SupportsBot and credential.SupportsUser (e.g., use credential.SupportsBot instead of 2 and credential.SupportsUser instead of 1) in the test assertions that check acct.SupportedIdentities (the blocks currently expecting 1 or 2 and the similar assertions later around the second occurrence).internal/util/proxy_test.go (1)
16-37: Optional: consolidate the duplicated SEC env helpers.
unsetEnvandunsetSecPluginEnvhere are byte-for-byte identical to the helpers added ininternal/secplugin/config_test.go. If/when a third site needs them, consider promoting them into a small shared testutil package (e.g.internal/testutil/envtest) so the SEC env-var list lives in one place and stays in sync withinternal/envvars.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/util/proxy_test.go` around lines 16 - 37, The two helpers unsetEnv and unsetSecPluginEnv are duplicated (also present in internal/secplugin/config_test.go); extract them into a shared test helper package (e.g. internal/testutil/envtest) and update both locations to import and call the shared functions, keeping the same signatures and using the same env var constants (envvars.CliSecEnable, envvars.CliSecProxy, envvars.CliSecCA, envvars.CliSecAuth) so the SEC env list stays centralized and in sync with internal/envvars.internal/secplugin/config_test.go (1)
135-167: Minor: also callunsetSecPluginEnvfor symmetry.
TestLoad_EnvOnlyConfigandTestLoad_EnvOverridesFileboth reset cache state but skip theunsetSecPluginEnv(t)call before setting the env vars they care about.t.Setenvdoes restore on cleanup so the tests are correct as written, but addingunsetSecPluginEnv(t)first would defend against any new SEC env var added in the future that the test forgets to set explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/secplugin/config_test.go` around lines 135 - 167, The tests TestLoad_EnvOnlyConfig (and similarly TestLoad_EnvOverridesFile) reset the plugin cache (loadOnce, loadCfg, loadErr) but don't call unsetSecPluginEnv(t) before setting specific env vars, which risks leftover SEC env vars affecting the test; add a call to unsetSecPluginEnv(t) at the start of TestLoad_EnvOnlyConfig (before any t.Setenv calls and before resetting loadOnce/loadCfg/loadErr) so the environment is cleared, then proceed with the existing t.Setenv calls and cache resets.extension/credential/secplugin/provider.go (1)
35-35:ResolveTokenbypasses theloadSecConfigtest seam.
ResolveAccountuses theloadSecConfigpackage var (line 38), which is the testability hook, butResolveTokencallsinternalsec.Load()directly (line 173). Tests that swap outloadSecConfigto drive provider behavior under different SEC configs won't be able to influenceResolveToken, leading to inconsistent test fixturing or accidental real-config reads fromsec_config.jsonduring unit tests.♻️ Suggested change
func (p *Provider) ResolveToken(ctx context.Context, req credential.TokenSpec) (*credential.Token, error) { - cfg, err := internalsec.Load() + cfg, err := loadSecConfig() if err != nil { return nil, &credential.BlockError{Provider: p.Name(), Reason: err.Error()} }Also applies to: 172-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/credential/secplugin/provider.go` at line 35, ResolveToken currently calls internalsec.Load() directly, bypassing the test seam provided by the package var loadSecConfig which ResolveAccount uses; change ResolveToken to call loadSecConfig() (the package-level variable) instead of internalsec.Load() so tests can override loadSecConfig to control SEC config; update any other direct internalsec.Load() calls in the ResolveToken flow to use the same loadSecConfig variable to ensure consistent testability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extension/credential/secplugin/provider_test.go`:
- Around line 14-16: The tests (TestProvider_UsesSecConfigDefaults and the
similar test at lines 57-59) currently stub loadSecConfig but leave the real CLI
config directory reachable; update both tests to isolate config state by setting
the environment variable LARKSUITE_CLI_CONFIG_DIR to a new temporary directory
(os.MkdirTemp) at the start of each test and restore the original env value in
t.Cleanup; keep the existing stub for loadSecConfig and ensure the temp
directory is removed in cleanup so ResolveAccount cannot read the developer's
real config.json during the test.
In `@extension/credential/secplugin/provider.go`:
- Around line 78-90: In the multi-app fallback block in provider.go the variable
defaultAs is being unconditionally overwritten with
credential.Identity(app.DefaultAs); change this so defaultAs is only set from
app.DefaultAs when it is empty (i.e., preserve values already populated from
CliDefaultAs or cfg.DefaultAs). Locate the block around
appID/brand/defaultAs/mode and add a guard like "if defaultAs == \"\" then
defaultAs = credential.Identity(app.DefaultAs)" so existing env/config values
are not discarded, keeping the subsequent StrictModeBot/StrictModeUser branches
behavior intact.
- Around line 57-59: The code sets defaultAs from cfg.DefaultAs without
validating it when envvars.CliDefaultAs is unset; extract the validation logic
currently applied to os.Getenv(envvars.CliDefaultAs) into a reusable helper
(e.g., validateDefaultAs(string) error) and call it after you resolve defaultAs
(both when you assign credential.Identity(strings.TrimSpace(cfg.DefaultAs)) and
when you read the env var). Ensure the helper enforces the same allowed cases
(the switch logic used now) and returns an error if the value is invalid so the
caller can fail/handle consistently.
In `@internal/secplugin/config_test.go`:
- Around line 39-47: The test helper writeFile currently uses the production
vfs.* APIs which couples fixture setup to the code under test; change writeFile
to call os.MkdirAll and os.WriteFile instead (preserving the same permissions
and error handling) so fixtures under t.TempDir() are created with the stdlib
filesystem, and remove the vfs import from the test file; keep the helper name
writeFile and its signature intact and ensure error messages remain
t.Fatalf("MkdirAll: %v", err) / t.Fatalf("WriteFile: %v", err).
- Around line 49-65: The inline comment is contradictory to the manual reset of
loadOnce/loadCfg/loadErr in TestLoad_MissingFileReturnsNil; either remove the
comment or replace it with a brief accurate explanation that the manual reset is
required because multiple tests in the same package share the package-level
sync.Once/Load() cache, so we explicitly reset loadOnce, loadCfg and loadErr
before calling Load() to ensure this test runs with a fresh state. Reference the
TestLoad_MissingFileReturnsNil test and the package-level symbols loadOnce,
loadCfg, loadErr and the Load() function when making the change.
In `@internal/secplugin/tls_ca.go`:
- Around line 40-46: The TLS client config creation/clone needs MinVersion set
explicitly to TLS 1.2: after creating or cloning t.TLSClientConfig (the
tls.Config assigned in the t.TLSClientConfig = &tls.Config{} /
t.TLSClientConfig.Clone() branch) set t.TLSClientConfig.MinVersion =
tls.VersionTLS12 so the transport uses a minimum of TLS 1.2 (do this regardless
of whether the config was newly allocated or cloned).
---
Nitpick comments:
In `@extension/credential/secplugin/provider_test.go`:
- Around line 51-54: Replace the magic numeric checks of
acct.SupportedIdentities with the named IdentitySupport constants from the
credential package: import the extension/credential package and assert against
credential.SupportsBot and credential.SupportsUser (e.g., use
credential.SupportsBot instead of 2 and credential.SupportsUser instead of 1) in
the test assertions that check acct.SupportedIdentities (the blocks currently
expecting 1 or 2 and the similar assertions later around the second occurrence).
In `@extension/credential/secplugin/provider.go`:
- Line 35: ResolveToken currently calls internalsec.Load() directly, bypassing
the test seam provided by the package var loadSecConfig which ResolveAccount
uses; change ResolveToken to call loadSecConfig() (the package-level variable)
instead of internalsec.Load() so tests can override loadSecConfig to control SEC
config; update any other direct internalsec.Load() calls in the ResolveToken
flow to use the same loadSecConfig variable to ensure consistent testability.
In `@internal/secplugin/config_test.go`:
- Around line 135-167: The tests TestLoad_EnvOnlyConfig (and similarly
TestLoad_EnvOverridesFile) reset the plugin cache (loadOnce, loadCfg, loadErr)
but don't call unsetSecPluginEnv(t) before setting specific env vars, which
risks leftover SEC env vars affecting the test; add a call to
unsetSecPluginEnv(t) at the start of TestLoad_EnvOnlyConfig (before any t.Setenv
calls and before resetting loadOnce/loadCfg/loadErr) so the environment is
cleared, then proceed with the existing t.Setenv calls and cache resets.
In `@internal/util/proxy_test.go`:
- Around line 16-37: The two helpers unsetEnv and unsetSecPluginEnv are
duplicated (also present in internal/secplugin/config_test.go); extract them
into a shared test helper package (e.g. internal/testutil/envtest) and update
both locations to import and call the shared functions, keeping the same
signatures and using the same env var constants (envvars.CliSecEnable,
envvars.CliSecProxy, envvars.CliSecCA, envvars.CliSecAuth) so the SEC env list
stays centralized and in sync with internal/envvars.
🪄 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: f3ab082c-b2e4-4a91-bc63-92f3f09ea713
📒 Files selected for processing (11)
extension/credential/secplugin/provider.goextension/credential/secplugin/provider_test.gointernal/envvars/envvars.gointernal/secplugin/README.mdinternal/secplugin/README.zh-CN.mdinternal/secplugin/config.gointernal/secplugin/config_test.gointernal/secplugin/tls_ca.gointernal/util/proxy.gointernal/util/proxy_test.gomain.go
Comment on lines
+49
to
+65
| func TestLoad_MissingFileReturnsNil(t *testing.T) { | ||
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) | ||
| loadOnce = sync.Once{} | ||
| loadCfg = nil | ||
| loadErr = nil | ||
| unsetSecPluginEnv(t) | ||
| // Reset cache for this test run by forking package state: | ||
| // Load() uses sync.Once; the repository test runner executes packages | ||
| // in separate processes, so this is safe without manual reset. | ||
| cfg, err := Load() | ||
| if err != nil { | ||
| t.Fatalf("Load() error = %v", err) | ||
| } | ||
| if cfg != nil { | ||
| t.Fatalf("Load() = %#v, want nil (missing file)", cfg) | ||
| } | ||
| } |
There was a problem hiding this comment.
Comment contradicts the code immediately above it.
Lines 51-53 perform a manual reset of loadOnce/loadCfg/loadErr, but the comment on lines 55-57 then claims this is "safe without manual reset". Either drop the comment (the reset is self-explanatory) or rewrite it to actually describe why the reset is needed (multiple Load()-exercising tests in the same package share sync.Once state).
🧹 Proposed change
loadOnce = sync.Once{}
loadCfg = nil
loadErr = nil
unsetSecPluginEnv(t)
- // Reset cache for this test run by forking package state:
- // Load() uses sync.Once; the repository test runner executes packages
- // in separate processes, so this is safe without manual reset.
cfg, err := Load()📝 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
| func TestLoad_MissingFileReturnsNil(t *testing.T) { | |
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) | |
| loadOnce = sync.Once{} | |
| loadCfg = nil | |
| loadErr = nil | |
| unsetSecPluginEnv(t) | |
| // Reset cache for this test run by forking package state: | |
| // Load() uses sync.Once; the repository test runner executes packages | |
| // in separate processes, so this is safe without manual reset. | |
| cfg, err := Load() | |
| if err != nil { | |
| t.Fatalf("Load() error = %v", err) | |
| } | |
| if cfg != nil { | |
| t.Fatalf("Load() = %#v, want nil (missing file)", cfg) | |
| } | |
| } | |
| func TestLoad_MissingFileReturnsNil(t *testing.T) { | |
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) | |
| loadOnce = sync.Once{} | |
| loadCfg = nil | |
| loadErr = nil | |
| unsetSecPluginEnv(t) | |
| cfg, err := Load() | |
| if err != nil { | |
| t.Fatalf("Load() error = %v", err) | |
| } | |
| if cfg != nil { | |
| t.Fatalf("Load() = %#v, want nil (missing file)", cfg) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/secplugin/config_test.go` around lines 49 - 65, The inline comment
is contradictory to the manual reset of loadOnce/loadCfg/loadErr in
TestLoad_MissingFileReturnsNil; either remove the comment or replace it with a
brief accurate explanation that the manual reset is required because multiple
tests in the same package share the package-level sync.Once/Load() cache, so we
explicitly reset loadOnce, loadCfg and loadErr before calling Load() to ensure
this test runs with a fresh state. Reference the TestLoad_MissingFileReturnsNil
test and the package-level symbols loadOnce, loadCfg, loadErr and the Load()
function when making the change.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
extension/credential/secplugin/provider.go (1)
175-201: Use theloadSecConfigindirection for testability consistency.
ResolveAccountuses the package-levelloadSecConfigvariable (line 41) so tests can stub config loading, butResolveTokencallsinternalsec.Load()directly (line 177). This is inconsistent and means token-path tests cannot stub the config the same way account-path tests can.♻️ Proposed change
- cfg, err := internalsec.Load() + cfg, err := loadSecConfig()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/credential/secplugin/provider.go` around lines 175 - 201, ResolveToken currently calls internalsec.Load() directly which prevents tests from stubbing config loading; change ResolveToken to call the package-level loadSecConfig indirection (the same variable used by ResolveAccount) instead of internalsec.Load(), preserving the existing error handling and return types (i.e., if loadSecConfig() returns (cfg, err) handle err by returning &credential.BlockError{Provider: p.Name(), Reason: err.Error()} and keep the same nil/placeholder token behavior when cfg is nil or cfg.AuthEnabled() is false); update references in the ResolveToken function to use the cfg returned by loadSecConfig().internal/secplugin/config.go (3)
240-257:ApplyToTransportlooks correct; one defensive note.Line 244's
http.DefaultTransport.(*http.Transport)will panic if anything in the process has replacedhttp.DefaultTransportwith a differentRoundTripperimplementation. In practice this is rare, but since this transport is critical (fail-closed), consider a typed assertion with, okand a clear error so a misconfigured embedder gets a clean error instead of a panic.♻️ Proposed change
if base == nil { - base = http.DefaultTransport.(*http.Transport) + dt, ok := http.DefaultTransport.(*http.Transport) + if !ok { + return nil, fmt.Errorf("sec plugin: http.DefaultTransport is not *http.Transport") + } + base = dt }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/secplugin/config.go` around lines 240 - 257, The code uses a naked type assertion http.DefaultTransport.(*http.Transport) in ApplyToTransport which can panic if DefaultTransport has been replaced; change this to a checked assertion (e.g., tDefault, ok := http.DefaultTransport.(*http.Transport)) and return a clear error when ok is false so ApplyToTransport fails cleanly instead of panicking; ensure the rest of ApplyToTransport (proxyURL(), base.Clone(), applyExtraRootCA) operates on the validated *http.Transport value.
122-128: Merge branch is awkwardly inverted — consider simplifying.When
cfg == nilyou skip env merging (correct, sincehasEnvwas false). Whencfg != nil, you overwrite the env-derivedcfgwithfileCfgand then re-runapplyEnvOverrides. The intent ("file as base, env wins") is clearer if you start fromfileCfgand conditionally apply env:♻️ Proposed simplification
- // Merge: file base + env overrides. - if cfg == nil { - cfg = &fileCfg - } else { - *cfg = fileCfg - applyEnvOverrides(cfg) - } - loadCfg = cfg + // Merge: file base + env overrides (env always wins when present). + merged := fileCfg + if hasEnv { + if err := applyEnvOverrides(&merged); err != nil { + loadErr = err + return + } + } + loadCfg = &mergedThis also drops the unused first call to
applyEnvOverridesinsideloadFromEnvfor this code path (its only purpose there is thehasEnvearly-return validation, which can be moved up).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/secplugin/config.go` around lines 122 - 128, The merge logic in the block that handles file + env overrides is inverted: instead of overwriting env-derived cfg with fileCfg and then re-applying applyEnvOverrides, start from fileCfg and apply env overrides only when env exists; update the code that sets cfg to initialize cfg = &fileCfg (or copy fileCfg into cfg) and then call applyEnvOverrides(cfg) only if loadFromEnv reported hasEnv/true, and remove the redundant applyEnvOverrides call inside loadFromEnv used solely for hasEnv detection; reference the variables and functions cfg, fileCfg, applyEnvOverrides, and loadFromEnv when making this change so the merging semantics become "file as base, env wins."
87-132: Cached*Configis returned by pointer — callers can mutate shared state.
Load()caches a single*Configand returns it to every caller. A caller mutating any field (e.g. test code, transport application paths) will silently affect every subsequent caller for the lifetime of the process. Consider returning a defensive copy, or document that the returned*Configmust be treated as immutable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/secplugin/config.go` around lines 87 - 132, Load() currently caches and returns the same *Config pointer (loadCfg), letting callers mutate shared state; change Load to always return a defensive copy of the cached config instead of the internal pointer. Keep the caching behavior (loadOnce, loadCfg) but before returning, if loadCfg != nil create and return a cloned Config (e.g., implement a helper cloneConfig(cfg *Config) *Config that performs a deep copy — by manual field copy or marshal/unmarshal — and return that copy; ensure code paths that set loadCfg still store the internal pointer but callers receive cloneConfig(loadCfg). Update any references to Load() usage as needed but do not expose the cached pointer directly.
🤖 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/secplugin/config.go`:
- Around line 198-223: The proxyURL validator in proxyURL() currently misses
checks for query, fragment and explicit port so URLs like
http://127.0.0.1?foo=bar or http://127.0.0.1#frag or port-less hosts slip
through; update proxyURL to also reject any URL with u.RawQuery != "" or
u.Fragment != "" and require u.Port() != "" (i.e. explicit port) before
returning u, keeping existing redaction/error formatting and using the same
envvars.CliSecProxy, redactProxyURL(raw) and url.Parse(raw) flow.
---
Nitpick comments:
In `@extension/credential/secplugin/provider.go`:
- Around line 175-201: ResolveToken currently calls internalsec.Load() directly
which prevents tests from stubbing config loading; change ResolveToken to call
the package-level loadSecConfig indirection (the same variable used by
ResolveAccount) instead of internalsec.Load(), preserving the existing error
handling and return types (i.e., if loadSecConfig() returns (cfg, err) handle
err by returning &credential.BlockError{Provider: p.Name(), Reason: err.Error()}
and keep the same nil/placeholder token behavior when cfg is nil or
cfg.AuthEnabled() is false); update references in the ResolveToken function to
use the cfg returned by loadSecConfig().
In `@internal/secplugin/config.go`:
- Around line 240-257: The code uses a naked type assertion
http.DefaultTransport.(*http.Transport) in ApplyToTransport which can panic if
DefaultTransport has been replaced; change this to a checked assertion (e.g.,
tDefault, ok := http.DefaultTransport.(*http.Transport)) and return a clear
error when ok is false so ApplyToTransport fails cleanly instead of panicking;
ensure the rest of ApplyToTransport (proxyURL(), base.Clone(), applyExtraRootCA)
operates on the validated *http.Transport value.
- Around line 122-128: The merge logic in the block that handles file + env
overrides is inverted: instead of overwriting env-derived cfg with fileCfg and
then re-applying applyEnvOverrides, start from fileCfg and apply env overrides
only when env exists; update the code that sets cfg to initialize cfg = &fileCfg
(or copy fileCfg into cfg) and then call applyEnvOverrides(cfg) only if
loadFromEnv reported hasEnv/true, and remove the redundant applyEnvOverrides
call inside loadFromEnv used solely for hasEnv detection; reference the
variables and functions cfg, fileCfg, applyEnvOverrides, and loadFromEnv when
making this change so the merging semantics become "file as base, env wins."
- Around line 87-132: Load() currently caches and returns the same *Config
pointer (loadCfg), letting callers mutate shared state; change Load to always
return a defensive copy of the cached config instead of the internal pointer.
Keep the caching behavior (loadOnce, loadCfg) but before returning, if loadCfg
!= nil create and return a cloned Config (e.g., implement a helper
cloneConfig(cfg *Config) *Config that performs a deep copy — by manual field
copy or marshal/unmarshal — and return that copy; ensure code paths that set
loadCfg still store the internal pointer but callers receive
cloneConfig(loadCfg). Update any references to Load() usage as needed but do not
expose the cached pointer directly.
🪄 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: 7ba7ea72-9bd8-4f45-a8a0-5a28f43d520f
📒 Files selected for processing (3)
extension/credential/secplugin/provider.gointernal/envvars/envvars.gointernal/secplugin/config.go
✅ Files skipped from review due to trivial changes (1)
- internal/envvars/envvars.go
JackZhao10086
force-pushed
the
feat/sec_plugin
branch
from
4bb648f to
32555b2
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (6)
extension/credential/secplugin/provider.go (2)
83-90:⚠️ Potential issue | 🟠 MajorPreserve the resolved
DefaultAsin the fallback path.
app.DefaultAsis assigned unconditionally here, which discards any env/config-provided default identity once the multi-app fallback runs. Only fill it whendefaultAsis still empty.Suggested fix
- defaultAs = credential.Identity(app.DefaultAs) + if defaultAs == "" { + defaultAs = credential.Identity(app.DefaultAs) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/credential/secplugin/provider.go` around lines 83 - 90, The code unconditionally overwrites the resolved default identity by assigning defaultAs = credential.Identity(app.DefaultAs); change this to only set defaultAs when it’s still empty (i.e., if defaultAs == ""), so env/config-provided default identities are preserved during the multi-app fallback; modify the block around appID, brand, and defaultAs (variables appID, brand, defaultAs and the use of app.DefaultAs / credential.Identity) to conditionally assign defaultAs only when it has not already been set.
55-64:⚠️ Potential issue | 🟡 MinorValidate
cfg.DefaultAsbefore reusing it.
cfg.DefaultAscan still flow through unvalidated whenCliDefaultAsis unset, so the provider may return an invalid identity instead of rejecting it consistently. Consider sharing the same validation logic between env and file sources.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/credential/secplugin/provider.go` around lines 55 - 64, The file currently assigns cfg.DefaultAs into defaultAs without running the same validation as the CLI/env path, which can allow an invalid identity to flow through; modify the provider so that cfg.DefaultAs is run through the same validation/parsing routine used for the CliDefaultAs/environment parsing (the code path that converts the env value into a credential.Identity) before assigning to defaultAs (or return an error/clear defaultAs if validation fails) — ensure you reference and reuse the existing validation function used for converting the env/CLI value to credential.Identity rather than directly doing defaultAs = credential.Identity(strings.TrimSpace(cfg.DefaultAs)).internal/secplugin/config_test.go (2)
41-50:⚠️ Potential issue | 🟡 MinorUse stdlib file helpers for the test fixture.
vfs.*is production plumbing; fixture creation undert.TempDir()should stay onos.MkdirAll/os.WriteFileso the test setup stays outside the abstraction under test. Based on learnings: "In larksuite/cli Go tests (files matching _test.go), when creating or populating test fixtures under t.TempDir(), use the standard library filesystem APIs: os.Mkdir, os.Create, os.CreateTemp, and os.WriteFile. Treat the vfs. abstraction as production-only infrastructure."Suggested fix
- if err := vfs.MkdirAll(filepath.Dir(path), 0700); err != nil { + if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { t.Fatalf("MkdirAll: %v", err) } - if err := vfs.WriteFile(path, data, perm); err != nil { + if err := os.WriteFile(path, data, perm); err != nil { t.Fatalf("WriteFile: %v", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/secplugin/config_test.go` around lines 41 - 50, The test helper writeFile currently uses the production vfs package; replace those calls with standard library file helpers so fixtures under t.TempDir() don't exercise the vfs abstraction. In writeFile, change vfs.MkdirAll to os.MkdirAll and vfs.WriteFile to os.WriteFile (keep the same parameters and error handling and retain t.Helper(), t.Fatalf usage) so the helper uses the stdlib for creating parent directories and writing the file.
60-62:⚠️ Potential issue | 🟡 MinorUpdate the cache-reset comment.
The comment says the reset is unnecessary, but the test explicitly resets
loadOnce,loadCfg, andloadErr. Either drop the comment or rewrite it to explain why the reset is required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/secplugin/config_test.go` around lines 60 - 62, The existing comment is misleading: the test explicitly resets the package-level cache variables loadOnce, loadCfg, and loadErr to force a fresh call to Load(); update the comment to state that the reset is required because sync.Once prevents subsequent Load() calls within the same process (so tests in the same package/process must clear those variables to isolate state), and mention that we clear loadOnce, loadCfg and loadErr here to ensure a deterministic fresh load for this test.internal/secplugin/tls_ca.go (1)
42-48:⚠️ Potential issue | 🟡 MinorSet an explicit TLS minimum on the cloned transport.
tls.Config{}still leaves the minimum version implicit here. Pinning the created/cloned config to TLS 1.2 avoids relying on stdlib defaults in this security-sensitive proxy path.Suggested fix
if t.TLSClientConfig == nil { - t.TLSClientConfig = &tls.Config{} + t.TLSClientConfig = &tls.Config{MinVersion: tls.VersionTLS12} } else { // Clone to avoid mutating shared config from the base transport. t.TLSClientConfig = t.TLSClientConfig.Clone() + if t.TLSClientConfig.MinVersion == 0 { + t.TLSClientConfig.MinVersion = tls.VersionTLS12 + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/secplugin/tls_ca.go` around lines 42 - 48, The TLS client config created or cloned in the transport (t.TLSClientConfig) should explicitly set a minimum TLS version to TLS 1.2; when you allocate a new tls.Config{} or when you call t.TLSClientConfig.Clone(), set t.TLSClientConfig.MinVersion = tls.VersionTLS12 before assigning RootCAs so the proxy does not rely on stdlib defaults (update the code paths around t.TLSClientConfig = &tls.Config{} and the Clone() branch to set MinVersion).internal/secplugin/config.go (1)
219-233:⚠️ Potential issue | 🟡 MinorTighten fixed-proxy validation.
url.Parsecurrently lets query strings, fragments, and port-less loopback hosts through. For a pinned local proxy endpoint, fail closed on those forms too.Suggested fix
if u.Hostname() != "127.0.0.1" { return nil, fmt.Errorf("invalid %s %q: host must be 127.0.0.1", envvars.CliSecProxy, redacted) } + if u.RawQuery != "" || u.Fragment != "" { + return nil, fmt.Errorf("invalid %s %q: query and fragment are not allowed", envvars.CliSecProxy, redacted) + } + if u.Port() == "" { + return nil, fmt.Errorf("invalid %s %q: port is required", envvars.CliSecProxy, redacted) + } if u.Path != "" && u.Path != "/" { return nil, fmt.Errorf("invalid %s %q: path is not allowed", envvars.CliSecProxy, redacted) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/secplugin/config.go` around lines 219 - 233, Tighten the proxy URL validation by rejecting query strings, fragments, and port-less loopback hosts: after the existing scheme/host checks (the block using u.Scheme, u.Host, u.Hostname(), u.Path and envvars.CliSecProxy/redacted), add checks that u.RawQuery == "" and u.Fragment == "" (error with a message like "query/fragment not allowed"), and require an explicit port by ensuring u.Port() != "" (error like "port is required, must be 127.0.0.1:<port>"). Keep the loopback hostname check using u.Hostname() == "127.0.0.1" and keep the path validation as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@extension/credential/secplugin/provider.go`:
- Around line 83-90: The code unconditionally overwrites the resolved default
identity by assigning defaultAs = credential.Identity(app.DefaultAs); change
this to only set defaultAs when it’s still empty (i.e., if defaultAs == ""), so
env/config-provided default identities are preserved during the multi-app
fallback; modify the block around appID, brand, and defaultAs (variables appID,
brand, defaultAs and the use of app.DefaultAs / credential.Identity) to
conditionally assign defaultAs only when it has not already been set.
- Around line 55-64: The file currently assigns cfg.DefaultAs into defaultAs
without running the same validation as the CLI/env path, which can allow an
invalid identity to flow through; modify the provider so that cfg.DefaultAs is
run through the same validation/parsing routine used for the
CliDefaultAs/environment parsing (the code path that converts the env value into
a credential.Identity) before assigning to defaultAs (or return an error/clear
defaultAs if validation fails) — ensure you reference and reuse the existing
validation function used for converting the env/CLI value to credential.Identity
rather than directly doing defaultAs =
credential.Identity(strings.TrimSpace(cfg.DefaultAs)).
In `@internal/secplugin/config_test.go`:
- Around line 41-50: The test helper writeFile currently uses the production vfs
package; replace those calls with standard library file helpers so fixtures
under t.TempDir() don't exercise the vfs abstraction. In writeFile, change
vfs.MkdirAll to os.MkdirAll and vfs.WriteFile to os.WriteFile (keep the same
parameters and error handling and retain t.Helper(), t.Fatalf usage) so the
helper uses the stdlib for creating parent directories and writing the file.
- Around line 60-62: The existing comment is misleading: the test explicitly
resets the package-level cache variables loadOnce, loadCfg, and loadErr to force
a fresh call to Load(); update the comment to state that the reset is required
because sync.Once prevents subsequent Load() calls within the same process (so
tests in the same package/process must clear those variables to isolate state),
and mention that we clear loadOnce, loadCfg and loadErr here to ensure a
deterministic fresh load for this test.
In `@internal/secplugin/config.go`:
- Around line 219-233: Tighten the proxy URL validation by rejecting query
strings, fragments, and port-less loopback hosts: after the existing scheme/host
checks (the block using u.Scheme, u.Host, u.Hostname(), u.Path and
envvars.CliSecProxy/redacted), add checks that u.RawQuery == "" and u.Fragment
== "" (error with a message like "query/fragment not allowed"), and require an
explicit port by ensuring u.Port() != "" (error like "port is required, must be
127.0.0.1:<port>"). Keep the loopback hostname check using u.Hostname() ==
"127.0.0.1" and keep the path validation as-is.
In `@internal/secplugin/tls_ca.go`:
- Around line 42-48: The TLS client config created or cloned in the transport
(t.TLSClientConfig) should explicitly set a minimum TLS version to TLS 1.2; when
you allocate a new tls.Config{} or when you call t.TLSClientConfig.Clone(), set
t.TLSClientConfig.MinVersion = tls.VersionTLS12 before assigning RootCAs so the
proxy does not rely on stdlib defaults (update the code paths around
t.TLSClientConfig = &tls.Config{} and the Clone() branch to set MinVersion).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7cf4df55-833f-4e6a-8380-fc9ddddec8ac
📒 Files selected for processing (12)
extension/credential/secplugin/provider.goextension/credential/secplugin/provider_test.gointernal/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.gomain.go
✅ Files skipped from review due to trivial changes (2)
- internal/secplugin/README.md
- internal/secplugin/README.zh-CN.md
🚧 Files skipped from review as they are similar to previous changes (4)
- main.go
- extension/credential/secplugin/provider_test.go
- internal/util/proxy.go
- internal/secplugin/tls_ca_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
extension/credential/secplugin/provider_test.go (1)
67-70: Prefer thecredential.SupportsBot/SupportsUserconstants over magic numbers.The strict-mode-from-disk subtest at lines 348-349 already uses
credential.SupportsBot/credential.SupportsUser. Using the literals2and1(with a hand-rolled comment) here is inconsistent and brittle to future bitmask changes.♻️ Suggested change
- // StrictMode=bot => SupportsBot only. - if acct.SupportedIdentities != 2 { - t.Fatalf("acct.SupportedIdentities = %d, want %d (SupportsBot)", acct.SupportedIdentities, 2) - } + if acct.SupportedIdentities != credential.SupportsBot { + t.Fatalf("acct.SupportedIdentities = %d, want %d (SupportsBot)", acct.SupportedIdentities, credential.SupportsBot) + }- // StrictMode=user => SupportsUser only (bit 1). - if acct.SupportedIdentities != 1 { - t.Fatalf("acct.SupportedIdentities = %d, want %d (SupportsUser)", acct.SupportedIdentities, 1) - } + if acct.SupportedIdentities != credential.SupportsUser { + t.Fatalf("acct.SupportedIdentities = %d, want %d (SupportsUser)", acct.SupportedIdentities, credential.SupportsUser) + }Also applies to: 112-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/credential/secplugin/provider_test.go` around lines 67 - 70, Replace the magic numeric literals used for SupportedIdentities with the credential bitmask constants: change the equality checks that compare acct.SupportedIdentities to 2 and 1 to use credential.SupportsBot and credential.SupportsUser respectively (e.g., in the assertion around acct.SupportedIdentities and also the similar check at the other occurrence around lines 112-115); update the t.Fatalf messages to reference the constant names for clarity if desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@extension/credential/secplugin/provider_test.go`:
- Line 309: The test does unchecked type assertions
`err.(*credential.BlockError)` (at the occurrences assigning to blockErr) which
will panic if err is a different type; change both to the comma-ok form: perform
`blockErr, ok := err.(*credential.BlockError)` and if !ok call `t.Fatalf` with a
clear message (or fail the test) so the test reports a readable failure instead
of an interface-conversion panic; update both occurrences (the assignments to
blockErr around the failing checks, including the one near the later assertion
at line 336) to follow the same pattern used earlier in the file.
---
Nitpick comments:
In `@extension/credential/secplugin/provider_test.go`:
- Around line 67-70: Replace the magic numeric literals used for
SupportedIdentities with the credential bitmask constants: change the equality
checks that compare acct.SupportedIdentities to 2 and 1 to use
credential.SupportsBot and credential.SupportsUser respectively (e.g., in the
assertion around acct.SupportedIdentities and also the similar check at the
other occurrence around lines 112-115); update the t.Fatalf messages to
reference the constant names for clarity if desired.
🪄 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: a429b8dc-a3ec-4b0b-b2e8-c9d57a2887a6
📒 Files selected for processing (1)
extension/credential/secplugin/provider_test.go
| if brand == "" && strings.TrimSpace(cfg.Brand) != "" { | ||
| brand = credential.Brand(strings.TrimSpace(cfg.Brand)) | ||
| } | ||
| if defaultAs == "" && strings.TrimSpace(cfg.DefaultAs) != "" { |
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
Introduce an optional "secplugin" mode that forces CLI outbound traffic through a local loopback proxy, optionally trusts an extra CA bundle, and supports
SEC_AUTHplaceholder-token authentication for proxy-injected credentials.Changes
internal/secpluginto load and validatesec_config.json/LARKSUITE_CLI_SEC_*, enforcehttp://127.0.0.1:<port>, and apply extra root CAs.extension/credential/secpluginprovider to return placeholder UAT/TAT whenSEC_AUTHis enabled and to read app defaults fromsec_config.json.main.goside-effect import.Test Plan
go test ./...)127.0.0.1proxy (LARKSUITE_CLI_SEC_ENABLE=true+LARKSUITE_CLI_SEC_PROXY=http://127.0.0.1:<port>)SEC_AUTHmode, downstream receives placeholder tokens (LARKSUITE_CLI_SEC_AUTH=true)Related Issues
Summary by CodeRabbit
New Features
Configuration
Documentation
Tests