From 5a839ec6fb7f45890c9054ceda33683588a3e480 Mon Sep 17 00:00:00 2001 From: David Grieser Date: Tue, 10 Nov 2026 14:02:36 +0100 Subject: [PATCH] fix: address review findings in selfupdate, secret, cobrax, live and pick - selfupdate: drop the credential header on redirects that leave the origin of the first request. Go's HTTP client strips Authorization on its own but forwards PRIVATE-TOKEN, JOB-TOKEN and DEPLOY-TOKEN, so a package download redirected to object storage leaked the GitLab token. The header stays off when a later redirect returns to the origin, and a caller's CheckRedirect still applies. - secret: Fallback.Get wraps ErrNotFound when the secondary store has no secret, also when the primary failed, so Resolve goes on to prompt. - secret: read a piped secret up to the first newline only, so a pipe that stays open does not block and the rest of stdin stays unread. - cobrax: JoinSpacedBoolValues joins a flag only when the command the arguments select has that flag marked, so an unmarked flag of the same name on another command keeps its meaning. - live: keep a copy of the drawn lines, so a caller reusing its slice does not change how far the next draw moves up. - pick: fit checklists and toggles to the terminal height with a row window that follows the cursor. - pick: keep the selected key through the first load of a lazy view. - pick: apply View.Title to group-filtered views too. Co-Authored-By: Claude Opus 5.5 --- cobrax/boolargs.go | 92 +++++++++++++++++++++++++++++++-------- cobrax/cobrax_test.go | 36 ++++++++++++--- live/live.go | 5 ++- live/live_test.go | 17 ++++++++ pick/list.go | 24 ++++++++-- pick/toggle.go | 66 ++++++++++++++++++++++++---- pick/toggle_test.go | 51 ++++++++++++++++++++++ pick/views_test.go | 46 ++++++++++++++++++++ secret/resolve.go | 30 +++++++++---- secret/resolve_test.go | 44 +++++++++++++++++++ secret/store.go | 7 ++- secret/store_test.go | 4 +- selfupdate/github.go | 17 ++++---- selfupdate/gitlab.go | 24 +++++----- selfupdate/source.go | 67 +++++++++++++++++++++++++--- selfupdate/source_test.go | 70 +++++++++++++++++++++++++++++ 16 files changed, 528 insertions(+), 72 deletions(-) diff --git a/cobrax/boolargs.go b/cobrax/boolargs.go index 223079c..a029b26 100644 --- a/cobrax/boolargs.go +++ b/cobrax/boolargs.go @@ -29,33 +29,33 @@ func AllowSpacedBool(flags *pflag.FlagSet, names ...string) { } // JoinSpacedBoolValues rewrites "--flag false" into "--flag=false" for the -// boolean flags of root and its subcommands that [AllowSpacedBool] marked, -// so the space form parses. Pass the result to root.SetArgs. +// boolean flags that [AllowSpacedBool] marked, so the space form parses. +// Pass the result to root.SetArgs. // // pflag gives a boolean flag NoOptDefVal "true", which lets "--flag" stand // alone and leaves a following "false" behind as a positional argument. -// Only the marked long flags are joined, only when the next argument is -// exactly "true" or "false" (in any case), and never after "--". For a -// shell completion request the word being completed is left alone. +// Only long flags are joined, and only when the command the arguments +// select has a marked flag of that name, its own or inherited: an unmarked +// flag of the same name on another command keeps its meaning. The next +// argument must be exactly "true" or "false" (in any case), and nothing +// after "--" is touched. For a shell completion request the word being +// completed is left alone. func JoinSpacedBoolValues(root *cobra.Command, args []string) []string { - marked := map[string]bool{} + anyMarked := false walk(root, func(cmd *cobra.Command) { - collect := func(flag *pflag.Flag) { - if _, ok := flag.Annotations[SpacedBoolAnnotation]; ok { - marked[flag.Name] = true - } - } - cmd.LocalNonPersistentFlags().VisitAll(collect) - cmd.PersistentFlags().VisitAll(collect) + cmd.LocalFlags().VisitAll(func(flag *pflag.Flag) { + anyMarked = anyMarked || spaced(flag) + }) }) - if len(marked) == 0 { + if !anyMarked { return args } - limit := len(args) + first, limit := 0, len(args) if len(args) > 0 && (args[0] == cobra.ShellCompRequestCmd || args[0] == cobra.ShellCompNoDescRequestCmd) { - limit-- // The last word is the one being completed. + first, limit = 1, limit-1 // The last word is the one being completed. } + cmd := selected(root, args[first:limit]) joined := make([]string, 0, len(args)) for i := 0; i < len(args); i++ { arg := args[i] @@ -65,7 +65,7 @@ func JoinSpacedBoolValues(root *cobra.Command, args []string) []string { name, long := strings.CutPrefix(arg, "--") // limit never exceeds len(args); the second bound states it for // readers and for gosec. - if long && marked[name] && i+1 < limit && i+1 < len(args) && isBoolLiteral(args[i+1]) { + if long && i >= first && i+1 < limit && i+1 < len(args) && isBoolLiteral(args[i+1]) && spaced(lookupFlag(cmd, name)) { joined = append(joined, arg+"="+strings.ToLower(args[i+1])) i++ continue @@ -75,6 +75,64 @@ func JoinSpacedBoolValues(root *cobra.Command, args []string) []string { return joined } +// selected returns the command args select, the way cobra's Find walks +// them, except that "true" or "false" after a marked flag is that flag's +// value rather than a command name. +func selected(root *cobra.Command, args []string) *cobra.Command { + cmd := root + for i := 0; i < len(args); i++ { + arg := args[i] + switch { + case arg == "--": + return cmd + case strings.HasPrefix(arg, "--") && !strings.Contains(arg, "="): + flag := lookupFlag(cmd, arg[2:]) + // As in cobra, a flag that is unknown here or needs a value takes + // the next argument. + if flag == nil || flag.NoOptDefVal == "" || spaced(flag) && i+1 < len(args) && isBoolLiteral(args[i+1]) { + i++ + } + case len(arg) == 2 && arg[0] == '-' && arg[1] != '-': + if flag := lookupShorthand(cmd, arg[1:]); flag == nil || flag.NoOptDefVal == "" { + i++ + } + case strings.HasPrefix(arg, "-"): + // Joined short flags and --flag=value carry their values. + default: + child, _, _ := cmd.Find([]string{arg}) + if child == nil || child == cmd { + return cmd // The first positional argument ends the walk. + } + cmd = child + } + } + return cmd +} + +// lookupFlag finds a long flag of cmd, its own or inherited from a parent. +func lookupFlag(cmd *cobra.Command, name string) *pflag.Flag { + if flag := cmd.LocalFlags().Lookup(name); flag != nil { + return flag + } + return cmd.InheritedFlags().Lookup(name) +} + +func lookupShorthand(cmd *cobra.Command, name string) *pflag.Flag { + if flag := cmd.LocalFlags().ShorthandLookup(name); flag != nil { + return flag + } + return cmd.InheritedFlags().ShorthandLookup(name) +} + +// spaced reports whether [AllowSpacedBool] marked flag; nil is not marked. +func spaced(flag *pflag.Flag) bool { + if flag == nil { + return false + } + _, ok := flag.Annotations[SpacedBoolAnnotation] + return ok +} + func isBoolLiteral(arg string) bool { switch strings.ToLower(arg) { case "true", "false": diff --git a/cobrax/cobrax_test.go b/cobrax/cobrax_test.go index 18f8120..c70f87d 100644 --- a/cobrax/cobrax_test.go +++ b/cobrax/cobrax_test.go @@ -369,8 +369,14 @@ func spacedRoot() *cobra.Command { } deploy.Flags().String("previous", "", "previous version") root.PersistentFlags().BoolP("verbose", "v", false, "verbose") + root.PersistentFlags().Bool("dry-run", false, "dry run") + root.PersistentFlags().String("output", "", "output format") AllowSpacedBool(deploy.Flags(), "active", "stable", "hidden") - root.AddCommand(deploy) + AllowSpacedBool(root.PersistentFlags(), "dry-run") + // status has a --stable flag of its own that is not marked. + status := &cobra.Command{Use: "status", RunE: func(*cobra.Command, []string) error { return nil }} + status.Flags().Bool("stable", false, "stable only") + root.AddCommand(deploy, status) return root } @@ -385,17 +391,35 @@ func TestJoinSpacedBoolValues(t *testing.T) { []string{"deploy", "app-1.2", "--stable", "false", "--apply"}, []string{"deploy", "app-1.2", "--stable=false", "--apply"}}, {"several at once", - []string{"--active", "false", "--stable", "true"}, - []string{"--active=false", "--stable=true"}}, + []string{"deploy", "--active", "false", "--stable", "true"}, + []string{"deploy", "--active=false", "--stable=true"}}, {"bare flag stands alone", []string{"--stable", "--apply"}, []string{"--stable", "--apply"}}, {"equals form untouched", []string{"--stable=false"}, []string{"--stable=false"}}, {"flag at the end", []string{"deploy", "--stable"}, []string{"deploy", "--stable"}}, {"unmarked flags keep their positional", []string{"--previous", "false", "--apply", "true"}, []string{"--previous", "false", "--apply", "true"}}, {"non-boolean value not swallowed", []string{"--stable", "app-1.2"}, []string{"--stable", "app-1.2"}}, - {"case is normalized", []string{"--active", "FALSE"}, []string{"--active=false"}}, + {"case is normalized", []string{"deploy", "--active", "FALSE"}, []string{"deploy", "--active=false"}}, {"nothing after -- is touched", - []string{"--stable", "false", "--", "--active", "false"}, - []string{"--stable=false", "--", "--active", "false"}}, + []string{"deploy", "--stable", "false", "--", "--active", "false"}, + []string{"deploy", "--stable=false", "--", "--active", "false"}}, + {"flags before the command name", + []string{"--stable", "false", "deploy"}, + []string{"--stable=false", "deploy"}}, + {"flags of the selected command only", + []string{"status", "--stable", "false"}, + []string{"status", "--stable", "false"}}, + {"marked flags are not joined on the root", + []string{"--active", "false"}, + []string{"--active", "false"}}, + {"marked persistent flag before a command", + []string{"--dry-run", "false", "deploy", "--stable", "true"}, + []string{"--dry-run=false", "deploy", "--stable=true"}}, + {"marked persistent flag in a command", + []string{"status", "--dry-run", "true"}, + []string{"status", "--dry-run=true"}}, + {"flag values do not select a command", + []string{"--output", "deploy", "status", "--stable", "false"}, + []string{"--output", "deploy", "status", "--stable", "false"}}, {"short flags are not candidates", []string{"-v", "false"}, []string{"-v", "false"}}, {"completion keeps the word being completed", []string{cobra.ShellCompRequestCmd, "deploy", "--active", "false", "--stable", "fal"}, diff --git a/live/live.go b/live/live.go index 9cd7d0d..58bb5a6 100644 --- a/live/live.go +++ b/live/live.go @@ -3,6 +3,7 @@ package live import ( "fmt" "io" + "slices" "strings" "github.com/mattn/go-runewidth" @@ -87,7 +88,9 @@ func (l *Live) Draw(lines []string) error { l.last = frame l.drawn = nil if l.Fits(lines) { - l.drawn = lines + // A copy: the caller may reuse its slice for the next frame, while + // the screen still shows these lines. + l.drawn = slices.Clone(lines) } return nil } diff --git a/live/live_test.go b/live/live_test.go index a0820c2..1c43889 100644 --- a/live/live_test.go +++ b/live/live_test.go @@ -49,6 +49,23 @@ func TestLiveCountsRowsAtTheCurrentWidth(t *testing.T) { } } +// A caller that reuses its slice for the next frame does not change what +// the next draw moves up over: the screen still shows the old lines. +func TestLiveKeepsItsOwnCopyOfTheDrawnLines(t *testing.T) { + var out bytes.Buffer + live := New(&out) + live.size = func() (int, int) { return 4, 24 } + + frame := []string{"abcdefgh"} + _ = live.Draw(frame) + out.Reset() + frame[0] = "x" + _ = live.Draw(frame) + if got, want := out.String(), "\r\x1b[2A\x1b[Jx\n"; got != want { + t.Errorf("redraw = %q, want %q", got, want) + } +} + // Lines taller than the screen lose their first lines, so the next draw can // still move back up to where this one started; the cursor's row counts too. func TestLiveKeepsToTheScreen(t *testing.T) { diff --git a/pick/list.go b/pick/list.go index a59054e..8e3905a 100644 --- a/pick/list.go +++ b/pick/list.go @@ -55,6 +55,9 @@ type list struct { loaded []bool failures []string pending bool + // pendingKey is the key of the row selected when the pending view was + // switched to, for the cursor to land on once its rows are loaded. + pendingKey string typed func(string) bool typedNote string @@ -126,8 +129,8 @@ func newList(opts Options, height int, color bool) *list { } } } + l.applyViewTitle() if l.scoped { - l.title = firstNonEmpty(l.views[l.view].Title, l.fallback) l.setItems(l.views[l.view].Items) l.pending = l.needsLoad() } else { @@ -351,6 +354,7 @@ func (l *list) switchView(delta int) { n := len(l.views) if !l.scoped { l.view = (l.view + delta + n) % n + l.applyViewTitle() l.setFilter(l.filter) return } @@ -360,14 +364,26 @@ func (l *list) switchView(delta int) { } l.view = (l.view + delta + n) % n l.pending = l.needsLoad() + l.pendingKey = "" + if l.pending { + l.pendingKey = key + } l.loadView(key) } +// applyViewTitle shows the current view's title, or the options' title for +// a view without one. +func (l *list) applyViewTitle() { + if len(l.views) > 0 { + l.title = firstNonEmpty(l.views[l.view].Title, l.fallback) + } +} + // loadView makes the current view's own rows the ones on screen. The filter // carries over, and the cursor lands on the row carrying key, or at the top. func (l *list) loadView(key string) { view := l.views[l.view] - l.title = firstNonEmpty(view.Title, l.fallback) + l.applyViewTitle() l.setItems(view.Items) l.cursor, l.top = 0, 0 l.applyFilter() @@ -394,6 +410,8 @@ func (l *list) loadPending() bool { return l.pending } // the fetch blocks. func (l *list) runPendingLoad() { l.pending = false + key := l.pendingKey + l.pendingKey = "" view := l.views[l.view] if view.Load == nil || l.loaded[l.view] { return @@ -409,7 +427,7 @@ func (l *list) runPendingLoad() { } } l.views[l.view].Items = items - l.loadView("") + l.loadView(key) } // toggleMark marks the row under the cursor or unmarks it. The cursor stays, diff --git a/pick/toggle.go b/pick/toggle.go index 4272ee9..9d8dc2b 100644 --- a/pick/toggle.go +++ b/pick/toggle.go @@ -1,6 +1,7 @@ package pick import ( + "fmt" "os" "strings" @@ -128,10 +129,13 @@ type switches struct { initial []bool labelWidth int cursor int - nested bool - checklist bool - color bool - theme theme.Theme + // top is the first row drawn and visible how many are, so the block + // fits on the terminal; visible is all rows until resize says less. + top, visible int + nested bool + checklist bool + color bool + theme theme.Theme } func newSwitches(opts switchOptions, color bool) *switches { @@ -145,6 +149,7 @@ func newSwitches(opts switchOptions, color bool) *switches { checklist: opts.checklist, color: color, theme: theme.Active(), + visible: len(opts.rows), } for i, row := range opts.rows { s.initial[i] = row.value @@ -164,9 +169,45 @@ func (s *switches) values() []bool { return values } -// resize is what makes switches a screenState; the set is short enough that -// there is nothing to refit. -func (s *switches) resize(int, int) {} +// chrome is how many lines the block spends on something other than rows: +// the title, the note where there is one, the blank lines around the rows, +// the key line, and the position line when not all rows fit. +func (s *switches) chrome(windowed bool) int { + lines := 4 + if s.note != "" { + lines++ + } + if windowed { + lines++ + } + return lines +} + +// resize fits the rows to the terminal height, as the list does: a block +// taller than the terminal would scroll, and the in-place redraw would then +// repaint the wrong lines. Rows that do not fit scroll with the cursor. +func (s *switches) resize(maxVisible, height int) { + all := len(s.rows) + fits := (height <= 0 || all <= height-s.chrome(false)-1) && (maxVisible <= 0 || all <= maxVisible) + if fits { + s.visible = all + } else { + s.visible = min(visibleRows(maxVisible, height, s.chrome(true)), all) + } + s.scrollToCursor() +} + +func (s *switches) windowed() bool { return s.visible < len(s.rows) } + +func (s *switches) scrollToCursor() { + if s.cursor < s.top { + s.top = s.cursor + } + if s.cursor >= s.top+s.visible { + s.top = s.cursor - s.visible + 1 + } + s.top = max(min(s.top, len(s.rows)-s.visible), 0) +} func (s *switches) apply(k key) action { switch k.kind { @@ -178,12 +219,16 @@ func (s *switches) apply(k key) action { return actionInterrupt case keyUp: s.cursor = max(s.cursor-1, 0) + s.scrollToCursor() case keyDown: s.cursor = min(s.cursor+1, len(s.rows)-1) + s.scrollToCursor() case keyHome, keyPageUp: s.cursor = 0 + s.scrollToCursor() case keyEnd, keyPageDown: s.cursor = len(s.rows) - 1 + s.scrollToCursor() case keyTab: s.throw() case keyRune: @@ -236,16 +281,19 @@ func (s *switches) render(n int) []string { n = 80 } p := s.painter() - lines := make([]string, 0, len(s.rows)+6) + lines := make([]string, 0, s.visible+s.chrome(true)) lines = append(lines, drawParts(titleParts(s.title, s.subject, s.theme), n, p)) if s.note != "" { lines = append(lines, p.paint(s.theme.Muted, cut(s.note, n))) } lines = append(lines, "") - for i := range s.rows { + for i := s.top; i < min(s.top+s.visible, len(s.rows)); i++ { lines = append(lines, s.renderRow(i, n)) } lines = append(lines, "") + if s.windowed() { + lines = append(lines, p.paint(s.theme.Muted, cut(fmt.Sprintf("%d of %d", s.cursor+1, len(s.rows)), n))) + } lines = append(lines, p.paint(s.theme.Muted, s.hint(n))) return lines } diff --git a/pick/toggle_test.go b/pick/toggle_test.go index 04ce26d..20a4037 100644 --- a/pick/toggle_test.go +++ b/pick/toggle_test.go @@ -2,10 +2,14 @@ package pick import ( "errors" + "fmt" "io" "slices" + "strconv" "strings" "testing" + + "github.com/mittwald/cli-kit/term" ) func testSwitches() *switches { @@ -224,3 +228,50 @@ func TestDriveSwitches(t *testing.T) { t.Errorf("EOF: err = %v, want ErrInterrupted", err) } } + +// A checklist longer than the terminal draws a window of rows that follows +// the cursor, so the block always fits and redraws in place. +func TestChecklistFitsTheTerminalHeight(t *testing.T) { + rows := make([]switchRow, 30) + for i := range rows { + rows[i] = switchRow{label: "box " + strconv.Itoa(i+1)} + } + c := newSwitches(switchOptions{title: "Which?", note: "a note", rows: rows, checklist: true}, false) + const height = 24 + c.resize(0, height) + check := func(step string, wantFirst, wantCursor string) { + t.Helper() + lines := c.render(80) + if len(lines) >= height { + t.Fatalf("%s: %d lines on a %d-row terminal", step, len(lines), height) + } + if got := term.StripANSI(lines[3]); !strings.HasSuffix(got, " "+wantFirst) { + t.Errorf("%s: first row %q, want %q", step, got, wantFirst) + } + var cursorRow string + for _, line := range lines { + if strings.HasPrefix(line, cursorMarker) { + cursorRow = line + } + } + if !strings.HasSuffix(cursorRow, " "+wantCursor) { + t.Errorf("%s: cursor row %q, want %q", step, cursorRow, wantCursor) + } + if got, want := lines[len(lines)-2], fmt.Sprintf("%d of 30", c.cursor+1); got != want { + t.Errorf("%s: position line %q, want %q", step, got, want) + } + } + check("start", "box 1", "box 1") + c.apply(key{kind: keyEnd}) + check("End", "box 14", "box 30") + for range 20 { + c.apply(key{kind: keyUp}) + } + check("up", "box 10", "box 10") + + // A taller terminal shows every row again, without the position line. + c.resize(0, 60) + if lines := c.render(80); len(lines) != 30+5 { + t.Errorf("%d lines with room for all rows, want 35", len(lines)) + } +} diff --git a/pick/views_test.go b/pick/views_test.go index 1a3a919..b8fafe7 100644 --- a/pick/views_test.go +++ b/pick/views_test.go @@ -318,3 +318,49 @@ func TestSelectRejectsEmptyListsAndNonTerminals(t *testing.T) { t.Errorf("Checklist() without a terminal = %v, want ErrNotATerminal", err) } } + +// A view loaded on its first switch keeps the cursor on the row with the +// key that was selected, as a view that already holds its rows does. +func TestLazyViewKeepsTheSelectedKey(t *testing.T) { + opts := scopedOptions() + opts.Views[1] = View{Name: "remote", Load: func() ([]Item, error) { + return []Item{ + {Cells: []string{"origin/release"}, Key: "release"}, + {Cells: []string{"origin/feature/login"}, Key: "feature/login"}, + }, nil + }} + l := newList(opts, 0, false) + l.apply(key{kind: keyDown}) + l.apply(key{kind: keyTab}) + l.runPendingLoad() + if got := l.items[l.selected()].Cells[0]; got != "origin/feature/login" { + t.Errorf("after the load the cursor is on %q, want the row with the same key", got) + } +} + +// Views that narrow the items by group show their own title too. +func TestGroupViewsShowTheirTitle(t *testing.T) { + l := newList(Options{ + Title: "Which one?", + Items: []Item{ + {Cells: []string{"1.4.26"}, Groups: []string{"install"}}, + {Cells: []string{"1.4.25"}, Groups: []string{"edit"}}, + }, + Views: []View{ + {Name: "all"}, + {Name: "edit", Groups: []string{"edit"}, Title: "Which version to edit?"}, + }, + View: 1, + }, 0, false) + if got := l.render(80)[0]; !strings.HasPrefix(got, "Which version to edit?") { + t.Errorf("title = %q, want the initial view's title", got) + } + l.apply(key{kind: keyTab}) + if got := l.render(80)[0]; !strings.HasPrefix(got, "Which one?") { + t.Errorf("title = %q, want the options' title for a view without one", got) + } + l.apply(key{kind: keyTab}) + if got := l.render(80)[0]; !strings.HasPrefix(got, "Which version to edit?") { + t.Errorf("title = %q, want the view's title after Tab", got) + } +} diff --git a/secret/resolve.go b/secret/resolve.go index 8dfd6f9..cfb481a 100644 --- a/secret/resolve.go +++ b/secret/resolve.go @@ -349,20 +349,34 @@ func (r *Resolver) stored() (string, bool, error) { return value, value != "", nil } -// readFirstLine reads the first line of rd, bounded by MaxLen. A trailing -// CR is dropped, so a file written on Windows works; interior spaces are -// kept, because a passphrase may contain them. +// readFirstLine reads the first line of rd, bounded by MaxLen. It stops at +// the newline, so a pipe that stays open after the secret does not block, +// and the rest of stdin stays unread. A trailing CR is dropped, so a file +// written on Windows works; interior spaces are kept, because a passphrase +// may contain them. func (r *Resolver) readFirstLine(rd io.Reader) (string, error) { limit := r.MaxLen if limit <= 0 { limit = DefaultMaxLen } - data, err := io.ReadAll(io.LimitReader(rd, int64(limit))) - if err != nil { - return "", err + data := make([]byte, 0, min(limit, 256)) + var b [1]byte + for len(data) < limit { + n, err := rd.Read(b[:]) + if n == 1 { + if b[0] == '\n' { + break + } + data = append(data, b[0]) + } + if errors.Is(err, io.EOF) { + break + } + if err != nil { + return "", err + } } - line, _, _ := strings.Cut(string(data), "\n") - line = strings.TrimSuffix(line, "\r") + line := strings.TrimSuffix(string(data), "\r") if r.TrimSpace { line = strings.TrimSpace(line) } diff --git a/secret/resolve_test.go b/secret/resolve_test.go index d37570d..0eb4422 100644 --- a/secret/resolve_test.go +++ b/secret/resolve_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" "testing" + "time" ) // stdinFile returns a file holding what a pipe would have carried. A real @@ -275,3 +276,46 @@ func TestTTYPromptWithoutTerminal(t *testing.T) { t.Fatalf("TTYPrompt on a file = %v, want ErrNoSource", err) } } + +func TestResolvePromptsWhenFallbackStoreIsEmpty(t *testing.T) { + var asked []Request + r := Resolver{Account: "default", Store: Fallback(failing{}, &Memory{}), + Stdin: stdinFile(t, ""), Prompt: answers(&asked, "typed")} + if got, source, err := r.Resolve(); err != nil || got != "typed" || source != SourcePrompt { + t.Fatalf("Resolve = %q, %v, %v; want the prompt's answer", got, source, err) + } +} + +func TestResolveStopsAtNewlineOfAnOpenPipe(t *testing.T) { + rd, wr, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + defer rd.Close() + defer wr.Close() + if _, err := wr.WriteString("piped\nmore input\n"); err != nil { + t.Fatal(err) + } + type result struct { + value string + err error + } + done := make(chan result, 1) + go func() { + r := Resolver{Stdin: rd} + value, _, err := r.Resolve() + done <- result{value, err} + }() + select { + case res := <-done: + if res.err != nil || res.value != "piped" { + t.Fatalf("Resolve = %q, %v", res.value, res.err) + } + case <-time.After(5 * time.Second): + t.Fatal("Resolve waited for the pipe to close") + } + rest := make([]byte, 64) + if n, _ := rd.Read(rest); string(rest[:n]) != "more input\n" { + t.Fatalf("Resolve consumed past the first line: left %q", rest[:n]) + } +} diff --git a/secret/store.go b/secret/store.go index 089bcef..75868f7 100644 --- a/secret/store.go +++ b/secret/store.go @@ -81,6 +81,9 @@ func (m *Memory) Delete(account string) error { // without a Secret Service, a container), with a [FileStore] behind it: // // - Get reads primary, and secondary when primary has no secret or fails. +// When secondary has no secret either, the error wraps [ErrNotFound], +// also when primary failed, so a [Resolver] goes on to prompt; it +// wraps primary's failure too. // - Set writes primary, and secondary only when primary fails. // - Delete removes the account from both, so a secret written to the // secondary while the primary was unavailable does not come back. It @@ -104,7 +107,9 @@ func (f fallback) Get(account string) (string, error) { return "", secondaryErr } if errors.Is(secondaryErr, ErrNotFound) { - return "", err + // Primary is taken to be unavailable, as in Set and Delete; the + // secret is simply not stored yet. + return "", errors.Join(secondaryErr, err) } return "", errors.Join(err, secondaryErr) } diff --git a/secret/store_test.go b/secret/store_test.go index fd141ef..fc4a751 100644 --- a/secret/store_test.go +++ b/secret/store_test.go @@ -237,8 +237,8 @@ func TestFallbackUsesSecondaryWhenPrimaryFails(t *testing.T) { if err := s.Delete("alice"); err != nil { t.Fatalf("Delete = %v", err) } - if _, err := s.Get("alice"); !errors.Is(err, errUnavailable) { - t.Fatalf("Get after Delete = %v, want the primary's failure", err) + if _, err := s.Get("alice"); !errors.Is(err, ErrNotFound) || !errors.Is(err, errUnavailable) { + t.Fatalf("Get after Delete = %v, want ErrNotFound and the primary's failure", err) } } diff --git a/selfupdate/github.go b/selfupdate/github.go index c4e56b0..337abae 100644 --- a/selfupdate/github.go +++ b/selfupdate/github.go @@ -24,7 +24,9 @@ type GitHub struct { // for GitHub Enterprise Server ("https://github.example.com/api/v3") or // tests. APIURL string - // Client sends the requests; nil means http.DefaultClient. + // Client sends the requests; nil means http.DefaultClient. A redirect + // to another origin (scheme, host and port) never carries the + // credential; the client's own CheckRedirect still applies. Client *http.Client // UserAgent identifies the requests; empty means a generic default. UserAgent string @@ -39,13 +41,12 @@ func (g *GitHub) api() string { func (g *GitHub) requester() requester { return requester{ - client: g.Client, - token: g.Token, - userAgent: g.UserAgent, - headers: map[string]string{"X-GitHub-Api-Version": "2022-11-28"}, - authorize: func(req *http.Request, token string) { - req.Header.Set("Authorization", "Bearer "+token) - }, + client: g.Client, + token: g.Token, + userAgent: g.UserAgent, + headers: map[string]string{"X-GitHub-Api-Version": "2022-11-28"}, + authHeader: "Authorization", + authScheme: "Bearer", } } diff --git a/selfupdate/gitlab.go b/selfupdate/gitlab.go index 1431d09..796f63e 100644 --- a/selfupdate/gitlab.go +++ b/selfupdate/gitlab.go @@ -46,7 +46,9 @@ type GitLab struct { // TokenHeader is the header the credential goes in; empty means // [HeaderPrivateToken]. TokenHeader string - // Client sends the requests; nil means http.DefaultClient. + // Client sends the requests; nil means http.DefaultClient. A redirect + // to another origin (scheme, host and port) never carries the + // credential; the client's own CheckRedirect still applies. Client *http.Client // UserAgent identifies the requests; empty means a generic default. UserAgent string @@ -72,18 +74,16 @@ func (g *GitLab) requester() requester { if header == "" { header = HeaderPrivateToken } - return requester{ - client: g.Client, - token: g.Token, - userAgent: g.UserAgent, - authorize: func(req *http.Request, token string) { - if header == HeaderAuthorization { - req.Header.Set(HeaderAuthorization, "Bearer "+token) - return - } - req.Header.Set(header, token) - }, + r := requester{ + client: g.Client, + token: g.Token, + userAgent: g.UserAgent, + authHeader: header, } + if http.CanonicalHeaderKey(header) == HeaderAuthorization { + r.authScheme = "Bearer" + } + return r } func (g *GitLab) check() error { diff --git a/selfupdate/source.go b/selfupdate/source.go index abfa845..c66a86b 100644 --- a/selfupdate/source.go +++ b/selfupdate/source.go @@ -5,7 +5,9 @@ import ( "errors" "fmt" "io" + "net" "net/http" + "net/url" "strings" ) @@ -72,8 +74,10 @@ type requester struct { userAgent string // headers are set on every request. headers map[string]string - // authorize sets the credential on a request. - authorize func(req *http.Request, token string) + // authHeader is the header the credential goes in. authScheme, when + // set, precedes the credential, as in "Bearer ". + authHeader string + authScheme string } // get sends a GET request and returns the response when its status is 200. @@ -147,13 +151,18 @@ func (r requester) send(ctx context.Context, url, accept, token string) (*http.R for name, value := range r.headers { req.Header.Set(name, value) } - if token != "" && r.authorize != nil { - r.authorize(req, token) - } client := r.client if client == nil { client = http.DefaultClient } + if token != "" && r.authHeader != "" { + value := token + if r.authScheme != "" { + value = r.authScheme + " " + token + } + req.Header.Set(r.authHeader, value) + client = withoutCredentialOnRedirect(client, r.authHeader) + } resp, err := client.Do(req) if err != nil { return nil, fmt.Errorf("selfupdate: %w", err) @@ -161,6 +170,54 @@ func (r requester) send(ctx context.Context, url, accept, token string) (*http.R return resp, nil } +// withoutCredentialOnRedirect returns a copy of client that removes header +// from a redirected request once the redirects left the origin (scheme, +// host and port) of the first request. The HTTP client copies all headers +// of the first request to each redirect and only drops Authorization and +// cookies on its own, so a GitLab token header would otherwise reach the +// storage host a package download redirects to. The client's own +// CheckRedirect still applies. +func withoutCredentialOnRedirect(client *http.Client, header string) *http.Client { + c := *client + next := client.CheckRedirect + c.CheckRedirect = func(req *http.Request, via []*http.Request) error { + left := !sameOrigin(req.URL, via[0].URL) + for _, prev := range via[1:] { + left = left || !sameOrigin(prev.URL, via[0].URL) + } + if left { + req.Header.Del(header) + } + if next != nil { + return next(req, via) + } + if len(via) >= 10 { + return errors.New("stopped after 10 redirects") + } + return nil + } + return &c +} + +func sameOrigin(a, b *url.URL) bool { + return strings.EqualFold(a.Scheme, b.Scheme) && strings.EqualFold(hostPort(a), hostPort(b)) +} + +// hostPort returns the URL's host with the scheme's default port made +// explicit, so that "example.com" and "example.com:443" compare equal. +func hostPort(u *url.URL) string { + port := u.Port() + if port == "" { + switch strings.ToLower(u.Scheme) { + case "https": + port = "443" + case "http": + port = "80" + } + } + return net.JoinHostPort(u.Hostname(), port) +} + func drain(resp *http.Response) { _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 64<<10)) _ = resp.Body.Close() diff --git a/selfupdate/source_test.go b/selfupdate/source_test.go index 06bbdea..355c55c 100644 --- a/selfupdate/source_test.go +++ b/selfupdate/source_test.go @@ -194,3 +194,73 @@ func TestGitHubLatestAndDownload(t *testing.T) { t.Fatal("Latest without Owner succeeded") } } + +func TestCredentialNotSentToOtherOrigins(t *testing.T) { + for _, header := range []string{HeaderPrivateToken, HeaderJobToken, HeaderDeployToken, HeaderAuthorization} { + t.Run(header, func(t *testing.T) { + var storageSaw []string + var gitlab *httptest.Server + // storage serves the file after the first redirect and bounces + // a second request back to the GitLab origin. + storage := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + storageSaw = append(storageSaw, r.Header.Get(header)) + if r.URL.Path == "/bounce" { + http.Redirect(w, r, gitlab.URL+"/back", http.StatusFound) + return + } + _, _ = io.WriteString(w, "archive") + })) + defer storage.Close() + var backSaw string + gitlab = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.URL.Path == "/back": + backSaw = r.Header.Get(header) + _, _ = io.WriteString(w, "archive") + case strings.HasSuffix(r.URL.Path, "/a.tar.gz"): + http.Redirect(w, r, storage.URL+"/blob", http.StatusFound) + case strings.HasSuffix(r.URL.Path, "/b.tar.gz"): + http.Redirect(w, r, storage.URL+"/bounce", http.StatusFound) + default: + http.NotFound(w, r) + } + })) + defer gitlab.Close() + + g := &GitLab{BaseURL: gitlab.URL, Project: "example/tool", Package: "tool", TokenHeader: header, + Token: func(context.Context, bool) (string, error) { return "secret-token", nil }} + for _, name := range []string{"a.tar.gz", "b.tar.gz"} { + body, err := g.Download(context.Background(), Release{Tag: "v1.0.0"}, name) + if err != nil { + t.Fatalf("Download %s = %v", name, err) + } + readAll(t, body) + } + for _, got := range storageSaw { + if got != "" { + t.Errorf("storage host received %s %q", header, got) + } + } + if backSaw != "" { + t.Errorf("redirect back to the origin received %s %q after leaving it", header, backSaw) + } + }) + } +} + +func TestCredentialRedirectPolicyKeepsClientPolicy(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, "/elsewhere", http.StatusFound) + })) + defer srv.Close() + stop := errors.New("no redirects") + client := &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return stop }} + g := &GitLab{BaseURL: srv.URL, Project: "example/tool", Client: client, + Token: func(context.Context, bool) (string, error) { return "tok", nil }} + if _, err := g.Latest(context.Background()); !errors.Is(err, stop) { + t.Fatalf("Latest = %v, want the client's CheckRedirect error", err) + } + if client.CheckRedirect == nil { + t.Fatal("client's CheckRedirect was replaced") + } +}