diff --git a/.gitea/workflows/build-image.yml b/.gitea/workflows/build-image.yml index ff62f3a..5ffc8a4 100644 --- a/.gitea/workflows/build-image.yml +++ b/.gitea/workflows/build-image.yml @@ -53,6 +53,10 @@ jobs: test: runs-on: ubuntu-latest timeout-minutes: 15 + # This job executes repository code (`go test`) on pull_request, so it gets + # the narrowest token the platform will give it. Nothing here writes. + permissions: + contents: read steps: - uses: actions/checkout@v4 with: diff --git a/cmd/gadfly/model.go b/cmd/gadfly/model.go index 8dbd281..389623b 100644 --- a/cmd/gadfly/model.go +++ b/cmd/gadfly/model.go @@ -74,12 +74,15 @@ func openAICompatOptions(provider, baseURL, key, keyHint string) []openai.Option // 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. + // + // The hint always names that secret, never the caller's keyHint: on the + // GADFLY_ENDPOINT_* path the caller's is the endpoint variable, and + // pointing a keyless operator at it advises them to put a credential + // somewhere Gitea does not mask. if key == "" { - if own := os.Getenv(builtinCompatKeyEnv(provider)); own != "" { - key, keyHint = own, builtinCompatKeyEnv(provider) - } + key = os.Getenv(builtinCompatKeyEnv(provider)) } - opts = append(opts, openai.WithAPIKey(key), openai.WithAPIKeyName(keyHint)) + opts = append(opts, openai.WithAPIKey(key), openai.WithAPIKeyName(builtinCompatKeyEnv(provider))) case key != "": opts = append(opts, openai.WithAPIKey(key)) // openai/openai-compatible with no explicit key keep openai.New's diff --git a/cmd/gadfly/model_test.go b/cmd/gadfly/model_test.go index 43db0ef..8c57826 100644 --- a/cmd/gadfly/model_test.go +++ b/cmd/gadfly/model_test.go @@ -269,7 +269,7 @@ func TestBuiltinCompatProvidersNeverInheritOpenAIKey(t *testing.T) { if err != nil { t.Fatalf("resolveModel: %v", err) } - assertFailsClosed(t, m, seen, foreign, "GADFLY_API_KEY") + assertFailsClosed(t, m, seen, foreign, "QWEN_API_KEY", "KIMI_API_KEY") }) t.Run(provider+" via GADFLY_ENDPOINT_*", func(t *testing.T) { @@ -283,7 +283,7 @@ func TestBuiltinCompatProvidersNeverInheritOpenAIKey(t *testing.T) { if err != nil { t.Fatalf("Model: %v", err) } - assertFailsClosed(t, m, seen, foreign, "GADFLY_ENDPOINT_EP") + assertFailsClosed(t, m, seen, foreign, "QWEN_API_KEY", "KIMI_API_KEY") }) } } @@ -303,7 +303,7 @@ func leakServer(t *testing.T) (*httptest.Server, *[]string) { return srv, &seen } -func assertFailsClosed(t *testing.T, m llm.Model, seen *[]string, foreign, wantHint string) { +func assertFailsClosed(t *testing.T, m llm.Model, seen *[]string, foreign string, wantAnyHint ...string) { t.Helper() _, err := m.Generate(context.Background(), llm.Request{Messages: []llm.Message{llm.UserText("hi")}}) @@ -313,15 +313,23 @@ func assertFailsClosed(t *testing.T, m llm.Model, seen *[]string, foreign, wantH } } if len(*seen) > 0 { - t.Errorf("a keyless %s provider reached the network (%d request(s)) instead of failing closed", wantHint, len(*seen)) + t.Errorf("a keyless provider reached the network (%d request(s)) instead of failing closed", len(*seen)) } // The positive half: prove it refused for the right reason, so the test // cannot pass on a provider that quietly did nothing at all. if err == nil { t.Fatal("keyless provider returned no error; expected a missing-key failure") } - if !strings.Contains(err.Error(), wantHint) { - t.Errorf("error = %v, want it to name %s so the operator knows what to set", err, wantHint) + // The hint must name a MASKED secret the operator can set, never the + // unmasked GADFLY_ENDPOINT_* variable. + named := false + for _, h := range wantAnyHint { + if strings.Contains(err.Error(), h) { + named = true + } + } + if !named { + t.Errorf("error = %v, want it to name one of %v so the operator knows what to set", err, wantAnyHint) } } diff --git a/scripts/preflight.sh b/scripts/preflight.sh index 08f2312..e323a23 100755 --- a/scripts/preflight.sh +++ b/scripts/preflight.sh @@ -36,28 +36,42 @@ gadfly_preflight_key() { # needs exactly the key the table checks. Exempting it — which an earlier # version of this guard did — turns the pre-flight off for the one engine # whose missing key it could still catch. + model="$(printf '%s' "$model" | tr -d '[:space:]')" # Go trims GADFLY_MODEL case "$model" in claude-code|claude-code/*) echo ""; return 0 ;; esac - # Only the registry path has knowable credential rules — see above. # Trim before testing: resolveModel does strings.TrimSpace on GADFLY_BASE_URL, # so a whitespace-only value takes the REGISTRY path there. Testing the raw # value here would call it "set", skip the check, and let the missing key # arrive as a 401 with no notice — the two must agree on what "unset" means. local base_url base_url="$(printf '%s' "${GADFLY_BASE_URL:-}" | tr -d '[:space:]')" + if [ -n "$base_url" ]; then - echo "" + # Endpoint-override path. Most providers take their credential from + # GADFLY_API_KEY here with a client-specific fallback, and those rules are + # not worth restating — this stays silent for them. + # + # The built-ins are the exception, and only since they gained an own-key + # fallback: a keyless kimi/qwen endpoint reads QWEN_API_KEY / KIMI_API_KEY + # on THIS path too, so "own key or GADFLY_API_KEY" is a rule that can be + # stated exactly. Leaving them unchecked here would let a keyless override + # config sail past the pre-flight and fail as a 401 — the failure the + # pre-flight exists to replace. + case "$provider" in + qwen|kimi) ;; + *) echo ""; return 0 ;; + esac + local own_env="$(printf '%s' "$provider" | tr '[:lower:]-' '[:upper:]_')_API_KEY" + if [ -n "${!own_env:-}" ] || [ -n "${GADFLY_API_KEY:-}" ]; then + echo "" + else + echo "$own_env" + fi return 0 fi - # ollama-cloud is checked on OLLAMA_API_KEY but hinted as OLLAMA_CLOUD_API_KEY: - # run.sh copies the consumer-facing OLLAMA_CLOUD_API_KEY secret into the - # OLLAMA_API_KEY the provider reads, BEFORE calling this. The hint names the - # variable the operator actually sets; the check reads the one the code uses. - # If that copy ever moves after this call, this arm reports a missing key for - # a configured run. local row row="$(_gadfly_preflight_table | awk -F: -v p="$provider" '$1 == p {print; exit}')" if [ -z "$row" ]; then diff --git a/scripts/preflight_test.sh b/scripts/preflight_test.sh index c3093bc..64e6ef6 100755 --- a/scripts/preflight_test.sh +++ b/scripts/preflight_test.sh @@ -60,15 +60,22 @@ echo "== GADFLY_API_KEY does NOT substitute on the registry path ==" # GADFLY_API_KEY changes nothing. Treating it as sufficient was a false pass. check "qwen w/ GADFLY_API_KEY only" "QWEN_API_KEY" "$(probe qwen GADFLY_API_KEY=k)" -echo "== override path (GADFLY_BASE_URL set) is deliberately not pre-flighted ==" +echo "== override path: built-ins ARE checked; others are not ==" # The credential there is GADFLY_API_KEY with a client-specific fallback, and # the built-ins' own variables are never read. Checking one path's rules # against the other produced a false pass in BOTH directions, so this path is # left alone rather than guessed at. -check "qwen + BASE_URL, no keys" "" "$(probe qwen GADFLY_BASE_URL=https://x)" -check "qwen + BASE_URL + own key" "" "$(probe qwen GADFLY_BASE_URL=https://x QWEN_API_KEY=k)" +# A built-in reads its own key on the override path too (openAICompatOptions +# falls back to QWEN_API_KEY/KIMI_API_KEY there), so "own key or GADFLY_API_KEY" +# is statable and worth checking — leaving it unchecked let a keyless config +# sail past and fail as a 401. +check "qwen + BASE_URL, no keys" "QWEN_API_KEY" "$(probe qwen GADFLY_BASE_URL=https://x)" +check "qwen + BASE_URL + own key" "" "$(probe qwen GADFLY_BASE_URL=https://x QWEN_API_KEY=k)" check "qwen + BASE_URL + GADFLY key" "" "$(probe qwen GADFLY_BASE_URL=https://x GADFLY_API_KEY=k)" -check "openai + BASE_URL, no keys" "" "$(probe openai GADFLY_BASE_URL=https://x)" +check "kimi + BASE_URL, no keys" "KIMI_API_KEY" "$(probe kimi GADFLY_BASE_URL=https://x)" +# Other providers' override-path rules are not statable, so this stays quiet. +check "openai + BASE_URL, no keys" "" "$(probe openai GADFLY_BASE_URL=https://x)" +check "anthropic + BASE_URL, none" "" "$(probe anthropic GADFLY_BASE_URL=https://x)" echo "== providers needing no key are never blocked, with nothing set ==" for p in ollama llama-swap llama-swaps llamaswap llamaswaps foreman google gemini some-dsn-name; do @@ -89,6 +96,8 @@ echo "== engine specs carry their own auth and are never pre-flighted ==" # ollama-cloud; judging it by that would skip a reviewer using # CLAUDE_CODE_OAUTH_TOKEN, which needs no Ollama key. check "bare claude-code, no ollama key" "" "$(GADFLY_TEST_MODEL=claude-code probe ollama-cloud)" +# Go trims GADFLY_MODEL, so padding must not bypass the exemption. +check "claude-code w/ whitespace" "" "$(GADFLY_TEST_MODEL=" claude-code " probe ollama-cloud)" check "claude-code/opus, no ollama key" "" "$(GADFLY_TEST_MODEL=claude-code/opus probe ollama-cloud)" # opencode is NOT exempt: it drives an ollama-cloud model and needs that key, # so skipping it would disable the pre-flight for the one engine it can help.