Copilot commented on code in PR #2807:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2807#discussion_r3602359455
##########
internal/webhook/v1/consumer_webhook.go:
##########
@@ -227,15 +228,101 @@ func (v *ConsumerCustomValidator)
extractCredentialKey(ctx context.Context, cons
return "", nil
}
- var cfg struct {
- Key string `json:"key"`
+ key, err := parseInlineKeyAuthKey(credential.Config.Raw)
+ if err != nil {
+ return "", fmt.Errorf("invalid key-auth credential config for
Consumer %s/%s: %w",
+ consumer.Namespace, consumer.Name, err)
}
- if err := json.Unmarshal(credential.Config.Raw, &cfg); err != nil {
- // Malformed JSON is not a hard error: skip duplicate detection
for this
- // credential so existing consumers with bad config are not
suddenly denied.
- consumerLog.V(1).Info("skipping duplicate key-auth check:
malformed credential config",
- "consumer", consumer.Name, "error", err)
+ return key, nil
+}
+
+// parseInlineKeyAuthKey extracts the key-auth "key" from an inline credential
+// config the same way downstream cjson does: exact-case, string-valued,
+// last-wins. Ambiguous configs that Go's struct decoder would silently reject
+// while cjson still resolves to a live key (duplicate "key" members, or a
+// non-string "key") are returned as errors so they can't bypass the duplicate
+// check. Genuinely malformed JSON that cjson also rejects returns ("", nil) so
+// existing consumers with broken config are skipped, not suddenly denied.
+func parseInlineKeyAuthKey(raw []byte) (string, error) {
+ dec := json.NewDecoder(bytes.NewReader(raw))
+
+ // Top level must be an object, else there is no usable key.
+ tok, err := dec.Token()
+ if err != nil {
return "", nil
}
- return cfg.Key, nil
+ if delim, ok := tok.(json.Delim); !ok || delim != '{' {
+ return "", nil
+ }
+
+ var (
+ key string
+ keyCount int
+ )
+ for dec.More() {
+ nameTok, err := dec.Token()
+ if err != nil {
+ return "", nil
+ }
+ name, ok := nameTok.(string)
+ if !ok {
+ return "", nil
+ }
+ if name != "key" {
+ if err := skipJSONValue(dec); err != nil {
+ return "", nil
+ }
+ continue
+ }
+
+ keyCount++
+ valTok, err := dec.Token()
+ if err != nil {
+ return "", nil
+ }
+ switch val := valTok.(type) {
+ case string:
+ key = val
+ case nil:
+ // null key: no usable value, but still counts for dup
detection.
+ default:
+ // number/bool/object/array: cjson would deliver a
value here while
+ // Go's struct decoder errors and skips. Reject instead.
+ return "", fmt.Errorf("key-auth credential \"key\" must
be a string")
+ }
+ }
+
+ if keyCount > 1 {
+ return "", fmt.Errorf("key-auth credential config has duplicate
\"key\" members")
+ }
+ return key, nil
Review Comment:
parseInlineKeyAuthKey can treat malformed JSON as valid because it never
consumes the closing '}' or checks for trailing non-whitespace bytes.
json.Decoder permits streaming multiple top-level values, so inputs like
`{"key":"K"` (truncated) or `{"key":"K"}{"key":"X"}` would currently return a
key instead of being treated as malformed and skipped, contradicting the
documented leniency for malformed configs.
##########
internal/webhook/v1/consumer_webhook_test.go:
##########
@@ -192,3 +192,67 @@ func
TestConsumerValidator_DenyDuplicateKeyAuthCredential(t *testing.T) {
require.Contains(t, err.Error(), `duplicate key-auth credential key
"shared-key"`)
require.Contains(t, err.Error(), "default/existing")
}
+
+// A duplicate-key inline config ({"key":123,"key":"K"}) is unreadable to Go's
+// struct decoder but resolves to "K" downstream via cjson. The webhook must
+// reject it instead of silently skipping the duplicate check.
+func TestConsumerValidator_DenyDuplicateKeyAuthCredential_ParserDivergence(t
*testing.T) {
+ existing := &apisixv1alpha1.Consumer{
+ ObjectMeta: metav1.ObjectMeta{Name: "existing", Namespace:
"default"},
+ Spec: apisixv1alpha1.ConsumerSpec{
+ GatewayRef: apisixv1alpha1.GatewayRef{Name:
"test-gateway"},
+ Credentials: []apisixv1alpha1.Credential{{
+ Type: "key-auth",
+ Config: apiextensionsv1.JSON{Raw:
[]byte(`{"key":"victims-key"}`)},
+ }},
+ },
+ }
+ consumer := &apisixv1alpha1.Consumer{
+ ObjectMeta: metav1.ObjectMeta{Name: "demo", Namespace:
"default"},
+ Spec: apisixv1alpha1.ConsumerSpec{
+ GatewayRef: apisixv1alpha1.GatewayRef{Name:
"test-gateway"},
+ Credentials: []apisixv1alpha1.Credential{{
+ Type: "key-auth",
+ Config: apiextensionsv1.JSON{Raw:
[]byte(`{"key":123,"key":"victims-key"}`)},
+ }},
+ },
+ }
+
+ validator := buildConsumerValidator(t, existing)
+
+ _, err := validator.ValidateCreate(context.Background(), consumer)
+ require.Error(t, err)
+ require.Contains(t, err.Error(), "invalid key-auth credential config")
+}
+
+func TestParseInlineKeyAuthKey(t *testing.T) {
+ tests := []struct {
+ name string
+ raw string
+ wantKey string
+ wantErr bool
+ }{
+ {name: "plain string key", raw: `{"key":"K"}`, wantKey: "K"},
+ {name: "extra fields ignored", raw:
`{"key":"K","foo":{"a":1}}`, wantKey: "K"},
+ {name: "duplicate key members", raw: `{"key":"a","key":"b"}`,
wantErr: true},
+ {name: "number then string (divergence PoC)", raw:
`{"key":123,"key":"K"}`, wantErr: true},
+ {name: "non-string key", raw: `{"key":123}`, wantErr: true},
+ {name: "object key", raw: `{"key":{"nested":1}}`, wantErr:
true},
+ {name: "null key skipped", raw: `{"key":null}`, wantKey: ""},
+ {name: "no key member", raw: `{"foo":"bar"}`, wantKey: ""},
+ {name: "exact-case only, Key ignored", raw: `{"Key":"K"}`,
wantKey: ""},
+ {name: "malformed json skipped", raw: `{"key":`, wantKey: ""},
+ {name: "non-object skipped", raw: `["key","K"]`, wantKey: ""},
+ }
Review Comment:
The parser tests cover a malformed prefix (`{"key":`) but not other
malformed shapes that the current token-walk can mistakenly accept (e.g.,
truncated objects or multiple top-level JSON values). Adding these cases would
prevent regressions around the intended "malformed JSON is skipped" behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
