fix(qwen): un-exempt opencode, and keep the Qwen key in a secret
Two findings this round contradicted each other — one asked me to extend the engine-spec exemption to a bare "opencode", the other said opencode should not be exempt at all. The code settles it: that engine drives an ollama-cloud model through the bundled CLI and authenticates with OLLAMA_API_KEY, so it needs exactly the key the pre-flight checks. Exempting it, which I did last round, switched the check off for the one engine it could still help. Only claude-code is exempt now — it carries CLAUDE_CODE_OAUTH_TOKEN and needs no Ollama key — and opencode/open-code get table rows so both spellings are covered. The README told operators to embed the Qwen key in a GADFLY_ENDPOINT_* var, while the workflow that forwards those vars warns in its own comments that vars are NOT masked. Rather than only rewording the docs, a keyless kimi/qwen endpoint now falls back to its own QWEN_API_KEY / KIMI_API_KEY — the same vendor's key, so the no-cross-vendor rule is untouched — which lets the URL live in a var and the credential in a secret. Break-checked by pointing that fallback at OPENAI_API_KEY: the leak test catches it. Smaller: isBuiltinCompatProvider mirrors isOpenAICompatProvider instead of an inline slices.Contains, with a test that every builtin is also in the compat list (a builtin missing from it would never reach the branch that protects it); the preflight.sh rationale is stated once rather than in two comment blocks; the Go test locates the shell script relative to its own source file; and the gofmt step takes GOPROXY=off like its neighbours. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This commit is contained in:
+24
-1
@@ -44,6 +44,19 @@ func isOpenAICompatProvider(name string) bool {
|
||||
return slices.Contains(openAICompatProviders, name)
|
||||
}
|
||||
|
||||
// isBuiltinCompatProvider mirrors isOpenAICompatProvider rather than testing
|
||||
// the slice inline, so both memberships are asked the same way.
|
||||
func isBuiltinCompatProvider(name string) bool {
|
||||
return slices.Contains(builtinCompatProviders, name)
|
||||
}
|
||||
|
||||
// builtinCompatKeyEnv is the provider's own credential variable, matching the
|
||||
// name majordomo's built-in reads on the registry path — so the same secret
|
||||
// works whether or not an explicit endpoint is configured.
|
||||
func builtinCompatKeyEnv(provider string) string {
|
||||
return strings.ToUpper(strings.ReplaceAll(provider, "-", "_")) + "_API_KEY"
|
||||
}
|
||||
|
||||
// openAICompatOptions builds the option set for an openai-compat provider, and
|
||||
// is the ONE place the no-cross-vendor-fallback rule lives.
|
||||
//
|
||||
@@ -55,7 +68,17 @@ func isOpenAICompatProvider(name string) bool {
|
||||
func openAICompatOptions(provider, baseURL, key, keyHint string) []openai.Option {
|
||||
opts := []openai.Option{openai.WithBaseURL(baseURL)}
|
||||
switch {
|
||||
case slices.Contains(builtinCompatProviders, provider):
|
||||
case isBuiltinCompatProvider(provider):
|
||||
// With no explicit key, fall back to the provider's OWN variable
|
||||
// (QWEN_API_KEY, KIMI_API_KEY). That is not the cross-vendor fallback
|
||||
// this function exists to prevent — it is the same vendor's key — and
|
||||
// it lets an operator keep the credential in a masked secret while the
|
||||
// endpoint URL lives in a var, which is NOT masked.
|
||||
if key == "" {
|
||||
if own := os.Getenv(builtinCompatKeyEnv(provider)); own != "" {
|
||||
key, keyHint = own, builtinCompatKeyEnv(provider)
|
||||
}
|
||||
}
|
||||
opts = append(opts, openai.WithAPIKey(key), openai.WithAPIKeyName(keyHint))
|
||||
case key != "":
|
||||
opts = append(opts, openai.WithAPIKey(key))
|
||||
|
||||
@@ -6,6 +6,7 @@ import (
|
||||
"net/http/httptest"
|
||||
"os/exec"
|
||||
"path/filepath"
|
||||
"runtime"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
@@ -185,7 +186,13 @@ func TestOpenAICompatProvidersAreFullyWired(t *testing.T) {
|
||||
advertised[strings.TrimSpace(n)] = true
|
||||
}
|
||||
|
||||
script := filepath.Join("..", "..", "scripts", "preflight.sh")
|
||||
// Locate the script relative to THIS source file rather than the working
|
||||
// directory, so moving the package does not silently break the lookup.
|
||||
_, thisFile, _, ok := runtime.Caller(0)
|
||||
if !ok {
|
||||
t.Fatal("runtime.Caller failed; cannot locate scripts/preflight.sh")
|
||||
}
|
||||
script := filepath.Join(filepath.Dir(thisFile), "..", "..", "scripts", "preflight.sh")
|
||||
out, err := exec.Command("bash", "-c", ". "+script+"; gadfly_preflight_providers").Output()
|
||||
if err != nil {
|
||||
t.Fatalf("query gadfly_preflight_providers from %s: %v", script, err)
|
||||
@@ -317,3 +324,52 @@ func assertFailsClosed(t *testing.T, m llm.Model, seen *[]string, foreign, wantH
|
||||
t.Errorf("error = %v, want it to name %s so the operator knows what to set", err, wantHint)
|
||||
}
|
||||
}
|
||||
|
||||
// TestBuiltinCompatOwnKeyFallback: with no key in the endpoint definition, a
|
||||
// built-in falls back to its OWN variable (QWEN_API_KEY, KIMI_API_KEY) — never
|
||||
// to another vendor's. This is what lets the credential live in a masked
|
||||
// secret while the endpoint URL lives in a GADFLY_ENDPOINT_* var, which Gitea
|
||||
// does not mask; the README used to advise embedding the key in that var.
|
||||
func TestBuiltinCompatOwnKeyFallback(t *testing.T) {
|
||||
const own, foreign = "sk-qwen-own", "sk-openai-must-not-travel"
|
||||
|
||||
srv, seen := leakServer(t)
|
||||
t.Setenv("OPENAI_API_KEY", foreign)
|
||||
t.Setenv("QWEN_API_KEY", own)
|
||||
|
||||
p, err := endpointProvider("ep", "qwen|"+srv.URL+"/v1") // no key field
|
||||
if err != nil {
|
||||
t.Fatalf("endpointProvider: %v", err)
|
||||
}
|
||||
m, err := p.Model("some-model")
|
||||
if err != nil {
|
||||
t.Fatalf("Model: %v", err)
|
||||
}
|
||||
if _, err := m.Generate(context.Background(), llm.Request{Messages: []llm.Message{llm.UserText("hi")}}); err != nil {
|
||||
t.Fatalf("Generate: %v", err)
|
||||
}
|
||||
if len(*seen) == 0 {
|
||||
t.Fatal("no request reached the server — the own-key fallback did not take effect")
|
||||
}
|
||||
for _, auth := range *seen {
|
||||
if strings.Contains(auth, foreign) {
|
||||
t.Errorf("Authorization carried the OpenAI key: %q", auth)
|
||||
}
|
||||
if !strings.Contains(auth, own) {
|
||||
t.Errorf("Authorization = %q, want the provider's own QWEN_API_KEY", auth)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestBuiltinCompatProvidersAreOpenAICompat: the two slices are parallel, and a
|
||||
// built-in missing from openAICompatProviders would never reach the branch that
|
||||
// applies its unconditional-key rule — it would fall through to the generic
|
||||
// switch and silently lose the protection.
|
||||
func TestBuiltinCompatProvidersAreOpenAICompat(t *testing.T) {
|
||||
for _, p := range builtinCompatProviders {
|
||||
if !isOpenAICompatProvider(p) {
|
||||
t.Errorf("%q is in builtinCompatProviders but not openAICompatProviders, so the "+
|
||||
"no-cross-vendor-fallback branch never runs for it", p)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user