feat[Go]: implement Langfuse API key management endpoints#15677
Open
hunnyboy1217 wants to merge 2 commits into
Open
feat[Go]: implement Langfuse API key management endpoints#15677hunnyboy1217 wants to merge 2 commits into
hunnyboy1217 wants to merge 2 commits into
Conversation
dosubot
Bot
added
the
size:XL
Contributor
📝 WalkthroughWalkthroughAdds tenant-scoped Langfuse credential management: GORM model and DAO, LangfuseService with external Basic-Auth validation and SSRF protections, Gin handlers for set/get/delete, and router endpoints at /api/v1/langfuse/api-key. ChangesLangfuse API Credential Management
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant Service
participant LangfuseAPI
participant DAO
participant DB
Client->>Handler: POST/PUT /api/v1/langfuse/api-key
Handler->>Service: SetAPIKey(tenantID, req)
Service->>LangfuseAPI: Basic-Auth GET /api/public/projects (auth-check)
LangfuseAPI-->>Service: 2xx or error
Service->>DAO: UpsertByTenantID
DAO->>DB: INSERT/UPDATE tenant_langfuse
DB-->>DAO: OK/error
Service-->>Handler: response (metadata without secret)
Handler-->>Client: JSON success/error
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. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/entity/langfuse.go`:
- Line 23: Change the TenantLangfuse.SecretKey struct tag so it is not output
during default JSON marshaling: replace json:"secret_key" with json:"-" on the
SecretKey field in the TenantLangfuse struct; if you still need to expose the
secret in controlled contexts, add an explicit accessor method (e.g.,
GetSecretKey()) or implement a custom marshaler rather than relying on the
struct tag.
In `@internal/service/langfuse.go`:
- Around line 67-76: The SetAPIKey flow currently trusts the user-controlled
host (used later in langfuseAuthCheck and outbound HTTP calls), so add a strict
host validator function (e.g., validateLangfuseHost or isSafeLangfuseHost) and
call it early in SetAPIKey before any network activity and reuse it in outbound
helpers; the validator must enforce allowed scheme (https only), normalize/parse
the host, resolve DNS and reject IPs that are private/loopback/link-local or
resolve to internal ranges, and reject suspicious hostnames (e.g., raw IPs or
local names) so SSRF/internal access is prevented when calling langfuseAuthCheck
and other functions that use req.Host.
- Around line 78-103: The current read-then-create/update in SetAPIKey (uses
s.langfuseDAO.GetByTenantID, Create, UpdateByTenantID) is racy; replace it with
an atomic upsert either by adding a DAO method like UpsertByTenantID that issues
a single DB operation (use GORM's Clauses.OnConflict/DoUpdate or a transaction
with INSERT ... ON CONFLICT) to set secret_key, public_key, host and tenant_id
(ensure tenant_id has a unique constraint), or perform the upsert inside a DB
transaction in the service layer; update the call in langfuse.go to call that
new atomic Upsert method instead of the read-then-create/update flow.
- Around line 183-185: In langfuseAuthCheck, replace the unbounded drain
(io.Copy(io.Discard, resp.Body)) with a bounded drain to avoid resource
exhaustion; use an io.LimitReader or io.CopyN with a defined maxDrainBytes
constant (e.g., 64KB or a reasonable limit) so only up to that many bytes are
read from resp.Body before closing, and optionally note/truncate if the body was
larger—keep the defer resp.Body.Close() and ensure the new bounded reader is
used in the io.Copy call.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce488816-dca8-4d6a-abc1-2073f3d7c6e6
📒 Files selected for processing (5)
internal/dao/langfuse.gointernal/entity/langfuse.gointernal/handler/langfuse.gointernal/router/router.gointernal/service/langfuse.go
Comment on lines
+67
to
+76
| func (s *LangfuseService) SetAPIKey(tenantID string, req *SetAPIKeyRequest) (map[string]interface{}, error) { | ||
| if strings.TrimSpace(req.SecretKey) == "" || | ||
| strings.TrimSpace(req.PublicKey) == "" || | ||
| strings.TrimSpace(req.Host) == "" { | ||
| return nil, fmt.Errorf("Missing required fields") | ||
| } | ||
|
|
||
| if err := langfuseAuthCheck(req.PublicKey, req.SecretKey, req.Host); err != nil { | ||
| return nil, fmt.Errorf("Invalid Langfuse keys") | ||
| } |
Contributor
There was a problem hiding this comment.
Validate and constrain host before any outbound request.
host is user-controlled and used directly for server-side HTTP calls. This enables SSRF against internal network targets unless the host is constrained (scheme, hostname, and private/loopback/link-local resolution checks).
Suggested hardening direction
func (s *LangfuseService) SetAPIKey(tenantID string, req *SetAPIKeyRequest) (map[string]interface{}, error) {
+ if err := validateLangfuseHost(req.Host); err != nil {
+ return nil, fmt.Errorf("Invalid host")
+ }
if strings.TrimSpace(req.SecretKey) == "" ||
strings.TrimSpace(req.PublicKey) == "" ||
strings.TrimSpace(req.Host) == "" {
return nil, fmt.Errorf("Missing required fields")
}// Add a strict validator (scheme + hostname + DNS/IP safety checks)
// and reuse it in both SetAPIKey and outbound helpers for defense in depth.Also applies to: 166-169, 193-196
🤖 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/service/langfuse.go` around lines 67 - 76, The SetAPIKey flow
currently trusts the user-controlled host (used later in langfuseAuthCheck and
outbound HTTP calls), so add a strict host validator function (e.g.,
validateLangfuseHost or isSafeLangfuseHost) and call it early in SetAPIKey
before any network activity and reuse it in outbound helpers; the validator must
enforce allowed scheme (https only), normalize/parse the host, resolve DNS and
reject IPs that are private/loopback/link-local or resolve to internal ranges,
and reject suspicious hostnames (e.g., raw IPs or local names) so SSRF/internal
access is prevented when calling langfuseAuthCheck and other functions that use
req.Host.
Ports three Langfuse credential endpoints from Python (langfuse_api.py) to Go, closing issue infiniflow#15675: POST/PUT /api/v1/langfuse/api-key (LangfuseHandler.SetAPIKey) GET /api/v1/langfuse/api-key (LangfuseHandler.GetAPIKey) DELETE /api/v1/langfuse/api-key (LangfuseHandler.DeleteAPIKey) New files: internal/entity/langfuse.go – TenantLangfuse entity (tenant_langfuse table) internal/dao/langfuse.go – LangfuseDAO: GetByTenantID, Create, UpdateByTenantID, DeleteByTenantID internal/service/langfuse.go – LangfuseService: SetAPIKey, GetAPIKey, DeleteAPIKey; langfuseAuthCheck and langfuseGetProjects HTTP helpers internal/handler/langfuse.go – LangfuseHandler with three HTTP handlers Changed files: internal/router/router.go – Router struct gains langfuseHandler; NewRouter constructs it; four routes added Functional parity with Python: SetAPIKey – validates all three fields; calls GET {host}/api/public/projects with Basic Auth (mirrors langfuse.auth_check()); upserts record; secret_key not echoed in response GetAPIKey – validates stored keys; fetches project list from Langfuse; returns public_key, host, project_id, project_name; secret_key intentionally excluded from response DeleteAPIKey – returns "Have not record" message when no keys stored, otherwise hard-deletes and returns true
hunnyboy1217
force-pushed
the
feat/go-langfuse-api
branch
from
68e131c to
b70d600
Compare
dosubot
Bot
added
size:L
Contributor
Author
|
Hi, @Haruko386 , |
Applies CodeRabbit's final-review feedback. 1. Secret key leakage (entity/langfuse.go): TenantLangfuse.SecretKey is now tagged json:"-" so it can never leak through any path that marshals the entity directly. 2. SSRF host guard (service/langfuse.go): the user-supplied Langfuse host is now validated (http/https scheme + hostname) and all outbound calls go through an HTTP client whose dialer resolves the host, rejects any non-globally-routable IP (loopback/private/link-local/multicast/unspecified/CGNAT), and pins the validated address — closing the authenticated-SSRF / DNS-rebinding vector. Note: this rejects self-hosted Langfuse on private networks (a deliberate security/usability tradeoff; relax isGloballyRoutableIP if internal hosts must be allowed). 3. Upsert race (dao + service): SetAPIKey's read-then-create/update is replaced by a single atomic LangfuseDAO.UpsertByTenantID using GORM clause.OnConflict on the tenant_id unique index, removing the concurrent-write race. 4. Unbounded drain (service/langfuse.go): langfuseAuthCheck now drains the response through io.LimitReader (1 MB cap) instead of an unbounded io.Copy(io.Discard, resp.Body); the outbound client also carries an explicit timeout.
dosubot
Bot
added
size:XL
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/service/langfuse.go (1)
113-120: ⚡ Quick winRemove the redundant auth-check call in
GetAPIKey.Line 113 does
langfuseAuthCheck, then Line 117 callslangfuseGetProjectsagainst the same endpoint with the same credentials. The second call already validates auth; removing the first cuts external calls and latency.Suggested simplification
- if err := langfuseAuthCheck(entry.PublicKey, entry.SecretKey, entry.Host); err != nil { - return nil, fmt.Errorf("Invalid Langfuse keys loaded") - } - projects, err := langfuseGetProjects(entry.PublicKey, entry.SecretKey, entry.Host) if err != nil { - return nil, fmt.Errorf("Error from Langfuse: %w", err) + return nil, fmt.Errorf("Invalid Langfuse keys loaded") }🤖 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/service/langfuse.go` around lines 113 - 120, Remove the redundant authentication call in GetAPIKey: the explicit call to langfuseAuthCheck(entry.PublicKey, entry.SecretKey, entry.Host) is unnecessary because langfuseGetProjects(entry.PublicKey, entry.SecretKey, entry.Host) performs the same auth validation; delete the langfuseAuthCheck call and its error branch so the function only calls langfuseGetProjects and handles its error/response.
🤖 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/service/langfuse.go`:
- Around line 236-238: The URL scheme check in the Langfuse host validator (the
u.Scheme conditional in internal/service/langfuse.go) currently permits "http"
and "https" which can leak Basic Auth credentials; change the logic to only
accept "https" and return an error if u.Scheme != "https". If you need to allow
"http" for local/dev testing, gate that exception behind an explicit, clearly
named opt-in (e.g. an environment flag like ALLOW_INSECURE_LANGFUSE or a
function IsDevAllowlist) and document/validate that flag before permitting
"http".
- Around line 263-276: The custom dialer closure currently pins the first
resolved IP (variable pinned) and returns it, causing failures if that IP is
down; instead iterate the resolved ips slice, skip any failing
isGloballyRoutableIP checks, and for each valid ip call dialer.DialContext(ctx,
network, net.JoinHostPort(ip.String(), port)) sequentially returning the first
successful connection; if all attempts fail, return a combined or last error and
if no addresses were valid return the existing "host resolved to no addresses"
error—update the DialContext closure logic that references pinned, ips,
isGloballyRoutableIP, and dialer.DialContext accordingly.
---
Nitpick comments:
In `@internal/service/langfuse.go`:
- Around line 113-120: Remove the redundant authentication call in GetAPIKey:
the explicit call to langfuseAuthCheck(entry.PublicKey, entry.SecretKey,
entry.Host) is unnecessary because langfuseGetProjects(entry.PublicKey,
entry.SecretKey, entry.Host) performs the same auth validation; delete the
langfuseAuthCheck call and its error branch so the function only calls
langfuseGetProjects and handles its error/response.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 50c2a439-5d69-4d6a-a040-3feb0226e28a
📒 Files selected for processing (3)
internal/dao/langfuse.gointernal/entity/langfuse.gointernal/service/langfuse.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/entity/langfuse.go
Comment on lines
+236
to
+238
| if u.Scheme != "http" && u.Scheme != "https" { | ||
| return fmt.Errorf("Langfuse host must use http or https") | ||
| } |
Contributor
There was a problem hiding this comment.
Require HTTPS for Langfuse host validation.
Line 236 currently allows http, but this flow sends Basic Auth credentials and can leak public_key:secret_key in transit. Restrict to https (or gate http behind an explicit non-production allowlist).
Suggested fix
- if u.Scheme != "http" && u.Scheme != "https" {
- return fmt.Errorf("Langfuse host must use http or https")
+ if u.Scheme != "https" {
+ return fmt.Errorf("Langfuse host must use https")
}📝 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
| if u.Scheme != "http" && u.Scheme != "https" { | |
| return fmt.Errorf("Langfuse host must use http or https") | |
| } | |
| if u.Scheme != "https" { | |
| return fmt.Errorf("Langfuse host must use https") | |
| } |
🤖 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/service/langfuse.go` around lines 236 - 238, The URL scheme check in
the Langfuse host validator (the u.Scheme conditional in
internal/service/langfuse.go) currently permits "http" and "https" which can
leak Basic Auth credentials; change the logic to only accept "https" and return
an error if u.Scheme != "https". If you need to allow "http" for local/dev
testing, gate that exception behind an explicit, clearly named opt-in (e.g. an
environment flag like ALLOW_INSECURE_LANGFUSE or a function IsDevAllowlist) and
document/validate that flag before permitting "http".
Comment on lines
+263
to
+276
| var pinned net.IP | ||
| for _, ip := range ips { | ||
| if !isGloballyRoutableIP(ip) { | ||
| return nil, fmt.Errorf("Langfuse host resolves to a non-public address (%s)", ip) | ||
| } | ||
| if pinned == nil { | ||
| pinned = ip | ||
| } | ||
| } | ||
| if pinned == nil { | ||
| return nil, fmt.Errorf("host %q resolved to no addresses", host) | ||
| } | ||
| return dialer.DialContext(ctx, network, net.JoinHostPort(pinned.String(), port)) | ||
| }, |
Contributor
There was a problem hiding this comment.
Avoid single-IP pinning in the custom dialer.
Line 268 pins only the first resolved IP. If that IP is down but others are healthy, requests fail unnecessarily. Try validated IPs sequentially before returning failure.
Suggested fix
- var pinned net.IP
- for _, ip := range ips {
+ for _, ip := range ips {
if !isGloballyRoutableIP(ip) {
return nil, fmt.Errorf("Langfuse host resolves to a non-public address (%s)", ip)
}
- if pinned == nil {
- pinned = ip
- }
}
- if pinned == nil {
+ if len(ips) == 0 {
return nil, fmt.Errorf("host %q resolved to no addresses", host)
}
- return dialer.DialContext(ctx, network, net.JoinHostPort(pinned.String(), port))
+ var lastErr error
+ for _, ip := range ips {
+ conn, err := dialer.DialContext(ctx, network, net.JoinHostPort(ip.String(), port))
+ if err == nil {
+ return conn, nil
+ }
+ lastErr = err
+ }
+ return nil, fmt.Errorf("could not connect to resolved addresses for %q: %w", host, lastErr)🤖 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/service/langfuse.go` around lines 263 - 276, The custom dialer
closure currently pins the first resolved IP (variable pinned) and returns it,
causing failures if that IP is down; instead iterate the resolved ips slice,
skip any failing isGloballyRoutableIP checks, and for each valid ip call
dialer.DialContext(ctx, network, net.JoinHostPort(ip.String(), port))
sequentially returning the first successful connection; if all attempts fail,
return a combined or last error and if no addresses were valid return the
existing "host resolved to no addresses" error—update the DialContext closure
logic that references pinned, ips, isGloballyRoutableIP, and dialer.DialContext
accordingly.
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.
What problem does this PR solve?
Closes #15675 — ports three Langfuse credential-management endpoints from Python (
api/apps/restful_apis/langfuse_api.py) to Go, following the layered architecture specified in the issue./api/v1/langfuse/api-keyLangfuseHandler.SetAPIKey/api/v1/langfuse/api-keyLangfuseHandler.GetAPIKey/api/v1/langfuse/api-keyLangfuseHandler.DeleteAPIKeyType of change
Implementation notes
Files created (per the architecture in the issue):
Functional parity table:
secret_key,public_key,hostall checked; missing any →"Missing required fields"— mirrors Python@validate_request+ explicit checklangfuseAuthCheck:GET {host}/api/public/projectswith Basic Auth (public_key:secret_key), 7 s timeout — mirrorslangfuse.auth_check()LangfuseDAO.GetByTenantID→ Create or UpdateByTenantID — mirrors Pythonfilter_by_tenant+save/update_by_tenanttenant_id,public_key,host— secret never echoedpublic_key,host,project_id,project_name;secret_keyintentionally excluded — satisfies issue requirementlangfuseGetProjects: sameGET /api/public/projectscall, parsesdata[0].idanddata[0].name— mirrors Pythonlangfuse.api.projects.get()"Have not record any Langfuse keys."— exact Python messageLangfuseDAO.DeleteByTenantID— mirrorsTenantLangfuseService.delete_modelauthorizedgroup (same as all other v1 routes)