Merged
Conversation
Keep real secrets (app_secret, access_token) out of sandbox environments. CLI instances inside sandboxes connect to a trusted sidecar process via HTTP; the sidecar verifies HMAC-signed requests and injects real tokens before forwarding to the Lark API. Key components: - `auth proxy` subcommand to start the sidecar server (build tag: authsidecar) - Noop credential provider returns sentinel tokens in sidecar mode - Transport interceptor rewrites requests to sidecar with HMAC signature - Env provider yields to sidecar provider when AUTH_PROXY is set - Supports both feishu and lark brand endpoints
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:
📝 WalkthroughWalkthroughThis PR introduces an auth sidecar proxy mode for handling authentication through an external HTTP proxy. It implements priority-based provider ordering in the credential registry, adds a new sidecar credential provider with priority 0, a transport interceptor that rewrites requests with HMAC signatures, cryptographic utilities for request verification, and a reference sidecar server implementation for development/testing. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as CLI Client
participant Cred as Credential<br/>Provider (Sidecar)
participant Transport as Transport<br/>Interceptor (Sidecar)
participant SidecarProxy as Sidecar<br/>Proxy Server
participant Backend as Backend<br/>API Server
Client->>Cred: Request credential
Cred->>Cred: Check CliAuthProxy env var
Cred-->>Client: Return sentinel token<br/>(UAT/TAT placeholder)
Client->>Client: Build HTTP request<br/>with sentinel token
Client->>Transport: PreRoundTrip hook
Transport->>Transport: Detect sentinel in<br/>Authorization/MCP header
Transport->>Transport: Buffer body & compute<br/>SHA-256 digest
Transport->>Transport: Compute HMAC-SHA256<br/>signature
Transport->>Transport: Rewrite URL to<br/>sidecar host
Transport->>Transport: Inject proxy headers<br/>(target, identity, sig, ts)
Transport-->>Client: Modified request
Client->>SidecarProxy: HTTP request with<br/>proxy headers
SidecarProxy->>SidecarProxy: Verify timestamp drift
SidecarProxy->>SidecarProxy: Verify HMAC signature
SidecarProxy->>SidecarProxy: Check target host<br/>in allowlist
SidecarProxy->>SidecarProxy: Resolve real token<br/>based on identity
SidecarProxy->>SidecarProxy: Inject real token<br/>into request
SidecarProxy->>Backend: Forward request to<br/>original target
Backend-->>SidecarProxy: Response
SidecarProxy-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@9a957b59d27c2b29a9c3a7bab5bb238f11c60144🧩 Skill updatenpx skills add larksuite/cli#feat/auth-sidecar-proxy -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
internal/sidecar/protocol.go (1)
108-117: Consider usingurl.Parsefor robustness.The manual scheme/slash stripping works for the tested inputs, but won't correctly handle edge cases like userinfo (
http://user:pass@host:port) or a URL with a stray path (http://host:port/fooreturnshost:port/foo). In practice callers runValidateProxyAddrfirst (which rejects paths), so this is a latent footgun rather than an active bug — worth hardening sinceProxyHostis exported.♻️ Possible refactor
func ProxyHost(authProxy string) string { - // Strip scheme - host := authProxy - if i := strings.Index(host, "://"); i >= 0 { - host = host[i+3:] - } - // Strip trailing slash - host = strings.TrimRight(host, "/") - return host + if strings.Contains(authProxy, "://") { + if u, err := url.Parse(authProxy); err == nil && u.Host != "" { + return u.Host + } + } + return strings.TrimRight(authProxy, "/") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sidecar/protocol.go` around lines 108 - 117, ProxyHost currently strips scheme and slashes manually which misses userinfo, ports and stray paths; change it to robustly parse the input using net/url: if authProxy lacks a scheme, prefix "http://" before calling url.Parse, then return the parsed URL's Host (u.Host) which correctly handles userinfo and port; if parsing fails or u.Host is empty, fall back to the previous safe trimming behavior (remove scheme if present and trim trailing slashes) so callers still get a usable host string. Ensure this logic is implemented inside the ProxyHost function.cmd/auth/proxy_test.go (2)
45-66: Fragile host-extraction in test helper.The "len of
https://" approach silently breaks if a test ever passes anhttp://target (it would strip 8 chars instead of 7, mangling the host). Consider a scheme-aware split to mirror what the production code (cmd/auth/proxy.golines 303-306 andextension/transport/sidecar/interceptor.go) actually does.Proposed refactor
- targetHost := target - if idx := len("https://"); len(target) > idx { - targetHost = target[idx:] - } + targetHost := target + if idx := strings.Index(target, "://"); idx >= 0 { + targetHost = target[idx+3:] + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/proxy_test.go` around lines 45 - 66, The test helper signedReq uses a brittle targetHost extraction via len("https://") which fails for other schemes; update signedReq to parse the target by splitting on "://" (e.g., strings.SplitN(target, "://", 2)) and use the second part when present, otherwise fall back to the original target, so targetHost matches the production logic used in cmd/auth/proxy.go and extension/transport/sidecar/interceptor.go; adjust imports if needed and keep signature creation via sidecar.Sign(key, method, targetHost, path, bodySHA, ts) unchanged.
151-169: Prefert.Setenvover manualos.Setenv/os.Unsetenvwith deferred cleanup.
t.Setenvhandles restoration (including the "wasn't set" case) and guards againstt.Parallelautomatically — less boilerplate and safer.Proposed refactor
- old, had := os.LookupEnv(envvars.CliAuthProxy) - os.Setenv(envvars.CliAuthProxy, "http://127.0.0.1:16384") - defer func() { - if had { - os.Setenv(envvars.CliAuthProxy, old) - } else { - os.Unsetenv(envvars.CliAuthProxy) - } - }() + t.Setenv(envvars.CliAuthProxy, "http://127.0.0.1:16384")The same applies to the
setEnv/unsetEnvhelpers inextension/credential/sidecar/provider_test.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/proxy_test.go` around lines 151 - 169, Replace the manual os.LookupEnv/os.Setenv/defer cleanup in TestRunProxy_RejectsSelfProxy with t.Setenv to let the testing framework manage restoration and t.Parallel safety: call t.Setenv(envvars.CliAuthProxy, "http://127.0.0.1:16384") at the start of the test, remove the old/had logic and deferred restore, and keep the runProxy(nil, &ProxyOptions{Listen: "127.0.0.1:0"}) and assertions unchanged; apply the same refactor to the setEnv/unsetEnv helper usages in extension/credential/sidecar/provider_test.go (replace manual env management with t.Setenv in those tests).cmd/auth/proxy.go (2)
437-445: UTF‑8 boundary issue insanitizeErrortruncation.Slicing a string at a byte offset can split a multi-byte rune and produce invalid UTF-8 in audit logs. Use a rune-aware truncation (or truncate then
strings.ToValidUTF8).Proposed fix
func sanitizeError(err error) string { s := err.Error() if len(s) > 200 { - return s[:200] + "..." + return strings.ToValidUTF8(s[:200], "") + "..." } return s }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/proxy.go` around lines 437 - 445, The sanitizeError function slices the error string by bytes which can split multi-byte runes and produce invalid UTF-8; update sanitizeError to perform rune-aware truncation so the returned string is always valid UTF-8 and still capped at ~200 bytes: inside sanitizeError iterate over runes (using utf8.RuneLen or utf8.DecodeRuneInString) accumulating byte length and stop before adding a rune that would exceed 200 bytes, then append "..." when truncated; keep the function name sanitizeError and its behavior of returning the full string when under the limit.
239-252: Replace magic bitmask literals with the namedcredential.Supports*constants.The comments already call out
SupportsUser bit/SupportsBot bit, and the equivalent constants are imported elsewhere in this package (credential.SupportsUser,credential.SupportsBot,credential.SupportsAll). Using them removes the risk of silently drifting if the bit assignment changes.Proposed refactor
- switch { - case cfg.SupportedIdentities == 0: // unknown/unset → allow both - ids[sidecar.IdentityUser] = true - ids[sidecar.IdentityBot] = true - case cfg.SupportedIdentities&1 != 0: // SupportsUser bit - ids[sidecar.IdentityUser] = true - } - if cfg.SupportedIdentities == 0 || cfg.SupportedIdentities&2 != 0 { // SupportsBot bit - ids[sidecar.IdentityBot] = true - } + if cfg.SupportedIdentities == 0 || cfg.SupportedIdentities&credential.SupportsUser != 0 { + ids[sidecar.IdentityUser] = true + } + if cfg.SupportedIdentities == 0 || cfg.SupportedIdentities&credential.SupportsBot != 0 { + ids[sidecar.IdentityBot] = true + }Also note the original
switchform drops the "user" bit whenSupportedIdentities == 0into its dedicated branch and relies on the post-switchifonly for bot — it's hard to reason about at a glance. The flattened form above is equivalent and easier to verify.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/proxy.go` around lines 239 - 252, In buildAllowedIdentities replace the magic bitmask literals (0, 1, 2) with the named constants credential.SupportsAll, credential.SupportsUser and credential.SupportsBot and simplify the branching: treat cfg.SupportedIdentities == credential.SupportsAll as allowing both, otherwise set ids[sidecar.IdentityUser] = true when cfg.SupportedIdentities&credential.SupportsUser != 0 and ids[sidecar.IdentityBot] = true when cfg.SupportedIdentities&credential.SupportsBot != 0; remove the original switch and trailing if and use these named constant checks so the intent is clear and resilient to bit changes.extension/transport/sidecar/interceptor.go (1)
122-131:Bearerprefix match is case-sensitive — minor robustness nit.Per RFC 7235, the auth scheme token is case-insensitive. If any SDK in the chain emits
bearer <sentinel>(lowercase) the sentinel won't match and the request will silently be passed through with the sentinel leaking to the upstream host. All current callers use"Bearer ", so this is defensive; just worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/transport/sidecar/interceptor.go` around lines 122 - 131, The Authorization header check in the interceptor uses a case-sensitive strings.TrimPrefix on "Bearer " so schemes like "bearer " will not be recognized; update the logic used in the req.Header.Get("Authorization") branch (where strings.TrimPrefix is called) to perform a case-insensitive check for the "Bearer" scheme before trimming (e.g., lower-case the prefix or use strings.HasPrefix(strings.ToLower(auth), "bearer ")), then extract the token and compare it against sidecar.SentinelUAT and sidecar.SentinelTAT to return sidecar.IdentityUser or sidecar.IdentityBot as before.extension/transport/sidecar/interceptor_test.go (1)
17-195: LGTM on coverage.The suite exercises the key branches (sentinel rewrite, pass-through, MCP variants, empty body, auth-header recording) against
PreRoundTrip. Consider also adding a regression test that ensuresX-Cli-Source‑style custom headers survive MCP-path rewrites and thatreq.GetBody(when originally non-nil) is re-wired so HTTP client retries can replay the body — currently only the default non-GetBody path is exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/transport/sidecar/interceptor_test.go` around lines 17 - 195, Add a regression test that calls Interceptor.PreRoundTrip on an MCP request (e.g., set req.URL to an MCP endpoint and set sidecar.HeaderMCPUAT to sidecar.SentinelUAT) but also sets a custom header like "X-Cli-Source" and a non-nil GetBody on the original *http.Request (create request with a ReadCloser and set req.GetBody to return a new reader of the same bytes); assert after PreRoundTrip that the host was rewritten, the MCP auth header was recorded/stripped (HeaderProxyAuthHeader / HeaderMCPUAT), the custom header "X-Cli-Source" is still present, and req.GetBody is non-nil and returns the original body bytes so HTTP client retries can replay the body. Ensure the test references Interceptor.PreRoundTrip, sidecar.HeaderMCPUAT, sidecar.HeaderProxyAuthHeader and uses req.GetBody in its assertions.extension/credential/sidecar/provider_test.go (1)
18-40: Replace custom env helpers witht.Setenv.
t.Setenvalready restores prior value (set or unset) and registers its own cleanup viat.Cleanup, so these helpers are redundant. Also,t.Setenvenforces non-parallel tests safely — rawos.Setenvdoes not.Proposed refactor
-func setEnv(t *testing.T, key, value string) { - t.Helper() - old, hadOld := os.LookupEnv(key) - os.Setenv(key, value) - t.Cleanup(func() { - if hadOld { - os.Setenv(key, old) - } else { - os.Unsetenv(key) - } - }) -} - -func unsetEnv(t *testing.T, key string) { - t.Helper() - old, hadOld := os.LookupEnv(key) - os.Unsetenv(key) - t.Cleanup(func() { - if hadOld { - os.Setenv(key, old) - } - }) -} +// Use t.Setenv(key, value) directly; for "unset" semantics in tests, call +// os.Unsetenv(key) only if preserving parallelism is unneeded, or set to "".Note:
t.Setenvhas no direct "unset" form — the simplest equivalent ist.Setenv(key, "")combined with youros.Getenv(key) == ""branches in the provider, which is how the code already behaves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/credential/sidecar/provider_test.go` around lines 18 - 40, The custom test helpers setEnv and unsetEnv in provider_test.go should be replaced with the standard testing.T method t.Setenv to avoid redundant cleanup logic and ensure safe non-parallel environment handling; locate the functions setEnv and unsetEnv and remove them, then update tests to call t.Setenv(key, value) for setting and to emulate "unset" use t.Setenv(key, "") (or set to empty string) where the code expects an unset/empty value, relying on t.Setenv's automatic restoration.internal/sidecar/hmac.go (1)
48-51: Simplify timestamp drift to pure int64 arithmetic.
math.Abs(float64(...))works correctly but adds an unnecessary int64→float64 round-trip. Since Unix epoch timestamps fit comfortably in int64 and we only need absolute difference for comparison, pure int64 arithmetic is simpler and clearer.MaxTimestampDriftis a plainintconstant (defined inprotocol.go:61), so int64 comparison is type-safe.Proposed refactor
- drift := math.Abs(float64(time.Now().Unix() - ts)) - if drift > MaxTimestampDrift { - return fmt.Errorf("timestamp drift %.0fs exceeds limit %ds", drift, MaxTimestampDrift) - } + drift := time.Now().Unix() - ts + if drift < 0 { + drift = -drift + } + if drift > int64(MaxTimestampDrift) { + return fmt.Errorf("timestamp drift %ds exceeds limit %d", drift, MaxTimestampDrift) + }This also eliminates the
mathimport.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sidecar/hmac.go` around lines 48 - 51, Replace the float math-based timestamp drift calculation with pure int64 arithmetic: compute drift as time.Now().Unix() - ts, take absolute by negating if negative, and compare against int64(MaxTimestampDrift) (or cast MaxTimestampDrift to int64) instead of using math.Abs on float64; update the drift variable and the conditional that returns the error (keep the same error message), and remove the unused "math" import. Reference symbols: ts, drift, MaxTimestampDrift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/auth/proxy_test.go`:
- Around line 171-221: The tests TestForwardClient_RedirectStripsAuth and
TestForwardClient_RedirectStripsMCPHeaders currently only rely on the redirect
target handler being invoked and don't assert the redirect was actually
followed; update both tests that use newForwardClient() to assert the response
indicates the redirect target was hit (e.g., check resp.StatusCode ==
http.StatusOK or increment/check a counter inside the redirectTarget handler) so
the test fails if redirects are not followed or the redirect handler is never
executed.
In `@cmd/auth/proxy.go`:
- Around line 165-176: The shutdown goroutine leaks the signal.Notify listener
and ignores shutdown errors; fix runProxy's goroutine by stopping the notifier
and handling Shutdown errors: create sigCh as now, immediately defer
signal.Stop(sigCh) so the notifier is unregistered when the goroutine exits,
keep the existing select on sigCh and ctx.Done(), then call shutdownCtx,
cancel(), call err := server.Shutdown(shutdownCtx) and log any non-nil err via
auditLogger (e.g., auditLogger.Printf("server shutdown error: %v", err)); if
shutdownCtx.Err() == context.DeadlineExceeded, call server.Close() and log that
forced close error as well.
- Around line 360-379: The loop copying headers currently lets client-supplied
auth headers through; update the logic so the sidecar is the sole source of auth
headers by stripping any client-supplied auth/MCP headers before injecting the
real token: when iterating r.Header (or immediately after constructing
forwardReq), skip or delete "Authorization", "X-Lark-MCP-UAT", and
"X-Lark-MCP-TAT" as well as the header name indicated by
sidecar.HeaderProxyAuthHeader so they are not forwarded, then set the injected
header from tokenResult.Token into forwardReq.Header (using "Authorization" when
HeaderProxyAuthHeader is empty or "Authorization"); reference isProxyHeader,
forwardReq, r.Header, sidecar.HeaderProxyAuthHeader, and tokenResult.Token.
- Around line 391-397: The current streaming of the upstream response (the
io.Copy(w, resp.Body) call) ignores errors so the audit entry still records
success; change the io.Copy call to capture its result (n, err := io.Copy(w,
resp.Body)) and if err != nil, record/log that error and update the audit
outcome/status accordingly (e.g., mark as upstream/error and include err and
bytes written) before returning; ensure the header-copy loop and
w.WriteHeader(resp.StatusCode) remain but that any io.Copy error is propagated
to the existing audit logging path so partial forwards are observable.
In `@extension/credential/registry.go`:
- Around line 17-38: The docstrings are inconsistent with the implementation:
update the comments in the Register and providerPriority blocks to state the
actual default priority is 10 (not 0 or 100) so they match the return value in
providerPriority; reference the Provider type and the optional Priority() int
method and clarify that providers without Priority() default to priority 10
(lower values consulted first) so comments around Register and providerPriority
are consistent with the code.
In `@extension/credential/sidecar/provider_test.go`:
- Around line 162-187: In TestResolveToken_Sentinels, guard against a nil token
returned by p.ResolveToken before accessing tok.Value or tok.Scopes: after each
ResolveToken call (both the UAT and TAT branches) add a check that tok != nil
and call t.Fatalf with a clear message if it is nil, so the test fails with an
actionable error instead of panicking on nil dereference when examining
tok.Value or tok.Scopes.
In `@extension/transport/sidecar/interceptor.go`:
- Around line 39-42: ResolveInterceptor currently calls
sidecar.ValidateProxyAddr(proxyAddr) and on error writes to stderr and calls
os.Exit(1); instead, do not terminate the process—either return an error per the
transport.Provider contract or follow the init() warn-and-skip behavior by
logging a warning and returning nil to skip the rewrite. Update
ResolveInterceptor to handle the validation error by using the existing
logger/warn path (matching the behavior in init() lines ~147-150) or by
returning the error to the caller rather than calling fmt.Fprintf/os.Exit;
reference the ResolveInterceptor function and sidecar.ValidateProxyAddr to
locate and change the exit logic.
---
Nitpick comments:
In `@cmd/auth/proxy_test.go`:
- Around line 45-66: The test helper signedReq uses a brittle targetHost
extraction via len("https://") which fails for other schemes; update signedReq
to parse the target by splitting on "://" (e.g., strings.SplitN(target, "://",
2)) and use the second part when present, otherwise fall back to the original
target, so targetHost matches the production logic used in cmd/auth/proxy.go and
extension/transport/sidecar/interceptor.go; adjust imports if needed and keep
signature creation via sidecar.Sign(key, method, targetHost, path, bodySHA, ts)
unchanged.
- Around line 151-169: Replace the manual os.LookupEnv/os.Setenv/defer cleanup
in TestRunProxy_RejectsSelfProxy with t.Setenv to let the testing framework
manage restoration and t.Parallel safety: call t.Setenv(envvars.CliAuthProxy,
"http://127.0.0.1:16384") at the start of the test, remove the old/had logic and
deferred restore, and keep the runProxy(nil, &ProxyOptions{Listen:
"127.0.0.1:0"}) and assertions unchanged; apply the same refactor to the
setEnv/unsetEnv helper usages in extension/credential/sidecar/provider_test.go
(replace manual env management with t.Setenv in those tests).
In `@cmd/auth/proxy.go`:
- Around line 437-445: The sanitizeError function slices the error string by
bytes which can split multi-byte runes and produce invalid UTF-8; update
sanitizeError to perform rune-aware truncation so the returned string is always
valid UTF-8 and still capped at ~200 bytes: inside sanitizeError iterate over
runes (using utf8.RuneLen or utf8.DecodeRuneInString) accumulating byte length
and stop before adding a rune that would exceed 200 bytes, then append "..."
when truncated; keep the function name sanitizeError and its behavior of
returning the full string when under the limit.
- Around line 239-252: In buildAllowedIdentities replace the magic bitmask
literals (0, 1, 2) with the named constants credential.SupportsAll,
credential.SupportsUser and credential.SupportsBot and simplify the branching:
treat cfg.SupportedIdentities == credential.SupportsAll as allowing both,
otherwise set ids[sidecar.IdentityUser] = true when
cfg.SupportedIdentities&credential.SupportsUser != 0 and
ids[sidecar.IdentityBot] = true when
cfg.SupportedIdentities&credential.SupportsBot != 0; remove the original switch
and trailing if and use these named constant checks so the intent is clear and
resilient to bit changes.
In `@extension/credential/sidecar/provider_test.go`:
- Around line 18-40: The custom test helpers setEnv and unsetEnv in
provider_test.go should be replaced with the standard testing.T method t.Setenv
to avoid redundant cleanup logic and ensure safe non-parallel environment
handling; locate the functions setEnv and unsetEnv and remove them, then update
tests to call t.Setenv(key, value) for setting and to emulate "unset" use
t.Setenv(key, "") (or set to empty string) where the code expects an unset/empty
value, relying on t.Setenv's automatic restoration.
In `@extension/transport/sidecar/interceptor_test.go`:
- Around line 17-195: Add a regression test that calls Interceptor.PreRoundTrip
on an MCP request (e.g., set req.URL to an MCP endpoint and set
sidecar.HeaderMCPUAT to sidecar.SentinelUAT) but also sets a custom header like
"X-Cli-Source" and a non-nil GetBody on the original *http.Request (create
request with a ReadCloser and set req.GetBody to return a new reader of the same
bytes); assert after PreRoundTrip that the host was rewritten, the MCP auth
header was recorded/stripped (HeaderProxyAuthHeader / HeaderMCPUAT), the custom
header "X-Cli-Source" is still present, and req.GetBody is non-nil and returns
the original body bytes so HTTP client retries can replay the body. Ensure the
test references Interceptor.PreRoundTrip, sidecar.HeaderMCPUAT,
sidecar.HeaderProxyAuthHeader and uses req.GetBody in its assertions.
In `@extension/transport/sidecar/interceptor.go`:
- Around line 122-131: The Authorization header check in the interceptor uses a
case-sensitive strings.TrimPrefix on "Bearer " so schemes like "bearer " will
not be recognized; update the logic used in the req.Header.Get("Authorization")
branch (where strings.TrimPrefix is called) to perform a case-insensitive check
for the "Bearer" scheme before trimming (e.g., lower-case the prefix or use
strings.HasPrefix(strings.ToLower(auth), "bearer ")), then extract the token and
compare it against sidecar.SentinelUAT and sidecar.SentinelTAT to return
sidecar.IdentityUser or sidecar.IdentityBot as before.
In `@internal/sidecar/hmac.go`:
- Around line 48-51: Replace the float math-based timestamp drift calculation
with pure int64 arithmetic: compute drift as time.Now().Unix() - ts, take
absolute by negating if negative, and compare against int64(MaxTimestampDrift)
(or cast MaxTimestampDrift to int64) instead of using math.Abs on float64;
update the drift variable and the conditional that returns the error (keep the
same error message), and remove the unused "math" import. Reference symbols: ts,
drift, MaxTimestampDrift.
In `@internal/sidecar/protocol.go`:
- Around line 108-117: ProxyHost currently strips scheme and slashes manually
which misses userinfo, ports and stray paths; change it to robustly parse the
input using net/url: if authProxy lacks a scheme, prefix "http://" before
calling url.Parse, then return the parsed URL's Host (u.Host) which correctly
handles userinfo and port; if parsing fails or u.Host is empty, fall back to the
previous safe trimming behavior (remove scheme if present and trim trailing
slashes) so callers still get a usable host string. Ensure this logic is
implemented inside the ProxyHost function.
🪄 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: e0a8e301-f198-45fd-a555-ca2219711dc7
📒 Files selected for processing (15)
cmd/auth/auth.gocmd/auth/proxy.gocmd/auth/proxy_stub.gocmd/auth/proxy_test.goextension/credential/registry.goextension/credential/registry_test.goextension/credential/sidecar/provider.goextension/credential/sidecar/provider_test.goextension/transport/sidecar/interceptor.goextension/transport/sidecar/interceptor_test.gointernal/envvars/envvars.gointernal/sidecar/hmac.gointernal/sidecar/hmac_test.gointernal/sidecar/protocol.gomain_authsidecar.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/transport/sidecar/interceptor.go`:
- Around line 91-100: ReadAll's error must be handled and the original req.Body
closed to avoid leaking resources and sending misleading signatures: when
calling io.ReadAll(req.Body) check the returned error; if err != nil close the
original req.Body (if it implements io.Closer), log or warn about the read
failure, and do NOT replace req.Body/req.GetBody or rewrite the request body so
the request is dispatched without sidecar signing; if ReadAll succeeds, replace
req.Body and req.GetBody with io.NopCloser(bytes.NewReader(bodyBytes)) as before
but also close the original req.Body after copying to avoid a leak.
🪄 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: c5fdffe0-ec03-418f-b334-481de5b5c4f5
📒 Files selected for processing (4)
cmd/auth/proxy.gocmd/auth/proxy_test.goextension/credential/registry.goextension/transport/sidecar/interceptor.go
✅ Files skipped from review due to trivial changes (2)
- cmd/auth/proxy_test.go
- cmd/auth/proxy.go
🚧 Files skipped from review as they are similar to previous changes (1)
- extension/credential/registry.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
extension/transport/sidecar/interceptor.go (2)
86-91: Consider settingGetBodyunconditionally once the body is buffered.You've already materialized the full body into
bodyBytes, so you can always provide a cheapGetBodyregardless of whether the original request had one. This enables transparenthttp.Transportretries (e.g. idempotent replays on connection close) for signed sidecar requests where the caller only setreq.Body. Since the signed timestamp is frozen at this point, a retry within the drift window remains valid.Proposed refactor
req.Body = io.NopCloser(bytes.NewReader(bodyBytes)) - if req.GetBody != nil { - req.GetBody = func() (io.ReadCloser, error) { - return io.NopCloser(bytes.NewReader(bodyBytes)), nil - } - } + req.GetBody = func() (io.ReadCloser, error) { + return io.NopCloser(bytes.NewReader(bodyBytes)), nil + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/transport/sidecar/interceptor.go` around lines 86 - 91, After buffering the request body into bodyBytes, always assign req.GetBody to a function that returns io.NopCloser(bytes.NewReader(bodyBytes)) (i.e., unconditionally set req.GetBody rather than only when req.GetBody != nil) and set req.Body = io.NopCloser(bytes.NewReader(bodyBytes)); update the code around the existing req.Body and req.GetBody usage so GetBody is provided regardless of the original request, enabling http.Transport retries for signed sidecar requests while the frozen timestamp remains valid.
33-37: Doc nit: type isInterceptor, notSidecarInterceptor.The godoc references a symbol name that doesn't exist in the package; small inconsistency worth tidying.
Proposed fix
-// ResolveInterceptor returns a SidecarInterceptor when sidecar mode is active. +// ResolveInterceptor returns an Interceptor when sidecar mode is active.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/transport/sidecar/interceptor.go` around lines 33 - 37, The doc comment above ResolveInterceptor refers to a non-existent symbol "SidecarInterceptor"; update the godoc to reference the actual type name "Interceptor" (or the correct exported symbol if you intend a different name) so the comment matches the code; locate the ResolveInterceptor function and its associated Interceptor type and change the comment text to say "returns an Interceptor" (or the exact symbol) and adjust any surrounding wording to keep it consistent.
🤖 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/transport/sidecar/interceptor.go`:
- Around line 47-52: The code silently allows an empty LARKSUITE_CLI_PROXY_KEY
which produces zero-length HMACs that always fail; update ResolveInterceptor
(and the package init() for consistency) to check envvars.CliProxyKey after
reading it, and if it's empty log a warning and disable the interceptor instead
of returning an Interceptor with a zero-length key; reference the Interceptor
type and its construction (the key: []byte(key) and sidecar.ProxyHost(proxyAddr)
call) and ensure the same guard is applied in init() so sidecar.Sign cannot be
called with an empty key.
- Around line 120-122: PreRoundTrip in interceptor.go rewrites
req.URL.Scheme/Host to route via i.sidecarHost but doesn't clear req.Host, so
outgoing Host header may still be the original upstream host; after setting
req.URL.Scheme and req.URL.Host in PreRoundTrip, set req.Host = "" to force use
of req.URL.Host for the Host header, and add a unit test that sets req.Host to
the original host before calling PreRoundTrip (calling the
interceptor.PreRoundTrip) and asserts req.Host is cleared and the effective Host
header would be the sidecar host.
---
Nitpick comments:
In `@extension/transport/sidecar/interceptor.go`:
- Around line 86-91: After buffering the request body into bodyBytes, always
assign req.GetBody to a function that returns
io.NopCloser(bytes.NewReader(bodyBytes)) (i.e., unconditionally set req.GetBody
rather than only when req.GetBody != nil) and set req.Body =
io.NopCloser(bytes.NewReader(bodyBytes)); update the code around the existing
req.Body and req.GetBody usage so GetBody is provided regardless of the original
request, enabling http.Transport retries for signed sidecar requests while the
frozen timestamp remains valid.
- Around line 33-37: The doc comment above ResolveInterceptor refers to a
non-existent symbol "SidecarInterceptor"; update the godoc to reference the
actual type name "Interceptor" (or the correct exported symbol if you intend a
different name) so the comment matches the code; locate the ResolveInterceptor
function and its associated Interceptor type and change the comment text to say
"returns an Interceptor" (or the exact symbol) and adjust any surrounding
wording to keep it consistent.
🪄 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: 2468eb18-b853-436d-9a3b-973940530350
📒 Files selected for processing (2)
extension/transport/sidecar/interceptor.goextension/transport/sidecar/interceptor_test.go
✅ Files skipped from review due to trivial changes (1)
- extension/transport/sidecar/interceptor_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #532 +/- ##
==========================================
+ Coverage 60.19% 60.35% +0.15%
==========================================
Files 390 393 +3
Lines 33433 33588 +155
==========================================
+ Hits 20125 20272 +147
- Misses 11426 11430 +4
- Partials 1882 1886 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…nce demo
The sidecar server is no longer shipped as a `lark-cli auth proxy`
subcommand. Instead, the CLI provides only the standard sidecar *client*
(via `-tags authsidecar`), while the wire-protocol utilities are exposed
as a public package for integrators to implement their own server.
Changes:
- Move `internal/sidecar/` → `sidecar/` so external integrators can
import HMAC signing, headers, sentinels and address validators.
- Remove `cmd/auth/proxy.go`, `proxy_stub.go`, `proxy_test.go` and the
conditional registration in `cmd/auth/auth.go`.
- Add `sidecar/server-demo/` — a reference server implementation behind
the `authsidecar_demo` build tag. It reuses the lark-cli credential
pipeline for local development; production integrators are expected
to replace the credential layer with their own secrets source.
- Update all internal imports from `internal/sidecar` to `sidecar`.
Rationale:
- Each integrator has different secrets management / HA / multi-tenant
requirements, so a one-size-fits-all server doesn't belong in the
shipped CLI.
- Keeping the client in-tree guarantees all sandbox-side code stays
protocol-compatible without a second repo to sync.
- The public `sidecar/` package pins the wire protocol as a stable
contract third-party servers must conform to.
Build matrix after this change:
- `go build` → standard CLI, no sidecar code
- `go build -tags authsidecar` → CLI + sidecar client
- `go build -tags authsidecar_demo \
./sidecar/server-demo/` → reference server binary
No production users are affected today because the server was not yet
released; existing sidecar-client users are unchanged.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
sidecar/server-demo/allowlist.go (1)
31-43: Usecredential.SupportsUser/SupportsBotconstants instead of magic bits.The bitmask values
1and2are hardcoded here, but the source of truth isextension/credential/types.go(SupportsUser = 1 << iota,SupportsBot). If those constants are ever reordered or repurposed, this allowlist will silently misclassify identities — a security-relevant drift since this drives the server's identity enforcement. Also the two-stepswitch+ trailingifis harder to read than a direct bitmask check.♻️ Proposed refactor
import ( "strings" + "github.com/larksuite/cli/extension/credential" "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/sidecar" ) @@ func buildAllowedIdentities(cfg *core.CliConfig) map[string]bool { ids := make(map[string]bool) - switch { - case cfg.SupportedIdentities == 0: // unknown/unset → allow both - ids[sidecar.IdentityUser] = true - ids[sidecar.IdentityBot] = true - case cfg.SupportedIdentities&1 != 0: // SupportsUser bit - ids[sidecar.IdentityUser] = true - } - if cfg.SupportedIdentities == 0 || cfg.SupportedIdentities&2 != 0 { // SupportsBot bit - ids[sidecar.IdentityBot] = true - } + supported := cfg.SupportedIdentities + if supported == 0 { // unknown/unset → allow both + supported = credential.SupportsAll + } + if supported&credential.SupportsUser != 0 { + ids[sidecar.IdentityUser] = true + } + if supported&credential.SupportsBot != 0 { + ids[sidecar.IdentityBot] = true + } return ids }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sidecar/server-demo/allowlist.go` around lines 31 - 43, Replace the magic bit checks in buildAllowedIdentities so it uses the named constants credential.SupportsUser and credential.SupportsBot instead of 1 and 2 and simplify the flow to direct bitmask checks: check cfg.SupportedIdentities&credential.SupportsUser != 0 to set sidecar.IdentityUser and cfg.SupportedIdentities&credential.SupportsBot != 0 to set sidecar.IdentityBot, while retaining the current fallback behavior when SupportedIdentities == 0 (allow both); also add the credential import if missing and remove the switch + trailing if in favor of the straightforward boolean checks.sidecar/server-demo/handler_test.go (2)
167-185: Uset.Setenvfor automatic cleanup.
t.Setenvhandles save/restore and also marks the test as non-parallel, which is exactly the semantics this block is hand-rolling.func TestRun_RejectsSelfProxy(t *testing.T) { - old, had := os.LookupEnv(envvars.CliAuthProxy) - os.Setenv(envvars.CliAuthProxy, "http://127.0.0.1:16384") - defer func() { - if had { - os.Setenv(envvars.CliAuthProxy, old) - } else { - os.Unsetenv(envvars.CliAuthProxy) - } - }() + t.Setenv(envvars.CliAuthProxy, "http://127.0.0.1:16384")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sidecar/server-demo/handler_test.go` around lines 167 - 185, Replace the manual env save/restore in TestRun_RejectsSelfProxy with t.Setenv to simplify and ensure proper cleanup and test-safety: remove os.LookupEnv/os.Setenv/os.Unsetenv and the defer block, call t.Setenv(envvars.CliAuthProxy, "http://127.0.0.1:16384") at the top of the test, and keep the rest of the test (the run call and assertions) unchanged so the test still exercises run(...).
341-367: Anonymous struct test fixtures are brittle againstcore.Endpointsshape.
buildAllowedHoststakes...core.Endpoints. This test passes anonymous structs of typestruct{Open, Accounts, MCP string}. That only compiles whilecore.Endpointshas exactly those three fields (Go assignability across named/unnamed types with identical underlying types). The moment someone adds a new field (e.g.Passport,Doc, ...) tocore.Endpoints, this test fails to compile even though the production code is still correct.Import and use the real type for robustness:
Suggested refactor
- feishu := struct{ Open, Accounts, MCP string }{ - "https://open.feishu.cn", "https://accounts.feishu.cn", "https://mcp.feishu.cn", - } - lark := struct{ Open, Accounts, MCP string }{ - "https://open.larksuite.com", "https://accounts.larksuite.com", "https://mcp.larksuite.com", - } + feishu := core.Endpoints{Open: "https://open.feishu.cn", Accounts: "https://accounts.feishu.cn", MCP: "https://mcp.feishu.cn"} + lark := core.Endpoints{Open: "https://open.larksuite.com", Accounts: "https://accounts.larksuite.com", MCP: "https://mcp.larksuite.com"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sidecar/server-demo/handler_test.go` around lines 341 - 367, The test TestBuildAllowedHosts uses anonymous structs that mirror core.Endpoints and will break if core.Endpoints gains fields; change the fixtures to use the actual type core.Endpoints (import the core package) and construct feishu and lark as core.Endpoints values (setting the Open, Accounts, MCP fields explicitly) then pass them into buildAllowedHosts(...core.Endpoints) so the test remains robust to adding fields to core.Endpoints.sidecar/server-demo/main.go (1)
163-165: Prefererrors.Is(err, http.ErrServerClosed).Direct
!=comparison breaks if the error ever gets wrapped downstream.- if err := server.Serve(listener); err != nil && err != http.ErrServerClosed { + if err := server.Serve(listener); err != nil && !errors.Is(err, http.ErrServerClosed) { return fmt.Errorf("sidecar server exited unexpectedly: %v", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sidecar/server-demo/main.go` around lines 163 - 165, The current check uses a direct comparison against http.ErrServerClosed which fails for wrapped errors; change the condition in the server.Serve(listener) error handling to use errors.Is(err, http.ErrServerClosed) (e.g. if err := server.Serve(listener); err != nil && !errors.Is(err, http.ErrServerClosed) { return fmt.Errorf("sidecar server exited unexpectedly: %v", err) }) and add an import for the standard "errors" package if it's not already imported.sidecar/server-demo/forward.go (1)
40-51: RedundantCanonicalHeaderKeycalls on constants.Canonicalizing the
sidecar.Header*constants on every call is wasted work if they're already canonical (and if they aren't, the constants themselves should be). Canonicalize the input once and compare against the bare constants:♻️ Proposed simplification
func isProxyHeader(key string) bool { - switch http.CanonicalHeaderKey(key) { - case http.CanonicalHeaderKey(sidecar.HeaderProxyTarget), - http.CanonicalHeaderKey(sidecar.HeaderProxyIdentity), - http.CanonicalHeaderKey(sidecar.HeaderProxySignature), - http.CanonicalHeaderKey(sidecar.HeaderProxyTimestamp), - http.CanonicalHeaderKey(sidecar.HeaderBodySHA256), - http.CanonicalHeaderKey(sidecar.HeaderProxyAuthHeader): + switch http.CanonicalHeaderKey(key) { + case sidecar.HeaderProxyTarget, + sidecar.HeaderProxyIdentity, + sidecar.HeaderProxySignature, + sidecar.HeaderProxyTimestamp, + sidecar.HeaderBodySHA256, + sidecar.HeaderProxyAuthHeader: return true } return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sidecar/server-demo/forward.go` around lines 40 - 51, The isProxyHeader function is repeatedly calling http.CanonicalHeaderKey on the constants which is redundant; instead call http.CanonicalHeaderKey once on the incoming key (e.g., key = http.CanonicalHeaderKey(key)) and then compare that result directly against the bare sidecar.HeaderProxyTarget, sidecar.HeaderProxyIdentity, sidecar.HeaderProxySignature, sidecar.HeaderProxyTimestamp, sidecar.HeaderBodySHA256, and sidecar.HeaderProxyAuthHeader constants in the switch. Remove the CanonicalHeaderKey(...) wrappers around the constants so the function performs a single canonicalization of the input and direct constant comparisons.extension/credential/sidecar/provider.go (1)
72-83: Optional: collapse identical switch arms.The two accepting cases both do
acct.DefaultAs = id, so they can be merged into a single case with a fallthrough on valid values. Purely cosmetic.♻️ Proposed refactor
- // Parse DefaultAs - switch id := credential.Identity(os.Getenv(envvars.CliDefaultAs)); id { - case "", credential.IdentityAuto: - acct.DefaultAs = id - case credential.IdentityUser, credential.IdentityBot: - acct.DefaultAs = id - default: - return nil, &credential.BlockError{ - Provider: "sidecar", - Reason: fmt.Sprintf("invalid %s %q (want user, bot, or auto)", envvars.CliDefaultAs, id), - } - } + // Parse DefaultAs + switch id := credential.Identity(os.Getenv(envvars.CliDefaultAs)); id { + case "", credential.IdentityAuto, credential.IdentityUser, credential.IdentityBot: + acct.DefaultAs = id + default: + return nil, &credential.BlockError{ + Provider: "sidecar", + Reason: fmt.Sprintf("invalid %s %q (want user, bot, or auto)", envvars.CliDefaultAs, id), + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extension/credential/sidecar/provider.go` around lines 72 - 83, The switch on credential.Identity(os.Getenv(envvars.CliDefaultAs)) has duplicate arms that both set acct.DefaultAs = id; simplify by collapsing the identical arms: combine credential.IdentityUser and credential.IdentityBot (and possibly the empty/IdentityAuto case handled separately) into a single case that assigns acct.DefaultAs = id, keeping the default case that returns the credential.BlockError; update the switch around acct.DefaultAs, credential.Identity, envvars.CliDefaultAs, credential.IdentityUser, credential.IdentityBot, credential.IdentityAuto, and credential.BlockError accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sidecar/server-demo/allowlist.go`:
- Around line 17-27: The buildAllowedHosts function currently slices after "://"
which can leave trailing slashes and is fragile; update buildAllowedHosts to
parse each endpoint string with url.Parse (in function buildAllowedHosts) and
use parsed.Hostname() (or parsed.Host if you need port) as the map key, skipping
empty URLs and handling parse errors (log/ignore invalid entries) so entries
like "https://open.feishu.cn/" become "open.feishu.cn" consistently with
ProxyHost().
In `@sidecar/server-demo/handler.go`:
- Around line 128-170: The proxy currently copies hop-by-hop headers through in
both directions; add a helper (e.g., removeHopByHopHeaders(h http.Header)) that
removes the standard RFC7230 hop-by-hop fields
("Connection","Keep-Alive","Proxy-Authenticate","Proxy-Authorization","TE","Trailer","Transfer-Encoding","Upgrade")
and also removes any header names listed in h.Get("Connection") before
forwarding. Call this helper right after building forwardReq.Header (near the
loop using isProxyHeader) and again before copying resp.Header into w.Header
(around the resp.Header loop) so both request and response strip hop-by-hop
headers; keep references to isProxyHeader, forwardReq.Header, resp.Header and
the response-copy block so you can locate the spots to change.
In `@sidecar/server-demo/main.go`:
- Around line 54-59: The fallback in defaultKeyFile uses a predictable /tmp path
which is insecure on shared hosts; change defaultKeyFile to either refuse to run
when os.UserHomeDir() is empty or build the fallback from
os.UserCacheDir()/os.TempDir() plus a securely generated random subdirectory
(e.g., random suffix per process) and ensure any created directories use 0700;
update code that writes the key to validate and create the parent directory
securely (MkdirAll + Chmod/umask considerations) before writing the key. Also
update the run logic to reject an empty --key-file flag (validate in run) so
callers cannot force the old unsafe default. Refer to function names
defaultKeyFile and run when making these changes.
- Around line 79-98: Validate the user-supplied file paths before performing any
file I/O: call validate.SafeInputPath on keyFile and logFile (when non-empty)
and handle/return any validation errors prior to invoking vfs.MkdirAll,
vfs.WriteFile or vfs.OpenFile; ensure keyFile is validated before creating its
directory and writing the key, and validate logFile before opening/creating the
audit log file so only safe paths reach vfs.MkdirAll, vfs.WriteFile and
vfs.OpenFile.
In `@sidecar/server-demo/README.md`:
- Around line 86-96: The fenced code blocks in the README that show the auth
sidecar output and the HMAC header list are missing a language label causing
markdownlint MD040; update those triple-backtick blocks to include a language
(e.g., change ``` to ```text or ```console) for the blocks that contain "Auth
sidecar listening on http://127.0.0.1:16384" and the block listing "method host
pathAndQuery bodySHA256 timestamp" (also apply the same change to the similar
block around lines 158-164) so the renderer will treat them as plain text and
MD040 will be resolved.
---
Nitpick comments:
In `@extension/credential/sidecar/provider.go`:
- Around line 72-83: The switch on
credential.Identity(os.Getenv(envvars.CliDefaultAs)) has duplicate arms that
both set acct.DefaultAs = id; simplify by collapsing the identical arms: combine
credential.IdentityUser and credential.IdentityBot (and possibly the
empty/IdentityAuto case handled separately) into a single case that assigns
acct.DefaultAs = id, keeping the default case that returns the
credential.BlockError; update the switch around acct.DefaultAs,
credential.Identity, envvars.CliDefaultAs, credential.IdentityUser,
credential.IdentityBot, credential.IdentityAuto, and credential.BlockError
accordingly.
In `@sidecar/server-demo/allowlist.go`:
- Around line 31-43: Replace the magic bit checks in buildAllowedIdentities so
it uses the named constants credential.SupportsUser and credential.SupportsBot
instead of 1 and 2 and simplify the flow to direct bitmask checks: check
cfg.SupportedIdentities&credential.SupportsUser != 0 to set sidecar.IdentityUser
and cfg.SupportedIdentities&credential.SupportsBot != 0 to set
sidecar.IdentityBot, while retaining the current fallback behavior when
SupportedIdentities == 0 (allow both); also add the credential import if missing
and remove the switch + trailing if in favor of the straightforward boolean
checks.
In `@sidecar/server-demo/forward.go`:
- Around line 40-51: The isProxyHeader function is repeatedly calling
http.CanonicalHeaderKey on the constants which is redundant; instead call
http.CanonicalHeaderKey once on the incoming key (e.g., key =
http.CanonicalHeaderKey(key)) and then compare that result directly against the
bare sidecar.HeaderProxyTarget, sidecar.HeaderProxyIdentity,
sidecar.HeaderProxySignature, sidecar.HeaderProxyTimestamp,
sidecar.HeaderBodySHA256, and sidecar.HeaderProxyAuthHeader constants in the
switch. Remove the CanonicalHeaderKey(...) wrappers around the constants so the
function performs a single canonicalization of the input and direct constant
comparisons.
In `@sidecar/server-demo/handler_test.go`:
- Around line 167-185: Replace the manual env save/restore in
TestRun_RejectsSelfProxy with t.Setenv to simplify and ensure proper cleanup and
test-safety: remove os.LookupEnv/os.Setenv/os.Unsetenv and the defer block, call
t.Setenv(envvars.CliAuthProxy, "http://127.0.0.1:16384") at the top of the test,
and keep the rest of the test (the run call and assertions) unchanged so the
test still exercises run(...).
- Around line 341-367: The test TestBuildAllowedHosts uses anonymous structs
that mirror core.Endpoints and will break if core.Endpoints gains fields; change
the fixtures to use the actual type core.Endpoints (import the core package) and
construct feishu and lark as core.Endpoints values (setting the Open, Accounts,
MCP fields explicitly) then pass them into buildAllowedHosts(...core.Endpoints)
so the test remains robust to adding fields to core.Endpoints.
In `@sidecar/server-demo/main.go`:
- Around line 163-165: The current check uses a direct comparison against
http.ErrServerClosed which fails for wrapped errors; change the condition in the
server.Serve(listener) error handling to use errors.Is(err,
http.ErrServerClosed) (e.g. if err := server.Serve(listener); err != nil &&
!errors.Is(err, http.ErrServerClosed) { return fmt.Errorf("sidecar server exited
unexpectedly: %v", err) }) and add an import for the standard "errors" package
if it's not already imported.
🪄 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: e9fd6156-29ec-459f-b8b2-e9b8ee26e3b0
📒 Files selected for processing (15)
.gitignoreextension/credential/sidecar/provider.goextension/credential/sidecar/provider_test.goextension/transport/sidecar/interceptor.goextension/transport/sidecar/interceptor_test.gosidecar/hmac.gosidecar/hmac_test.gosidecar/protocol.gosidecar/server-demo/README.mdsidecar/server-demo/allowlist.gosidecar/server-demo/audit.gosidecar/server-demo/forward.gosidecar/server-demo/handler.gosidecar/server-demo/handler_test.gosidecar/server-demo/main.go
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- extension/credential/sidecar/provider_test.go
- Server: enforce https-only target (no path/query/userinfo), pin
forwardURL to https:// — blocks cleartext token leak
- Protocol v1: canonical now covers version/identity/auth-header,
blocks identity-flip replay within drift window
- Client: ValidateProxyAddr requires loopback or same-host alias,
rejects userinfo and https (interceptor is http-only); cross-machine
is out of scope
- Build: non-authsidecar builds exit(2) when AUTH_PROXY is set,
preventing silent fallback to env credentials
- Demo: whitelist auth-header to Authorization / X-Lark-MCP-{UAT,TAT},
blocks token injection into Cookie / UA / X-Forwarded-For exfil paths
github-actions
bot
added
size/XL
liangshuo-1
approved these changes
Apr 20, 2026
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an auth sidecar proxy mode that keeps real secrets (
app_secret, access tokens) out of sandbox environments. The CLI running inside a sandbox speaks a stable HTTP wireprotocol (HMAC-signed) to a trusted sidecar process, which injects the real credentials before forwarding to the Lark OpenAPI. The sidecar client is shipped as an opt-in build tag
(
authsidecar); the server side is intentionally not shipped as a subcommand — only the wire-protocol package and a reference demo server are published so each integrator canplug in their own secrets backend.
Changes
sidecar/package with the stable wire protocol: headers (X-Lark-Proxy-*,X-Lark-Body-SHA256), HMAC-SHA256 signing/verification, sentinel token constants,ValidateProxyAddr, andProxyHosthelpers.extension/credential/sidecar, build tagauthsidecar): returns a minimalAccountwithNoAppSecretand sentinel UAT/TAT tokens whenLARKSUITE_CLI_AUTH_PROXYis set; validatesAUTH_PROXY,APP_ID,PROXY_KEY,DEFAULT_AS,STRICT_MODE.extension/transport/sidecar, build tagauthsidecar): detects sentinel tokens onAuthorization/X-Lark-MCP-UAT/TAT, buffers the body(preserving
GetBodyfor retries), strips client-supplied auth headers, sets proxy/identity/body-digest headers, HMAC-signs the request, and rewrites the URL to the sidecar host.credential.Register(lowest wins, default10) so the sidecar provider (priority0) yields sentinels before env-based providers would resolvereal secrets.
LARKSUITE_CLI_AUTH_PROXY(sidecar HTTP address) andLARKSUITE_CLI_PROXY_KEY(shared HMAC key).main_authsidecar.goblank imports under theauthsidecarbuild tag.sidecar/server-demo/— reference server (build tagauthsidecar_demo) with allowlist, audit, forward, HMAC verification, and timestamp-drift checks; intended as a startingpoint, not a production binary.
Test Plan
sidecar/hmac_test.go,sidecar/server-demo/handler_test.go,extension/credential/sidecar/provider_test.go,extension/transport/sidecar/interceptor_test.go,extension/credential/registry_test.go(priority ordering).go build(standard CLI, no sidecar code) — unchanged behavior.go build -tags authsidecar— CLI + sidecar client.go build -tags authsidecar_demo ./sidecar/server-demo/— reference server binary.LARKSUITE_CLI_AUTH_PROXY=http://127.0.0.1:16384 LARKSUITE_CLI_PROXY_KEY=<key> lark im +send ...and confirm thesandbox-side process never sees the real
app_secretor access tokens, and the request reachesopen.feishu.cnwith the correct injectedAuthorizationheader.Related Issues