feat(core): allow overriding endpoint base URLs via env vars#1069
feat(core): allow overriding endpoint base URLs via env vars#1069wgzesg wants to merge 2 commits into
Conversation
|
|
📝 WalkthroughWalkthroughAdds exported env-var constants and helpers to let ResolveEndpoints apply per-field environment overrides for service base URLs, normalizing and validating inputs; includes tests for full/partial overrides, empty/invalid inputs (with warnings), path prefixes, and http scheme acceptance. ChangesEndpoint Configuration via Environment Variables
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
github-actions
Bot
added
the
size/M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/core/types_test.go (1)
56-102: ⚡ Quick winAdd malformed-override coverage to lock expected behavior.
These tests don’t yet cover invalid non-absolute override values. Adding cases like
"foo"and"https://"(expecting defaults to remain unchanged) will prevent regressions in endpoint safety/robustness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/types_test.go` around lines 56 - 102, Add tests that verify ResolveEndpoints ignores malformed non-absolute env overrides by extending or adding to TestResolveEndpoints_EnvOverride* cases: set EnvOpenBaseURL (and/or other Env* variables) to invalid values like "foo" and "https://" and assert the returned endpoints from ResolveEndpoints(BrandFeishu) and ResolveEndpoints(BrandLark) remain the respective defaults; use the existing test names (TestResolveEndpoints_EnvOverride, TestResolveEndpoints_EnvOverride_PartialKeepsBrandDefaults, TestResolveEndpoints_EnvOverride_EmptyIgnored) and reference ResolveEndpoints, EnvOpenBaseURL, EnvAccountsBaseURL, EnvMCPBaseURL, and EnvAppLinkBaseURL to locate where to add these malformed-value assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/core/types.go`:
- Around line 76-99: applyEndpointEnvOverrides currently accepts any non-empty
trimmed value; change it to validate that the normalized value is an absolute
URL before assigning into Endpoints (Open, Accounts, MCP, AppLink). For each env
var processed in applyEndpointEnvOverrides, call normalizeBaseURL then parse the
result with url.Parse (or net/url) and ensure parsed.Scheme and parsed.Host are
non-empty (and optionally reject non-http/https schemes); only assign to ep
fields when the URL is valid, otherwise leave the existing value unchanged (or
log a warning). Keep normalizeBaseURL behavior (trim + strip trailing slashes)
and reference the symbols applyEndpointEnvOverrides, normalizeBaseURL, and the
Endpoints fields (Open, Accounts, MCP, AppLink) when making the change.
---
Nitpick comments:
In `@internal/core/types_test.go`:
- Around line 56-102: Add tests that verify ResolveEndpoints ignores malformed
non-absolute env overrides by extending or adding to
TestResolveEndpoints_EnvOverride* cases: set EnvOpenBaseURL (and/or other Env*
variables) to invalid values like "foo" and "https://" and assert the returned
endpoints from ResolveEndpoints(BrandFeishu) and ResolveEndpoints(BrandLark)
remain the respective defaults; use the existing test names
(TestResolveEndpoints_EnvOverride,
TestResolveEndpoints_EnvOverride_PartialKeepsBrandDefaults,
TestResolveEndpoints_EnvOverride_EmptyIgnored) and reference ResolveEndpoints,
EnvOpenBaseURL, EnvAccountsBaseURL, EnvMCPBaseURL, and EnvAppLinkBaseURL to
locate where to add these malformed-value assertions.
🪄 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: 931d6023-10ff-4d4e-a370-26d02011e946
📒 Files selected for processing (2)
internal/core/types.gointernal/core/types_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/core/types.go`:
- Around line 116-127: isValidBaseURL currently treats inputs like "http://:80"
as valid and allows query/fragment parts; update it to use u.Hostname() to
ensure a non-empty hostname (reject hostless ":port"), and also reject any URL
that has non-empty u.RawQuery or u.Fragment. Keep the existing checks for
non-empty input, successful url.Parse, and scheme being "http" or "https", and
return false if any of these new conditions fail.
🪄 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: ad2b12ac-335e-4915-af66-0ee8ff3aead7
📒 Files selected for processing (2)
internal/core/types.gointernal/core/types_test.go
| // isValidBaseURL reports whether v is an absolute http/https URL with a host. | ||
| // A non-empty path is allowed (some deployments serve the API under a prefix). | ||
| func isValidBaseURL(v string) bool { | ||
| if v == "" { | ||
| return false | ||
| } | ||
| u, err := url.Parse(v) | ||
| if err != nil || u.Host == "" { | ||
| return false | ||
| } | ||
| return u.Scheme == "https" || u.Scheme == "http" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go's net/url package, what are the parsed fields (Scheme, Host, Hostname, RawQuery, Fragment) for these inputs: "http://:80", "https://example.com/base?x=1", and "https://example.com/base#frag"? Does URL.Hostname() return empty for ":80" hosts?
💡 Result:
For each input, assume you parse with url.Parse (net/url). The parsed fields you asked about are taken directly from the resulting url.URL struct: Scheme, Host, Hostname (via method), RawQuery, Fragment. The Hostname method is documented as returning u.Host with any valid port stripped, and it uses splitHostPort on u.Host [1][2]. 1) Input: "http://:80" - Scheme: "http" [1] - Host: ":80" [1][2] - Hostname: "" (empty) [2] - RawQuery: "" [1] - Fragment: "" [1] Does URL.Hostname return empty for ":80" hosts? Yes. For u.Host = ":80", splitHostPort finds the last ':' and if the trailing port is valid numeric, it splits host (prefix before ':') which is empty, and returns that empty host; the method then returns it [2]. This means Hostname is empty for the host string ":80". 2) Input: "https://example.com/base?x=1" - Scheme: "https" [1] - Host: "example.com" [1] - Hostname: "example.com" [1][2] - RawQuery: "x=1" [1] - Fragment: "" [1] 3) Input: "https://example.com/base#frag" - Scheme: "https" [1] - Host: "example.com" [1] - Hostname: "example.com" [1][2] - RawQuery: "" [1] - Fragment: "frag" [1]
Citations:
- 1: https://pkg.go.dev/net/url
- 2: net/url: mention net.JoinHostPort for constructing url.URL.Host golang/go#61093
Tighten base-URL validation to reject hostless “:port” and query/fragment values.
net/url keeps u.Host as ":80" for http://:80, but u.Hostname() is empty—so the current check (u.Host == "") incorrectly treats it as valid. The current code also allows bases containing ? / #, since it doesn’t reject RawQuery or Fragment.
🔧 Proposed fix
func isValidBaseURL(v string) bool {
if v == "" {
return false
}
u, err := url.Parse(v)
- if err != nil || u.Host == "" {
+ if err != nil || u.Host == "" || u.Hostname() == "" {
return false
}
- return u.Scheme == "https" || u.Scheme == "http"
+ if u.RawQuery != "" || u.Fragment != "" {
+ return false
+ }
+ return u.Scheme == "https" || u.Scheme == "http"
}📝 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.
| // isValidBaseURL reports whether v is an absolute http/https URL with a host. | |
| // A non-empty path is allowed (some deployments serve the API under a prefix). | |
| func isValidBaseURL(v string) bool { | |
| if v == "" { | |
| return false | |
| } | |
| u, err := url.Parse(v) | |
| if err != nil || u.Host == "" { | |
| return false | |
| } | |
| return u.Scheme == "https" || u.Scheme == "http" | |
| } | |
| // isValidBaseURL reports whether v is an absolute http/https URL with a host. | |
| // A non-empty path is allowed (some deployments serve the API under a prefix). | |
| func isValidBaseURL(v string) bool { | |
| if v == "" { | |
| return false | |
| } | |
| u, err := url.Parse(v) | |
| if err != nil || u.Host == "" || u.Hostname() == "" { | |
| return false | |
| } | |
| if u.RawQuery != "" || u.Fragment != "" { | |
| return false | |
| } | |
| return u.Scheme == "https" || u.Scheme == "http" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/core/types.go` around lines 116 - 127, isValidBaseURL currently
treats inputs like "http://:80" as valid and allows query/fragment parts; update
it to use u.Hostname() to ensure a non-empty hostname (reject hostless ":port"),
and also reject any URL that has non-empty u.RawQuery or u.Fragment. Keep the
existing checks for non-empty input, successful url.Parse, and scheme being
"http" or "https", and return false if any of these new conditions fail.
Add four optional environment variables that override the brand-default hosts returned by core.ResolveEndpoints, regardless of brand: LARKSUITE_CLI_OPEN_BASE_URL LARKSUITE_CLI_ACCOUNTS_BASE_URL LARKSUITE_CLI_MCP_BASE_URL LARKSUITE_CLI_APPLINK_BASE_URL Values are trimmed and have trailing slashes stripped; empty or whitespace-only values are ignored. Every existing call site already routes through ResolveEndpoints, so OAuth device flow, app registration, console scope links, consumer URLs, doctor checks, and credential resolution all pick up the override automatically. Public repo ships only the override mechanism — no internal hostnames. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously applyEndpointEnvOverrides accepted any non-empty value, even
though downstream code concatenates these bases to path segments (e.g.
ep.Open + "/open-apis/..."). A malformed value like "fsopen.bytedance.net"
(missing scheme) would produce invalid request URLs deep in the SDK.
- Require scheme in {http, https} and a non-empty host (path prefix
allowed, since some deployments serve the API under a path).
- Warn on stderr when an override is set but invalid, then fall back to
the brand default. Silent fallback would be dangerous: a typo'd inner
host would route credentials to the public host without the user
realizing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
wgzesg
force-pushed
the
feat/configurable-endpoint-base-urls
branch
from
f978eb7 to
cf42816
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/core/types.go (1)
116-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject hostless and query/fragment base URLs in validator.
Line 123 only checks
u.Host, so values likehttp://:80can pass; queries/fragments also pass today and can break downstream URL concatenation (internal/client/client.go:207-233).🔧 Proposed fix
func isValidBaseURL(v string) bool { if v == "" { return false } u, err := url.Parse(v) - if err != nil || u.Host == "" { + if err != nil || u.Host == "" || u.Hostname() == "" { + return false + } + if u.RawQuery != "" || u.Fragment != "" { return false } return u.Scheme == "https" || u.Scheme == "http" }#!/bin/bash set -euo pipefail # Verify current validator conditions in code. sed -n '116,130p' internal/core/types.go | cat -n # Verify whether edge-case tests are present. rg -n 'http://:80|\?x=|`#frag`' internal/core/types_test.go -S || true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/types.go` around lines 116 - 127, The validator is too permissive: isValidBaseURL currently only checks u.Host and allows hostless forms like "http://:80" and URLs with queries/fragments; update isValidBaseURL to also require a non-empty hostname (use u.Hostname() != "") and reject any URL that contains a RawQuery or Fragment (u.RawQuery == "" and u.Fragment == ""), while keeping the existing scheme check for "http"/"https" so downstream concatenation won't break.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@internal/core/types.go`:
- Around line 116-127: The validator is too permissive: isValidBaseURL currently
only checks u.Host and allows hostless forms like "http://:80" and URLs with
queries/fragments; update isValidBaseURL to also require a non-empty hostname
(use u.Hostname() != "") and reject any URL that contains a RawQuery or Fragment
(u.RawQuery == "" and u.Fragment == ""), while keeping the existing scheme check
for "http"/"https" so downstream concatenation won't break.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ce27e75-4259-4913-986a-5f38ca656261
📒 Files selected for processing (2)
internal/core/types.gointernal/core/types_test.go
2 participants
Add four optional environment variables that override the brand-default hosts returned by core.ResolveEndpoints, regardless of brand:
LARKSUITE_CLI_OPEN_BASE_URL
LARKSUITE_CLI_ACCOUNTS_BASE_URL
LARKSUITE_CLI_MCP_BASE_URL
LARKSUITE_CLI_APPLINK_BASE_URL
Values are trimmed and have trailing slashes stripped; empty or whitespace-only values are ignored. Every existing call site already routes through ResolveEndpoints, so OAuth device flow, app registration, console scope links, consumer URLs, doctor checks, and credential resolution all pick up the override automatically.
Public repo ships only the override mechanism — no internal hostnames.
Summary
Changes
Test Plan
lark xxxcommand works as expectedRelated Issues
Summary by CodeRabbit
New Features
Tests