Skip to content

Commit cb4a53c

Browse files
committed
fix(service): backquote metavar leak; missing-required hint names both input paths
pflag's UnquoteUsage treats a backquoted word in a flag's usage string as the flag's metavar, so a metadata description like wiki space_id's "可替换为`my_library`" rendered the flag as "--space-id my_library" instead of "--space-id string". Backquotes are markdown emphasis in the metadata prose, never meaningful in terminal help — drop them in inlineClause so every prose surface (field description, enum option) is covered at the one normalization point. The pre-flight error for a missing required parameter pointed only at `lark-cli schema <path>`, forcing a round-trip even though the recovery is one flag away. The hint now names both input paths: the typed flag when the binder registered one, and the --params form either way. Whether a typed flag exists is the binder's knowledge, not cmd.Flags(): a params-only field's kebab name does resolve to a flag — the colliding one (e.g. the output --format) — and a Lookup-based hint would steer the reader to set the wrong flag. Change-Id: I19c3318a3ca6159c944f993698ccaaa23af9229b
1 parent 39552ed commit cb4a53c

5 files changed

Lines changed: 98 additions & 3 deletions

File tree

cmd/service/paramflags.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,22 @@ func (b *paramFlagBinder) paramsOnlyHelp() string {
115115
return sb.String()
116116
}
117117

118+
// hasTypedFlag reports whether the binder registered a typed flag for the
119+
// param named name. False for params-only fields — a flag with the same kebab
120+
// name may exist (that's the collision), but it is not this param's input.
121+
// Nil-safe for direct buildServiceRequest callers that have no binder.
122+
func (b *paramFlagBinder) hasTypedFlag(name string) bool {
123+
if b == nil {
124+
return false
125+
}
126+
for _, pf := range b.bound {
127+
if pf.field.Name == name {
128+
return true
129+
}
130+
}
131+
return false
132+
}
133+
118134
// overlay lets an explicit typed flag override the same key in --params
119135
// (--params is the base). Only changed flags apply, so the --params-only path is
120136
// unchanged. A nil binder or cmd is a no-op.

cmd/service/paramflags_test.go

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,11 @@
44
package service
55

66
import (
7+
"errors"
78
"strings"
89
"testing"
910

11+
"github.com/larksuite/cli/errs"
1012
"github.com/larksuite/cli/internal/cmdutil"
1113
"github.com/larksuite/cli/internal/meta"
1214
"github.com/spf13/cobra"
@@ -574,3 +576,51 @@ func TestServiceMethod_TypedFlag_HelpShowsBounds(t *testing.T) {
574576
t.Errorf("flag usage should carry bounds, got %q", fl.Usage)
575577
}
576578
}
579+
580+
// The missing-required hint must name both recovery paths — the typed flag and
581+
// the --params fallback — so a reader who only knows one input style can
582+
// proceed without a round-trip through schema.
583+
func TestServiceMethod_MissingRequired_HintNamesFlagAndParams(t *testing.T) {
584+
f, _, _, _ := cmdutil.TestFactory(t, testConfig)
585+
cmd := NewCmdServiceMethod(f, imSpec(), imChatMembersCreate(), "create", "chat.members", nil)
586+
cmd.SetArgs([]string{"--data", `{"id_list":["ou_x"]}`, "--dry-run"})
587+
588+
err := cmd.Execute()
589+
var ve *errs.ValidationError
590+
if !errors.As(err, &ve) {
591+
t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err)
592+
}
593+
for _, want := range []string{"--chat-id", `--params '{"chat_id": "<value>"}'`, "lark-cli schema im.chat.members.create"} {
594+
if !strings.Contains(ve.Hint, want) {
595+
t.Errorf("hint %q should contain %q", ve.Hint, want)
596+
}
597+
}
598+
}
599+
600+
// A params-only required field (kebab name claimed by the standard --format
601+
// flag) has no typed flag to offer: the hint must give only the --params form,
602+
// never steer the reader to the colliding flag.
603+
func TestServiceMethod_MissingRequired_ParamsOnlyHintSkipsFlag(t *testing.T) {
604+
method := meta.FromMap(map[string]interface{}{
605+
"path": "messages",
606+
"httpMethod": "GET",
607+
"parameters": map[string]interface{}{
608+
"format": map[string]interface{}{"type": "string", "location": "query", "required": true},
609+
},
610+
})
611+
f, _, _, _ := cmdutil.TestFactory(t, testConfig)
612+
cmd := NewCmdServiceMethod(f, imSpec(), method, "list", "messages", nil)
613+
cmd.SetArgs([]string{"--dry-run"})
614+
615+
err := cmd.Execute()
616+
var ve *errs.ValidationError
617+
if !errors.As(err, &ve) {
618+
t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err)
619+
}
620+
if !strings.Contains(ve.Hint, `--params '{"format": "<value>"}'`) {
621+
t.Errorf("hint %q should carry the --params form", ve.Hint)
622+
}
623+
if strings.Contains(ve.Hint, "set --format") {
624+
t.Errorf("hint %q must not steer to the colliding --format flag", ve.Hint)
625+
}
626+
}

cmd/service/paramhelp.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,11 @@ func inlineClause(s, stops string, max int) string {
8383
return ""
8484
}
8585
s = markdownLinkRe.ReplaceAllString(s, "$1")
86+
// Backquotes must go: pflag's UnquoteUsage treats a backquoted word in a
87+
// flag's usage string as the flag's metavar, so a description like wiki
88+
// space_id's "可替换为`my_library`" would render the flag as
89+
// "--space-id my_library" instead of "--space-id string".
90+
s = strings.ReplaceAll(s, "`", "")
8691
if i := strings.IndexAny(s, stops); i >= 0 {
8792
s = s[:i]
8893
}

cmd/service/sanitize_test.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,3 +48,14 @@ func TestSanitizeFieldDesc_TrimsDanglingPunctuation(t *testing.T) {
4848
}
4949
}
5050
}
51+
52+
func TestSanitizeFieldDesc_StripsBackquotes(t *testing.T) {
53+
// pflag's UnquoteUsage takes a backquoted word in a flag's usage string as
54+
// the flag's metavar: wiki space_id's description rendered the flag as
55+
// "--space-id my_library" instead of "--space-id string".
56+
in := "[知识空间id](https://x/wiki),如果查询我的文档库可替换为`my_library`"
57+
want := "知识空间id,如果查询我的文档库可替换为my_library"
58+
if got := sanitizeFieldDesc(in); got != want {
59+
t.Errorf("sanitizeFieldDesc(%q) = %q, want %q", in, got, want)
60+
}
61+
}

cmd/service/service.go

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -472,12 +472,25 @@ func unusableParamValue(v interface{}) bool {
472472
return ok && s == ""
473473
}
474474

475+
// missingParamHint is the recovery hint for a missing required parameter. It
476+
// names both input paths — the typed flag when the binder registered one, and
477+
// the --params fallback — plus the schema pointer. A params-only field gets
478+
// only the --params form: a flag with its kebab name exists but belongs to
479+
// something else (e.g. the output --format), and the hint must not steer
480+
// there. Asking the binder, not cmd.Flags(), is what tells those apart.
481+
func missingParamHint(opts *ServiceMethodOptions, f meta.Field) string {
482+
paramsForm := fmt.Sprintf("--params '{%q: \"<value>\"}'", f.Name)
483+
if opts.binder.hasTypedFlag(f.Name) {
484+
return fmt.Sprintf("set --%s <value> (or %s); see: lark-cli schema %s", f.FlagName(), paramsForm, opts.SchemaPath)
485+
}
486+
return fmt.Sprintf("set %s; see: lark-cli schema %s", paramsForm, opts.SchemaPath)
487+
}
488+
475489
// buildServiceRequest parses flags, builds the URL with path/query params, and returns a RawApiRequest.
476490
// When dryRun is true and a file is provided, file reading is skipped and
477491
// FileUploadMeta is returned instead so the caller can render dry-run output.
478492
func buildServiceRequest(opts *ServiceMethodOptions) (client.RawApiRequest, *cmdutil.FileUploadMeta, error) {
479493
method := opts.Method
480-
schemaPath := opts.SchemaPath
481494
httpMethod := method.HTTPMethod
482495

483496
// stdin is an io.Reader consumed at most once. Only one of --params/--data
@@ -509,7 +522,7 @@ func buildServiceRequest(opts *ServiceMethodOptions) (client.RawApiRequest, *cmd
509522
if !ok || unusableParamValue(val) {
510523
return client.RawApiRequest{}, nil, errs.NewValidationError(errs.SubtypeInvalidArgument,
511524
"missing required path parameter: %s", s.Name).
512-
WithHint("lark-cli schema %s", schemaPath).
525+
WithHint("%s", missingParamHint(opts, s)).
513526
WithParam(s.Name)
514527
}
515528
valStr := fmt.Sprintf("%v", val)
@@ -530,7 +543,7 @@ func buildServiceRequest(opts *ServiceMethodOptions) (client.RawApiRequest, *cmd
530543
if s.Required && !isPaginationParam && (!exists || unusableParamValue(value)) {
531544
return client.RawApiRequest{}, nil, errs.NewValidationError(errs.SubtypeInvalidArgument,
532545
"missing required query parameter: %s", s.Name).
533-
WithHint("lark-cli schema %s", schemaPath).
546+
WithHint("%s", missingParamHint(opts, s)).
534547
WithParam(s.Name)
535548
}
536549
if exists && !unusableParamValue(value) {

0 commit comments

Comments
 (0)