Skip to content

Commit 475c394

Browse files
authored
fix: Address full-repo review findings (28 of 61) (#21)
- Remove parser repoRoot global, thread root parameter (CRITICAL) - Fix FetchAll data races with teamPartials struct - Add GetProfiles pagination loop - Validate Fleet server URL with url.Parse - Warn on insecure HTTP and world-readable config files - Reject .. in branch names, enforce HTTPS in doRequest - Add -- separator to git fetch/diff commands - Add .. path traversal check in scope.go - Escape resource names in markdown table cells - Sanitize HTTPError body (strip newlines) - Remove repoRoot global, accept .yaml team files - Error on non-map YAML overlay in merge - Report query logging changes to empty string - Deduplicate matchesAnyTeam, normalizeSoftwarePath - Remove bare block scope in diffSoftware - Fix stale docs: API endpoints, config paths, merge description Ref #20
1 parent 33f6964 commit 475c394

File tree

12 files changed

+247
-187
lines changed

12 files changed

+247
-187
lines changed

README.md

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ fleet-plan --git --base base.yml --env environments/prod.yml --format markdown
7070
| `-v`, `--verbose` | Show full old/new values for modified fields | `-v` |
7171
| `--heading` | Custom heading for markdown output | `--heading "Staging diff"` |
7272
| `--detailed-exitcodes` | Exit 2 when changes detected (0=none, 1=error) | `--detailed-exitcodes` |
73-
| `--git` | CI mode: auto-detect platform, resolve changed files, infer teams, post MR/PR comment | `--git` |
73+
| `--git` | CI mode: auto-detect platform, resolve changed files, infer teams, post MR/PR comment (requires `--format markdown`) | `--git` |
7474
| `--base` | Path to base.yml for multi-env config merge (requires `--env`) | `--base base.yml` |
7575
| `--env` | Path to environment overlay YAML, merged with `--base` in-memory | `--env environments/prod.yml` |
7676

@@ -87,7 +87,11 @@ Use `fleetctl gitops --dry-run` for secret substitution, server-side validation,
8787

8888
### Auth
8989

90-
Set via flags, environment variables, or a config file (`~/.config/fleet-plan.json`):
90+
Set via flags, environment variables, or a config file. Config file search order (first found wins):
91+
92+
1. `<repo>/.config/fleet-plan.json`
93+
2. `~/.config/fleet-plan.json`
94+
9195

9296
```bash
9397
# Env vars (CI)
@@ -133,6 +137,7 @@ When `--git` is active, fleet-plan:
133137
| Env var | Used for |
134138
|---|---|
135139
| `FLEET_URL` / `FLEET_TOKEN` | Fleet API auth |
140+
| `FLEET_PLAN_INSECURE` | Set to `1` to allow plain HTTP connections (local dev only) |
136141
| `FLEET_PLAN_BOT` | GitLab: token for posting MR comments |
137142
| `GITHUB_TOKEN` | GitHub: token for posting PR comments |
138143
| `CI_JOB_URL` / `GITHUB_SERVER_URL` + `GITHUB_REPOSITORY` + `GITHUB_RUN_ID` | Link back to the pipeline job in the comment |

cmd/fleet-plan/main.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,7 @@ func runDiff(cmd *cobra.Command, _ []string) error {
144144

145145
fmt.Fprintf(os.Stderr, "Fetching Fleet state from %s...\n", auth.URL)
146146

147-
fetchGlobal := repo.Global != nil
148-
state, err := client.FetchAll(ctx, fetchGlobal)
147+
state, err := client.FetchAll(ctx, repo.Global != nil)
149148
if err != nil {
150149
return err
151150
}

docs/API-Endpoints.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,12 @@ All read-only. fleet-plan never writes to your Fleet server.
1010
| `GET` | `/api/v1/fleet/teams/{id}/policies` | Per-team policies |
1111
| `GET` | `/api/v1/fleet/global/policies` | Global policies (when default.yml parsed) |
1212
| `GET` | `/api/v1/fleet/queries` | Per-team and global queries |
13-
| `GET` | `/api/v1/fleet/mdm/profiles` | MDM configuration profiles |
13+
| `GET` | `/api/v1/fleet/configuration_profiles` | MDM configuration profiles |
1414
| `GET` | `/api/v1/fleet/software/titles` | Managed software titles (paginated) |
1515
| `GET` | `/api/v1/fleet/software/fleet_maintained_apps` | Fleet-maintained app catalog (paginated) |
1616
| `GET` | `/api/v1/fleet/scripts` | Team scripts for line-count diff (paginated) |
17+
| `GET` | `/api/v1/fleet/scripts/{id}?alt=media` | Script content download |
18+
| `GET` | `/api/v1/fleet/software/titles/{id}` | Software title detail |
1719

1820
Global endpoints (`/config`, `/global/policies`, `/queries` with teamID=0) are only called when `default.yml` defines global sections.
1921

docs/Architecture.md

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ Priority order (highest wins):
6666

6767
1. `--url` / `--token` flags
6868
2. `FLEET_URL` / `FLEET_TOKEN` env vars
69-
3. Config file: `~/.config/fleet-plan.json` or `<repo>/.config/fleet-plan.json`
69+
3. Config file: `<repo>/.config/fleet-plan.json` (checked first), then `~/.config/fleet-plan.json` (fallback)
7070

7171
Config file supports multiple contexts:
7272

@@ -118,11 +118,7 @@ Whitespace is normalized before comparison to avoid false positives from YAML vs
118118

119119
## Demo GIF
120120

121-
`assets/vhs-demo.go` renders representative output from testdata for the README demo GIF. See [assets/README.md](https://github.com/TsekNet/fleet-plan/blob/main/assets/README.md) for prerequisites and setup. Regenerate:
122-
123-
```bash
124-
vhs assets/demo.tape
125-
```
121+
`assets/vhs-demo.go` renders representative output from testdata for the README demo GIF. See [assets/README.md](https://github.com/TsekNet/fleet-plan/blob/main/assets/README.md) for prerequisites, setup, and regeneration steps.
126122

127123
---
128124

@@ -148,7 +144,7 @@ When `--git` is active, the `git` package detects the CI platform and drives the
148144
- Maps are deep-merged: nested keys in the overlay are merged into the base map recursively.
149145
- Arrays are replaced: an overlay array replaces the base array entirely (no element-level merge).
150146

151-
This mirrors how fleet-gitops environment overlays work, so the diff reflects the final merged config without writing a temporary file.
147+
This mirrors how fleet-gitops environment overlays work. The merged result is written to a temp file that is cleaned up on exit, so no persistent files are left behind.
152148

153149
---
154150

internal/api/client.go

Lines changed: 93 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,17 @@ type Client struct {
3131
func NewClient(baseURL, token string) (*Client, error) {
3232
baseURL = strings.TrimRight(baseURL, "/")
3333

34-
if strings.HasPrefix(strings.ToLower(baseURL), "http://") && os.Getenv("FLEET_PLAN_INSECURE") != "1" {
35-
return nil, fmt.Errorf("refusing to send API token over plain HTTP (%s)\nUse https:// or set FLEET_PLAN_INSECURE=1 to override", baseURL)
34+
parsed, err := url.Parse(baseURL)
35+
if err != nil || parsed.Scheme == "" || parsed.Host == "" {
36+
return nil, fmt.Errorf("invalid Fleet server URL %q: must include scheme and host", baseURL)
37+
}
38+
39+
insecure := os.Getenv("FLEET_PLAN_INSECURE") == "1"
40+
if strings.ToLower(parsed.Scheme) == "http" {
41+
if !insecure {
42+
return nil, fmt.Errorf("refusing to send API token over plain HTTP (%s)\nUse https:// or set FLEET_PLAN_INSECURE=1 to override", baseURL)
43+
}
44+
fmt.Fprintf(os.Stderr, "warning: FLEET_PLAN_INSECURE=1, sending API token over plain HTTP\n")
3645
}
3746

3847
return &Client{
@@ -88,7 +97,9 @@ type HTTPError struct {
8897
}
8998

9099
func (e *HTTPError) Error() string {
91-
return fmt.Sprintf("HTTP %d from %s: %s", e.StatusCode, e.URL, e.Body)
100+
body := strings.ReplaceAll(e.Body, "\n", " ")
101+
body = strings.ReplaceAll(body, "\r", " ")
102+
return fmt.Sprintf("HTTP %d from %s: %s", e.StatusCode, e.URL, body)
92103
}
93104

94105
// isPermissionError returns true if err is an HTTP 403 or 404, which indicates
@@ -291,6 +302,9 @@ type labelsResponse struct {
291302

292303
type profilesResponse struct {
293304
Profiles []Profile `json:"profiles"`
305+
Meta struct {
306+
HasNextResults bool `json:"has_next_results"`
307+
} `json:"meta"`
294308
}
295309

296310
type scriptsResponse struct {
@@ -525,17 +539,32 @@ func (c *Client) GetLabels(ctx context.Context) ([]Label, error) {
525539
return all, nil
526540
}
527541

528-
// GetProfiles fetches MDM profiles for a team.
542+
// GetProfiles fetches MDM profiles for a team with pagination.
529543
func (c *Client) GetProfiles(ctx context.Context, teamID uint) ([]Profile, error) {
530-
q := url.Values{"per_page": {"250"}}
531-
if teamID > 0 {
532-
q.Set("team_id", strconv.FormatUint(uint64(teamID), 10))
533-
}
534-
var resp profilesResponse
535-
if err := c.get(ctx, "/api/v1/fleet/configuration_profiles", q, &resp); err != nil {
536-
return nil, fmt.Errorf("fetching profiles (team %d): %w", teamID, err)
544+
var all []Profile
545+
page := 0
546+
for {
547+
q := url.Values{
548+
"per_page": {"250"},
549+
"page": {strconv.Itoa(page)},
550+
}
551+
if teamID > 0 {
552+
q.Set("team_id", strconv.FormatUint(uint64(teamID), 10))
553+
}
554+
var resp profilesResponse
555+
if err := c.get(ctx, "/api/v1/fleet/configuration_profiles", q, &resp); err != nil {
556+
return nil, fmt.Errorf("fetching profiles (team %d): %w", teamID, err)
557+
}
558+
all = append(all, resp.Profiles...)
559+
if !resp.Meta.HasNextResults || len(resp.Profiles) == 0 {
560+
break
561+
}
562+
page++
563+
if page > 100 { // safety: max 25k profiles
564+
break
565+
}
537566
}
538-
return resp.Profiles, nil
567+
return all, nil
539568
}
540569

541570
// GetScripts fetches scripts for a team with pagination.
@@ -646,34 +675,55 @@ func (c *Client) FetchAll(ctx context.Context, fetchGlobal ...bool) (*FleetState
646675
g, gctx := errgroup.WithContext(ctx)
647676
g.SetLimit(5)
648677

678+
// Local variables for global results; assigned to state after g.Wait().
679+
var (
680+
globalConfig map[string]any
681+
globalPolicies []Policy
682+
globalQueries []Query
683+
)
684+
649685
// Fetch global config/policies/queries in parallel with team resources
650686
if wantGlobal {
651687
g.Go(func() error {
652688
cfg, err := c.GetConfig(gctx)
653689
if err != nil {
654690
return err
655691
}
656-
state.Config = cfg
692+
globalConfig = cfg
657693
return nil
658694
})
659695
g.Go(func() error {
660696
policies, err := c.GetPolicies(gctx, 0)
661697
if err != nil {
662698
return err
663699
}
664-
state.GlobalPolicies = policies
700+
globalPolicies = policies
665701
return nil
666702
})
667703
g.Go(func() error {
668704
queries, err := c.GetQueries(gctx, 0)
669705
if err != nil {
670706
return err
671707
}
672-
state.GlobalQueries = queries
708+
globalQueries = queries
673709
return nil
674710
})
675711
}
676712

713+
// teamPartials holds per-goroutine results indexed by team slot.
714+
// Each field is written by exactly one goroutine, so there is no data race.
715+
type teamPartial struct {
716+
policies []Policy
717+
queries []Query
718+
profiles []Profile
719+
profilesUnavailable bool
720+
softwareTitles []SoftwareTitle
721+
softwareUnavailable bool
722+
scripts []Script
723+
scriptsUnavailable bool
724+
}
725+
teamPartials := make([]teamPartial, len(teams))
726+
677727
teamResults := make([]Team, len(teams))
678728
for i, t := range teams {
679729
teamResults[i] = t
@@ -685,7 +735,7 @@ func (c *Client) FetchAll(ctx context.Context, fetchGlobal ...bool) (*FleetState
685735
if err != nil {
686736
return err
687737
}
688-
teamResults[idx].Policies = policies
738+
teamPartials[idx].policies = policies
689739
return nil
690740
})
691741

@@ -694,7 +744,7 @@ func (c *Client) FetchAll(ctx context.Context, fetchGlobal ...bool) (*FleetState
694744
if err != nil {
695745
return err
696746
}
697-
teamResults[idx].Queries = queries
747+
teamPartials[idx].queries = queries
698748
return nil
699749
})
700750

@@ -704,10 +754,10 @@ func (c *Client) FetchAll(ctx context.Context, fetchGlobal ...bool) (*FleetState
704754
if !isPermissionError(err) {
705755
return err
706756
}
707-
teamResults[idx].ProfilesUnavailable = true
757+
teamPartials[idx].profilesUnavailable = true
708758
profiles = nil
709759
}
710-
teamResults[idx].Profiles = profiles
760+
teamPartials[idx].profiles = profiles
711761
return nil
712762
})
713763

@@ -717,10 +767,10 @@ func (c *Client) FetchAll(ctx context.Context, fetchGlobal ...bool) (*FleetState
717767
if !isPermissionError(err) {
718768
return err
719769
}
720-
teamResults[idx].SoftwareUnavailable = true
770+
teamPartials[idx].softwareUnavailable = true
721771
softwareTitles = nil
722772
}
723-
teamResults[idx].SoftwareTitles = softwareTitles
773+
teamPartials[idx].softwareTitles = softwareTitles
724774
return nil
725775
})
726776

@@ -730,10 +780,10 @@ func (c *Client) FetchAll(ctx context.Context, fetchGlobal ...bool) (*FleetState
730780
if !isPermissionError(err) {
731781
return err
732782
}
733-
teamResults[idx].ScriptsUnavailable = true
783+
teamPartials[idx].scriptsUnavailable = true
734784
scripts = nil
735785
}
736-
teamResults[idx].Scripts = scripts
786+
teamPartials[idx].scripts = scripts
737787
return nil
738788
})
739789
}
@@ -742,6 +792,26 @@ func (c *Client) FetchAll(ctx context.Context, fetchGlobal ...bool) (*FleetState
742792
return nil, err
743793
}
744794

795+
// Assign global results after all goroutines have completed.
796+
if wantGlobal {
797+
state.Config = globalConfig
798+
state.GlobalPolicies = globalPolicies
799+
state.GlobalQueries = globalQueries
800+
}
801+
802+
// Assign per-team partial results.
803+
for i := range teamResults {
804+
p := &teamPartials[i]
805+
teamResults[i].Policies = p.policies
806+
teamResults[i].Queries = p.queries
807+
teamResults[i].Profiles = p.profiles
808+
teamResults[i].ProfilesUnavailable = p.profilesUnavailable
809+
teamResults[i].SoftwareTitles = p.softwareTitles
810+
teamResults[i].SoftwareUnavailable = p.softwareUnavailable
811+
teamResults[i].Scripts = p.scripts
812+
teamResults[i].ScriptsUnavailable = p.scriptsUnavailable
813+
}
814+
745815
// Enrich script contents (second pass, needs script IDs from first pass)
746816
for i := range teamResults {
747817
if !teamResults[i].ScriptsUnavailable && len(teamResults[i].Scripts) > 0 {

internal/config/config.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,11 @@ func loadConfigFile(root string) configFile {
6262
if err != nil {
6363
return configFile{}
6464
}
65+
if info, err := os.Stat(path); err == nil {
66+
if mode := info.Mode().Perm(); mode&0o077 != 0 {
67+
fmt.Fprintf(os.Stderr, "warning: %s is readable by others (mode %04o), consider chmod 600\n", path, mode)
68+
}
69+
}
6570
var cfg configFile
6671
if err := json.Unmarshal(data, &cfg); err != nil {
6772
return configFile{}

0 commit comments

Comments
 (0)